fix(basket): correct geo resolution telemetry + drop redis geoip cache - #589
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
The latest updates on your projects. Learn more about Unkey Deploy
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its |
Greptile SummaryThis PR removes Redis-backed, coarsened-IP GeoIP caching in favor of direct full-IP MaxMind lookups and adds resolution-source telemetry.
Confidence Score: 4/5The skipped-IP telemetry defect should be fixed before merging so the promised source-based accounting covers every request; precise city and region logging should also be reconsidered. The primary telemetry correction remains incomplete because the early skipped-IP return emits no source, while the MaxMind branch additionally broadens precise user-location data sent to operational telemetry. Files Needing Attention: apps/basket/src/utils/ip-geo.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[getGeo receives client IP] --> B{Invalid or local IP?}
B -- Yes --> C[Emit skipped metadata and return]
B -- No --> D[Direct MaxMind lookup using full IP]
D --> E{Country resolved?}
E -- Yes --> F[Emit source=maxmind and geo fields]
E -- No --> G{Cloudflare country header?}
G -- Yes --> H[Emit source=cloudflare_header]
G -- No --> I[Emit source=unresolved]
|
| mergeWideEvent({ | ||
| geo: geo.country | ||
| ? { | ||
| source: "maxmind", | ||
| country: geo.country, | ||
| region: geo.region, | ||
| city: geo.city, | ||
| } | ||
| : { source: "unresolved" }, | ||
| }); |
There was a problem hiding this comment.
Telemetry expands precise geo data
The new MaxMind merge sends user-derived city and region data to operational telemetry even for callers that never persist an analytics event. These fields are unnecessary for measuring resolution source and broaden the retention and audience exposure of precise location data.
| mergeWideEvent({ | |
| geo: geo.country | |
| ? { | |
| source: "maxmind", | |
| country: geo.country, | |
| region: geo.region, | |
| city: geo.city, | |
| } | |
| : { source: "unresolved" }, | |
| }); | |
| mergeWideEvent({ | |
| geo: geo.country | |
| ? { | |
| source: "maxmind", | |
| country: geo.country, | |
| } | |
| : { source: "unresolved" }, | |
| }); |
Knowledge Base Used: Basket Ingestion Flow
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
What
Two slices fixing the basket IP-geo pipeline.
1. Geo resolution telemetry blind spot (
fix)The Axiom geo dashboard showed 0.4% resolution / 17.1M "failed to locate" — a measurement artifact, not an outage.
getGeo()only merged ageo.*wide-event field on the Cloudflare-fallback and skipped-IP paths. On the MaxMind success path (~99.5% of traffic) it returned the country but never logged it, so Axiom recordedgeo.country = nulland the dashboard read those as failures.Evidence (7d,
basketdataset):geo.source=cloudflare_header= 76,957 = exactly the "resolved" count; zero GeoIP load errors; MaxMind lookups run on all 17.18M events at 0.26ms p50; flat 30-day trend with no cliff.Fix: every request now emits a
geo.source—maxmind/cloudflare_header/unresolved/skipped.2. Drop the Redis GeoIP cache (
perf)timing.getGeop50 was 142ms while the in-memory MaxMind lookup is 0.26ms — thecacheableRedis wrapper was ~500x slower than the value it cached. Removed the cache and the/24coarsenIpForCachehelper; lookups now hit the in-process reader on the full IP, restoring city/region precision the coarsening discarded.Test
bun test src/utils/ip-geo.test.ts-> 20 pass;check-typesgreen across all packages.Follow-up (dashboard owner)
Update the Axiom panels to compute resolution from
geo.source(maxmind+cloudflare_header= resolved) instead ofisnull(geo.country).Summary by cubic
Fixes geo resolution telemetry by always emitting
geo.source, and removes the Redis GeoIP cache to use the in-process MaxMind lookup. This corrects false “failed to locate” readings in Axiom and cuts lookup p50 from ~142ms to ~0.26ms while restoring city/region precision.geo.source(maxmind+cloudflare_header= resolved) instead ofisnull(geo.country).Written for commit 1aa2b32. Summary will update on new commits.