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

Promtail: Add 'country' to geoip pipeline-stage #11378

Closed
wants to merge 5 commits into from

Conversation

superstes
Copy link

@superstes superstes commented Dec 3, 2023

What this PR does / why we need it:

Maxmind provides multiple GeoIP2 databases.
The three most used ones are City, Country and ASN as they are also available as free GeoLiteIP2 databases.

Some users might want/need to use the 'Country' database - so I added support for it.
A reason: If the user only has the 'Country' database licensed.

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
    Skipped because it's a simple enhancement

@superstes superstes requested a review from a team as a code owner December 3, 2023 19:20
@CLAassistant
Copy link

CLAassistant commented Dec 3, 2023

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Dec 3, 2023
Copy link
Contributor

github-actions bot commented Dec 3, 2023

Trivy scan found the following vulnerabilities:

@superstes
Copy link
Author

Note: Also related to the fix of PR #10256

@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 4, 2023
@superstes
Copy link
Author

superstes commented Dec 4, 2023

I've now also added database-specific tests.

To note: The minimal (1 record) testdata-databases are added for the sake of simplicity.

One could also:

  • add the mmdbwriter as (test-)dependency and build those database-binaries at test-runtime from JSON
  • or download the maxmind testdata on each test-run

@cstyan
Copy link
Contributor

cstyan commented Dec 4, 2023

Hello @superstes , 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. Additionally, the agent team is more likely to have time to dedicate to your PR.

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.

@superstes
Copy link
Author

@cstyan Thanks for information.
Is there any public discussion regarding this promtail/agent topic?
I would be interested in it as I'm currently implenting a Syslog => Promtail => Loki <= Grafana solution in one of my projects.

@cstyan
Copy link
Contributor

cstyan commented Dec 5, 2023

@superstes public discussion in what sense?

I'll also point out that the agent has a hard fork of promtail code, so you can likely make a very similar improvement there: https://github.com/grafana/agent/blob/b7f7c57a9420af409508eb3acc5618371b3f8107/component/loki/process/stages/geoip.go

@superstes
Copy link
Author

superstes commented Dec 5, 2023

@cstyan 'Discussion' might've been the wrong word.
I meant: 'Where could one ask questions regarding the comparison/overlap of grafana-agent and promtail?'

Per example:

  • I would guess grafana-agent does not (yet) implement all functionality of promtail, or does it?
  • How to 'translate/port' specific promtail setups/config to grafana-agent

@JStickler
Copy link
Contributor

There should be feature parity because the Agent team took a fork of Promtail back in March 2023 and incorporated it into Grafana Agent. We’re just starting the process of trying to point requests for new features to grafana/agent.
https://grafana.com/docs/agent/latest/flow/getting-started/migrating-from-promtail/

@superstes
Copy link
Author

superstes commented Dec 6, 2023

@JStickler Thank you for the information.
Then I'll take the time and migrate my setup to the agent and create a PR at the grafana/agent repository.

@cstyan
Copy link
Contributor

cstyan commented Dec 19, 2023

closing since it looks like your agent pr was merged, thanks for your time @superstes we appreciate it :)

@cstyan cstyan closed this Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants