loki icon indicating copy to clipboard operation
loki copied to clipboard

Promtail: Include geoip fields in the extracted map

Open esev opened this issue 2 years ago • 6 comments

What this PR does / why we need it:

The geoip parsing stage currently only populates the labelset with fields. This makes it difficult to use as fields like geoip_location_latitude/geoip_location_longitude and geoip_autonomous_system_number due to them having high cardinality. In addition, stages that operate on the extracted map cannot access these fields.

This PR adds the fields to the extracted map as well. That allows the fields to be usable in other stages, like pack & template, which can be used to store the fields in the message output.

Which issue(s) this PR fixes: Related to #9076 (Partial fix: adds to the extracted map, doesn't remove from labels)

Checklist

  • [x] Reviewed the CONTRIBUTING.md guide (required)
  • [x] Documentation added
  • [ ] Tests updated
  • [ ] CHANGELOG.md updated
    • [ ] If the change is worth mentioning in the release notes, add add-to-release-notes label
  • [ ] Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • [ ] For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR

esev avatar Aug 16 '23 01:08 esev

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Aug 16 '23 01:08 CLAassistant

That allows the fields to be usable in other stages, like pack and template which can be used to store the fields in the message output.

I think it would be helpful to add an example of this ^.

JStickler avatar Aug 16 '23 18:08 JStickler

I think it would be helpful to add an example of this ^.

Thanks for the feedback. I've added two examples to the documentation.

esev avatar Aug 17 '23 00:08 esev

Hello, thanks for your PR.

We're currently reevaluating promtails position as a project within Grafana Labs. Internally we're actually using the Agent for both metrics and logs collection at this point. Potentially we can consider this a bugfix and move forward, I'll consult with the rest of the team.

While we haven't made a formal decision yet, we expect in the near future that all new feature work will be done in the Agent's log collection pipelines rather than in Promtail.

cstyan avatar Nov 07 '23 20:11 cstyan

Good day. I just want to check if there have been any decisions made that impacts if this PR will be merged or not? I am running into cardinality issues with some of the log streams I am using with geoip, even after dropping most of the labels.

longtomjr avatar Feb 16 '24 07:02 longtomjr

No update as of right now.

cstyan avatar Feb 19 '24 19:02 cstyan

Hi there, any update on the status of this PR?

Fixing this seems like a vital bugfix to keep label low-cardinality when using the geoip parsing stage in promtail. Unless I am missing something and there is another way to use geoip safely @cstyan ?

Additionally (it was initially unclear to me), without this fix the issue is exhibited even when using the newer Grafana Agent in static mode. I only found this out after going to the effort of migrating from promtail to agent after reading your comment above.

itsmejoeeey avatar Apr 01 '24 07:04 itsmejoeeey

With the agent now becoming alloy 1.0 (link) promtail is in a "feature complete" status, as I mentioned above. I can still check in on whether we'd include this as a bug fix, if the current state of the geoip stage results in the labels being unusable anyways due to cardinality then I think it's reasonable.

@grafana/agent-squad can you track a separate issue for confirming/fixing the same behaviour in the agent/alloy? thanks

cstyan avatar Apr 12 '24 00:04 cstyan

This PR fullfill a need that we have on log analysis What can we do to help the process ?

kyrianae avatar Apr 12 '24 12:04 kyrianae

geoip fields to extracted map is necessary for log analysis,please consider merge the pr

imtipi avatar Apr 12 '24 12:04 imtipi

We're okay with moving forward with this PR as a fix rather than a feature. IMO we probably should have some basic tests here, @esev I know this has been sitting here for a while please LMK if you have some time to add tests, otherwise I can try and find to add some myself.

cstyan avatar Apr 15 '24 20:04 cstyan

We're okay with moving forward with this PR as a fix rather than a feature. IMO we probably should have some basic tests here, @esev I know this has been sitting here for a while please LMK if you have some time to add tests, otherwise I can try and find to add some myself.

i kept using this fix since pr appeared,and no crash so far(although I know this isn't the professional test)

imtipi avatar Apr 17 '24 11:04 imtipi