feat: geoip decorator support - #1027
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1027 +/- ##
==========================================
- Coverage 47.15% 47.09% -0.07%
==========================================
Files 258 260 +2
Lines 26981 27286 +305
==========================================
+ Hits 12722 12849 +127
- Misses 13413 13575 +162
- Partials 846 862 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
829f2dc to
9d69afa
Compare
|
hey @theSuess this looks like a really cool feature can we use that instead of implementing it in obi, or does this give abilities we can't do with the processor? |
|
By implementing it in OBI, we get the grouping out of the box, while it would be more complicated to do it in the collector. For example, this config will only preserve the country looked up: Which would involve multiple processing steps when doing it on the collector layer. For me the biggest advantage is in ease of use, though I can see the advantage of centralizing lookup in the collector |
|
Amazing contribution @theSuess !! Tank you very much. We will review it carefully and come back to you with some comments. Please notice that this process might be slower than usual due to the holiday season. I have a question regarding performance. Do you think is worth caching in-memory the most frequent IP lookups to minimize the access to the local GeoIP database? Or is the |
|
I didn't check the underlying lookup performance too closely, but I can write a benchmark to make sure this is fast enough and put a cache in between if not |
9d69afa to
938e47e
Compare
|
I ran a few benchmarks and caching does speed up things quite a bit, especially for the maxmind DB as it requires two lookups instead of one. If the cache size is too small for the amount of IPs processed, it becomes a bit slower but only by a few ns The latest commit includes these benchmarks and an updated implementation that uses the cache |
grcevski
left a comment
grcevski
left a comment
There was a problem hiding this comment.
It looks like there are some lint issues.
1f98961 to
a1d2f3e
Compare
|
Lint issues should be fixed now. The ARM integration tests never failed before, so a restart should turn this check green (don't have the permission to do so myself) |
mariomac
left a comment
mariomac
left a comment
There was a problem hiding this comment.
Thank you a lot for your contribution! I have few minor comments related to the logging.
| for _, flow := range flows { | ||
| srcInfo, err := cachedLookup(flow.Id.SrcIP()) | ||
| if err != nil { | ||
| log.Warn("failed to perform geoip lookup for source", "err", err) |
There was a problem hiding this comment.
Could this flood the user output? In this case I'd either:
- set this log as
Debuglevel - or print the first message as warning, then the rest of messages as debug.
| } | ||
| dstInfo, err := cachedLookup(flow.Id.DstIP()) | ||
| if err != nil { | ||
| log.Warn("failed to perform geoip lookup for destination", "err", err) |
There was a problem hiding this comment.
Same as for the previous log.Warn
a1d2f3e to
33c236f
Compare
mariomac
left a comment
mariomac
left a comment
There was a problem hiding this comment.
LGTM! Thanks for addressing the changes
|
Thanks for your contribution @theSuess ! |
5 participants
This PR adds support for GeoIP lookups using ipinfo.io or MaxMind GeoIP2-Lite.
By using this decorator, users can get more insights into traffic patterns without exploding the metric cardinality. An example metric looks like this:
This functionality is enabled by setting the path to either the IPInfo Lite or GeoIP2 Lite databases.
We can't bundle the MaxMind database due to licensing restrictions (as discussed in open-telemetry/opentelemetry-collector-contrib#33510 (comment)) but it should be possible for ipinfo as it's licensed under the
Creative Commons Attribution-ShareAlike 4.0license.The files used for testing are the MaxMind test-data and the IPInfo sample-database