loki
loki copied to clipboard
Promtail: Include geoip fields in the extracted map
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.mdguide (required) - [x] Documentation added
- [ ] Tests updated
- [ ]
CHANGELOG.mdupdated- [ ] If the change is worth mentioning in the release notes, add
add-to-release-noteslabel
- [ ] If the change is worth mentioning in the release notes, add
- [ ] 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.yamland updateproduction/helm/loki/CHANGELOG.mdandproduction/helm/loki/README.md. Example PR
That allows the fields to be usable in other stages, like
packandtemplatewhich can be used to store the fields in the message output.
I think it would be helpful to add an example of this ^.
I think it would be helpful to add an example of this ^.
Thanks for the feedback. I've added two examples to the documentation.
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.
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.
No update as of right now.
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.
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
This PR fullfill a need that we have on log analysis What can we do to help the process ?
geoip fields to extracted map is necessary for log analysis,please consider merge the pr
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.
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)