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

feat(processor): added ability for geolocation enrichment during pipeline processing #3866

Merged
merged 29 commits into from
Oct 25, 2023

Conversation

abhimanyubabbar
Copy link
Contributor

@abhimanyubabbar abhimanyubabbar commented Sep 14, 2023

Description

The change is adding a pipeline enricher ( geo enrichment ) when processing gateway jobs for a particular source. Below are sequence of events:

  • The customer will toggle a boolean flag on the Webapp which will augment the information at source level.
  • Rudder server will read this information and if pipeline enrichment is enabled, it will start enriching the geo information into the event based on the request IP.
  • We will be using unofficial Maxmind GeoIP libraries to read the Maxmind in memory database and the records will be then enriched accordingly.

Linear Ticket

Ticket

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@abhimanyubabbar abhimanyubabbar changed the title feat(dm): added ability for geolocation enrichment during pipeline pr… feat(dm): added ability for geolocation enrichment during pipeline processing Sep 14, 2023
Copy link
Member

@lvrach lvrach left a comment

Choose a reason for hiding this comment

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

Looks good to me so far, I would take another look once we add tests

@abhimanyubabbar abhimanyubabbar marked this pull request as ready for review October 12, 2023 10:28
@abhimanyubabbar abhimanyubabbar changed the title feat(dm): added ability for geolocation enrichment during pipeline processing feat(processor): added ability for geolocation enrichment during pipeline processing Oct 12, 2023
Copy link
Member

@lvrach lvrach left a comment

Choose a reason for hiding this comment

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

Looks good to me with some non-blocking comments.

Most important is to avoid t.Setenv in tests as you have already mentioned.

internal/enricher/geolocation.go Outdated Show resolved Hide resolved
internal/enricher/geolocation.go Outdated Show resolved Hide resolved
internal/enricher/geolocation.go Outdated Show resolved Hide resolved
internal/enricher/geolocation.go Outdated Show resolved Hide resolved
internal/enricher/geolocation.go Outdated Show resolved Hide resolved
internal/enricher/geolocation.go Outdated Show resolved Hide resolved
internal/enricher/geolocation.go Outdated Show resolved Hide resolved
processor/processor.go Outdated Show resolved Hide resolved
services/geolocation/types.go Outdated Show resolved Hide resolved
services/geolocation/types.go Outdated Show resolved Hide resolved
backend-config/types.go Outdated Show resolved Hide resolved
app/apphandlers/embeddedAppHandler.go Outdated Show resolved Hide resolved
app/apphandlers/processorAppHandler.go Outdated Show resolved Hide resolved
services/geolocation/maxmind.go Show resolved Hide resolved
services/geolocation/maxmind_test.go Outdated Show resolved Hide resolved
services/geolocation/maxmind_test.go Outdated Show resolved Hide resolved
services/geolocation/maxmind_test.go Outdated Show resolved Hide resolved
services/geolocation/maxmind_test.go Outdated Show resolved Hide resolved
internal/enricher/geolocation.go Outdated Show resolved Hide resolved
internal/enricher/geolocation.go Outdated Show resolved Hide resolved
internal/enricher/geolocation.go Outdated Show resolved Hide resolved
internal/enricher/geolocation.go Outdated Show resolved Hide resolved
internal/enricher/geolocation_test.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Attention: 65 lines in your changes are missing coverage. Please review.

Comparison is base (d0d99bc) 71.50% compared to head (d7c2ab1) 71.38%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3866      +/-   ##
==========================================
- Coverage   71.50%   71.38%   -0.13%     
==========================================
  Files         369      371       +2     
  Lines       54226    54473     +247     
==========================================
+ Hits        38777    38886     +109     
- Misses      13162    13274     +112     
- Partials     2287     2313      +26     
Files Coverage Δ
backend-config/types.go 88.88% <ø> (ø)
processor/manager.go 92.00% <100.00%> (+1.83%) ⬆️
testhelper/backendconfigtest/source_builder.go 84.61% <100.00%> (+1.28%) ⬆️
app/apphandlers/embeddedAppHandler.go 74.66% <66.66%> (-0.26%) ⬇️
app/apphandlers/setup.go 83.72% <75.00%> (-1.42%) ⬇️
processor/processor.go 88.52% <86.95%> (-0.11%) ⬇️
runner/runner.go 69.54% <50.00%> (+0.12%) ⬆️
app/apphandlers/processorAppHandler.go 7.57% <0.00%> (-0.27%) ⬇️
services/geolocation/maxmind.go 64.28% <64.28%> (ø)
internal/enricher/geolocation.go 79.51% <79.51%> (ø)

... and 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

go.mod Outdated Show resolved Hide resolved
internal/enricher/enricher.go Outdated Show resolved Hide resolved
internal/enricher/geolocation.go Outdated Show resolved Hide resolved
services/geolocation/types.go Outdated Show resolved Hide resolved
services/geolocation/maxmind.go Outdated Show resolved Hide resolved
services/geolocation/maxmind.go Outdated Show resolved Hide resolved
internal/enricher/geolocation.go Outdated Show resolved Hide resolved
internal/enricher/geolocation.go Show resolved Hide resolved
processor/processor.go Outdated Show resolved Hide resolved
processor/processor.go Outdated Show resolved Hide resolved
app/apphandlers/embeddedAppHandler.go Outdated Show resolved Hide resolved
internal/enricher/geolocation.go Outdated Show resolved Hide resolved
backend-config/types.go Show resolved Hide resolved
processor/processor.go Outdated Show resolved Hide resolved
processor/processor.go Outdated Show resolved Hide resolved
@abhimanyubabbar abhimanyubabbar force-pushed the feat.geolocation-enrichment branch 2 times, most recently from 9bc533f to 4567f7e Compare October 19, 2023 03:31
processor/processor.go Outdated Show resolved Hide resolved
processor/processor.go Outdated Show resolved Hide resolved
@abhimanyubabbar
Copy link
Contributor Author

Some open ended questions which I might need some help in answering would be @atzoum :

  • Should we attach lookup for empty IP's / invalid IP's ?
  • Should we add empty lookup if the lookup itself failed ?

The underlying question is that should we just add lookup to every event regardless of lookup succeeds or failed as it keeps parity on the event level flowing through the system or we prevent any lookup ?

@abhimanyubabbar abhimanyubabbar merged commit 28497cf into master Oct 25, 2023
35 checks passed
@github-actions github-actions bot deleted the feat.geolocation-enrichment branch December 26, 2023 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants