Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[processor/geoip] Add error_mode configuration option #35069

Open
rogercoll opened this issue Sep 9, 2024 · 7 comments
Open

[processor/geoip] Add error_mode configuration option #35069

rogercoll opened this issue Sep 9, 2024 · 7 comments
Labels
enhancement New feature or request processor/geoip

Comments

@rogercoll
Copy link
Contributor

rogercoll commented Sep 9, 2024

Component(s)

processor/geoip

Is your feature request related to a problem? Please describe.

When using the GeoIP processor with MaxMind provider, if an IP is detected in the attributes but not found in the internal databases, the processor will issue an error. Depending on the user's workflow, that might be expected (known IPs), but in some others the processor should continue the attributes processing (any IP can be processed).

Related issue: #35047

Context (previous proposal): #33319 (comment)

Describe the solution you'd like

A configuration option to ignore, propagate or silently skip the error, similar to the transform processor error_mode option:

error_mode description
ignore The processor ignores errors returned by statements, logs the error, and continues on to the next statement. This is the recommended mode.
silent The processor ignores errors returned by statements, does not log the error, and continues on to the next statement.
propagate The processor returns the error up the pipeline. This will result in the payload being dropped from the collector.

Describe alternatives you've considered

The ErrorMode configuration option resides on the ottl package, given that multiple components are willing to adopt this configuration option, should we decouple it to the ottl package and move it to a more collector's generic one?

Additional context

No response

@rogercoll rogercoll added enhancement New feature or request needs triage New item requiring triage labels Sep 9, 2024
Copy link
Contributor

github-actions bot commented Sep 9, 2024

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@crobert-1
Copy link
Member

This issue makes sense to me, and was filed by a code owner. Removing needs triage.

@crobert-1 crobert-1 removed the needs triage New item requiring triage label Sep 9, 2024
@andrzej-stencel
Copy link
Member

One situation is when the IP that was found in the telemetry record was not found in the GeoIP database. Another situation is when the IP was not found in the telemetry record in the first place (the attribute is missing or empty). Yet another situation is when the value of the attribute is not a valid IP.

I suppose the error_mode option should affect all these situations?

An alternative could be to have separate options for each of these situations, like on_missing_ip, on_invalid_ip, on_missing_geo, with possible values ignore|silent|propagate.

@strawgate
Copy link
Contributor

strawgate commented Sep 14, 2024

One of the challenges I see with this approach is that rfc1918 IP addresses (private IPs) are extremely common in source.address and will always fail an ip location lookup.

  1. I believe a user would not expect to have errors occurring simply because the location database does not contain a particular address.
  2. I believe a user would expect to see an indicator when invalid IP addresses are fed.
  3. I am unsure what a user would expect to see when the attribute is missing but it may depend on how difficult it is to exclude records from being processed by the processor.

Perhaps as @andrzej-stencel indicates, separate settings should be used or perhaps we have settings which decide if these cases are errors in the first place?

Perhaps we have error_mode with ignore|silent|propagate and then we have error_on_missing_ip, error_on_invalid_ip, and error_on_missing_geo that can be true|false. This way we can configure what is and isn't an error while still relying on the shared error_mode option to determine what happens if it is an error?

@andrzej-stencel
Copy link
Member

Even before we reach conclusion on how we want error mode to be configured, I think we should change the current behavior to not error out when IP is missing, is invalid or cannot be found in geo database. What do you think @rogercoll, @strawgate?

Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Nov 18, 2024
@strawgate
Copy link
Contributor

still active

@github-actions github-actions bot removed the Stale label Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request processor/geoip
Projects
None yet
Development

No branches or pull requests

4 participants