Skip to content

Implement the geoip context #3731

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

Merged
merged 21 commits into from
Dec 19, 2023
Merged

Implement the geoip context #3731

merged 21 commits into from
Dec 19, 2023

Conversation

Dakostu
Copy link

@Dakostu Dakostu commented Dec 12, 2023

This change implements the geoip context - a built-in that reads MaxMind DB files and uses IP values in events to enrich them with the MaxMind DB geolocation data.

@Dakostu Dakostu added the feature New functionality label Dec 12, 2023
@Dakostu Dakostu requested a review from a team December 12, 2023 11:27
@Dakostu Dakostu marked this pull request as ready for review December 12, 2023 11:29
@Dakostu Dakostu force-pushed the topic/geoip-context branch 12 times, most recently from 8f0725d to e9ac320 Compare December 15, 2023 12:06
@tobim
Copy link
Member

tobim commented Dec 15, 2023

The diff here should hopefully fix the static binary build:

diff --git a/nix/overlay.nix b/nix/overlay.nix
index e3c04f88be..71f1e92e81 100644
--- a/nix/overlay.nix
+++ b/nix/overlay.nix
@@ -168,6 +168,9 @@ in {
     prev.writeShellScriptBin name ''
       echo "stub-${name}: $@" >&2
     '';
+  libmaxminddb = overrideAttrsIf isStatic prev.libmaxminddb (orig: {
+    nativeBuildInputs = (orig.nativeBuildInputs or []) ++ [prev.buildPackages.cmake];
+  });
   fluent-bit = let
     fluent-bit' =
       overrideAttrsIf isDarwin prev.fluent-bit

@Dakostu Dakostu force-pushed the topic/geoip-context branch from e9ac320 to d5c8960 Compare December 15, 2023 12:45
@mavam mavam added the engine Core pipeline and storage engine label Dec 15, 2023
Copy link
Member

@tobim tobim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already looking pretty good. I did not look at the conversion cases in detail, for those we need a functional test that hits all of the code paths.

Copy link
Member

@tobim tobim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that we already have tests in the private repo.

Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well in practice; mostly questions now to get the UX right.

@Dakostu Dakostu force-pushed the topic/geoip-context branch from bbeaf8f to 51329fa Compare December 15, 2023 15:52
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready to be shipped. You likely need to rebase the context plugin changes after my blocking operators PR, though.

@Dakostu
Copy link
Author

Dakostu commented Dec 19, 2023

Doing that right now as we speak.

@Dakostu Dakostu force-pushed the topic/geoip-context branch from ef265b9 to 3af5b3a Compare December 19, 2023 09:41
@Dakostu Dakostu enabled auto-merge December 19, 2023 09:47
@Dakostu Dakostu force-pushed the topic/geoip-context branch from 3af5b3a to 4cfcee9 Compare December 19, 2023 11:42
@Dakostu Dakostu disabled auto-merge December 19, 2023 11:48
@Dakostu Dakostu enabled auto-merge December 19, 2023 12:03
Daniel Kostuj and others added 3 commits December 19, 2023 14:04
Our integration test framework does not resolve relative data paths
correctly for externally-built plugins. This causes the tests to fail in
CI that work well locally. We should port these to the new framework,
but that can wait for the next PR.
@dominiklohmann dominiklohmann merged commit 3d192ce into main Dec 19, 2023
@dominiklohmann dominiklohmann deleted the topic/geoip-context branch December 19, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engine Core pipeline and storage engine feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants