[analytics] Add browser request accept-language#4060
Conversation
WalkthroughModified Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Backend/Otel/OtelKernel.cs (1)
85-88: Accept-Language enrichment is solid; consider avoiding magic string and double-check consent expectationsThe new
Accept-Languagetagging is implemented safely withTryGetValueand produces a sensible string vialangs.ToString(). Two minor follow-ups to consider:
- Use a constant (e.g.,
Microsoft.Net.Http.Headers.HeaderNames.AcceptLanguage) instead of the raw"Accept-Language"literal to avoid typos and ease reuse.- Today this tag is added for all requests, regardless of
analyticsOn/consent. If your privacy/analytics policy expects all analytics-related attributes (even “non-PII” ones) to be gated by consent, you may want to condition this on the same consent signal used elsewhere; if Accept-Language is explicitly allowed outside consent, current behavior is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Backend/Otel/OtelKernel.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: docker_build
- GitHub Check: test_build (8.0.x)
- GitHub Check: tox (3.12)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
Backend/Otel/OtelKernel.cs (1)
101-103: Null-safe query check refactor looks goodThe new
if (!string.IsNullOrEmpty(request.RequestUri?.Query))keeps prior behavior while simplifying the null checks and still avoidsNullReferenceExceptionwhenRequestUriis unset. No further changes needed from my side.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4060 +/- ##
===========================================
+ Coverage 74.55% 85.66% +11.10%
===========================================
Files 294 54 -240
Lines 10897 4763 -6134
Branches 1366 587 -779
===========================================
- Hits 8124 4080 -4044
+ Misses 2378 539 -1839
+ Partials 395 144 -251
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:
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Backend/Otel/OtelKernel.cs (1)
80-86: Avoid setting empty Accept-Language tag; consider consent alignment
request.Headers.AcceptLanguageis a valid typed property onIHeaderDictionaryand will compile. However, calling.ToString()on a missing header returns an empty string rather than skipping the tag. Consider checking for presence before tagging:- TrackSession(activity, request); - activity.SetTag("http.request.header.accept_language", request.Headers.AcceptLanguage.ToString()); + TrackSession(activity, request); + var acceptLanguage = request.Headers.AcceptLanguage; + if (!StringValues.IsNullOrEmpty(acceptLanguage)) + { + activity.SetTag("http.request.header.accept_language", acceptLanguage.ToString()); + }Additionally, if product requirements mandate that this header should only be captured when analytics consent is enabled, gate this on the same consent signal already checked in
TrackConsent().
🧹 Nitpick comments (1)
Backend/Otel/OtelKernel.cs (1)
95-101: HttpClient query tagging change looks good; keep an eye on PII in queriesThe simplified null/empty check for
request.RequestUri?.Queryis correct and keeps the behavior (only tag when a non-empty query exists) while reducing nesting.requestis non-null in this callback, so the remaining null-conditional onRequestUriis sufficient.Given you are tagging the full
url.query, just ensure that upstream callers don’t place secrets or sensitive PII in query strings, or consider redaction if that’s a risk.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Backend/Otel/OtelKernel.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: docker_build
- GitHub Check: test_build (8.0.x)
- GitHub Check: Analyze (csharp)
- GitHub Check: tox (3.12)
jasonleenaylor
left a comment
There was a problem hiding this comment.
@jasonleenaylor reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)
Enables browser language analytics, non-PII analytics requested by SIL leadership.
This change is
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.