[geoip] refactor maxmind provider lookup functions

[hwyuan]

Commit Message: refactor maxmind provider lookup functions
Additional Description:
Extracted common maxmind db lookup functions into lookupInMaxmindDb
Introduced isLookupEnabledForDbType and populateGeoLookupResultForDbType to handle db-specific logics
Risk Level: low
Testing: TODO
Docs Changes:
Release Notes:
Platform Specific Features:

[repokitteh-read-only]

Hi @hwyuan, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #31490 was opened by hwyuan.

see: more, trace.

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #31490 was opened by hwyuan.

see: more, trace.

[hwyuan]

RFC: MaxmindDbPtr is a unique_ptr, so we are assuming the pointer is always available in this function call, would be something to pay attention to when db pointer is reloaded (such as in hot reload)

[ravenblackx]

There is no point in taking a const reference to a smart pointer - if you're going to require that it exists for the duration of the call, the function should just take a reference to a MaxmindDb.

[hwyuan]

right now stat_prefix doesn't add much value since it's always the same as db_type, if we are ok with db_type using a string (vs. enum), then we could probably combine these two fields

[nezdolik]

Don't think you need to pass/use stat_prefix here, as all maxmind stats is created in stat scope that already has stat_prefix as part of its name: https://github.com/envoyproxy/envoy/pull/31490/files#diff-d21c11080878cdf65727845da9fa3846d138efb810faeb12e8d16006713446deL30. Tagging @jmarantz as stats expert to confirm

[ravenblackx]

Use absl::string_view for read-only string parameters, it makes the function more flexible. (In that if the caller has a string_view or a constant and the function takes a std::string&, the caller has to create a string object.)

(Throughout - this actually already makes a difference since the caller is passing a bare string constant, which means it is unnecessarily allocating a temporary string object.)

Or since it's an internal implementation detail, make it an enum, and avoid all the string compares.

Or, since it's actually only ever used as a constant and branched like a switch, it would probably make the most sense to redo the whole thing as an enum template function, something like

class enum DbType {CITY, ISP, ANON};

template <DbType T> bool isLookupEnabledForDbType() const;
template <DbType T> void populateGeoLookupResult(
    MMDB_lookup_result_s& mmdb_lookup_result,
    absl::flat_hash_map<std::string, std::string>& lookup_result) const;
template <DbType T> void lookupInMaxMindDb(...);

Implementing specializations for the first two (in lieu of the branches) and using the type to explicitly call with the appropriate type at the callsites.

(I would suggest actually making the specialized template functions non-member functions that take the required members as parameters, so that they don't have to be templated in the header, at least as far as that is possible. It would work for isLookupEnabled but I haven't looked closely to see if it's possible for populateGeoLookupResult)

[ravenblackx]

There is no point in taking a const reference to a smart pointer - if you're going to require that it exists for the duration of the call, the function should just take a reference to a MaxmindDb.

[nezdolik]

Don't think you need to pass/use stat_prefix here, as all maxmind stats is created in stat scope that already has stat_prefix as part of its name: https://github.com/envoyproxy/envoy/pull/31490/files#diff-d21c11080878cdf65727845da9fa3846d138efb810faeb12e8d16006713446deL30. Tagging @jmarantz as stats expert to confirm

[nezdolik]

since all populateGeoLookupResultFor*Db methods have very similar structure, would it be possible to refactor them into single method?

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[github-actions]

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[hwyuan]

seems PR is closed, possible to re-open or in this case sending a new PR is preferred?

[phlax]

@hwyuan reopened

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[github-actions]

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!