feat(browser): Include culture context with events#19148
feat(browser): Include culture context with events#19148
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Codecov Results 📊Generated by Codecov Action |
size-limit report 📦
|
logaretm
force-pushed
the
awad/js-1455-capture-culture-context-from-browser-sdks
branch
from
1b4fd0d to
94966b7
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a new cultureContextIntegration to the browser SDK that automatically captures culture-related information (locale, timezone, and calendar) from the browser's Intl.DateTimeFormat API and includes it in all events sent to Sentry.
Changes:
- Adds a new
cultureContextIntegrationthat captures locale, timezone, and calendar information from the browser - Registers the integration as a default integration in the browser SDK
- Updates test expectations across multiple test suites to account for the new culture context
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/browser/src/integrations/culturecontext.ts | Implements the new culture context integration using the Intl API |
| packages/browser/src/sdk.ts | Adds cultureContextIntegration to default integrations |
| packages/browser/src/index.ts | Exports the new integration for public use |
| dev-packages/e2e-tests/test-applications/nextjs-pages-dir/tests/transactions.test.ts | Updates test expectations to use expect.objectContaining for contexts |
| dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/transactions.test.ts | Updates test expectations to use expect.objectContaining for contexts |
| dev-packages/browser-integration-tests/utils/replayEventTemplates.ts | Adds culture context expectations to replay event templates |
| dev-packages/browser-integration-tests/suites/replay/captureReplayFromReplayPackage/test.ts | Adds culture context expectations to replay tests |
| dev-packages/browser-integration-tests/suites/replay/captureReplay/test.ts | Adds culture context expectations to replay tests |
| dev-packages/browser-integration-tests/suites/public-api/startSpan/standalone-mixed-transaction/test.ts | Updates test expectations to use expect.objectContaining for contexts |
| dev-packages/browser-integration-tests/suites/public-api/debug/test.ts | Adds CultureContext to expected integration logs |
| dev-packages/browser-integration-tests/suites/integrations/cultureContext/test.ts | Adds new integration test verifying culture context capture |
| dev-packages/browser-integration-tests/suites/integrations/cultureContext/subject.js | Test subject file that triggers an error for testing |
| dev-packages/browser-integration-tests/suites/integrations/cultureContext/init.js | Test initialization file that sets up the CultureContext integration |
| dev-packages/browser-integration-tests/suites/feedback/captureFeedbackCsp/test.ts | Adds culture context expectations to feedback tests |
| dev-packages/browser-integration-tests/suites/feedback/captureFeedbackAndReplay/hasSampling/test.ts | Adds culture context expectations to feedback tests |
| dev-packages/browser-integration-tests/suites/feedback/captureFeedback/test.ts | Adds culture context expectations to feedback tests |
| dev-packages/browser-integration-tests/suites/feedback/attachTo/test.ts | Adds culture context expectations to feedback tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| calendar: options.calendar, | ||
| }; | ||
| } catch { | ||
| // Ignore errors |
There was a problem hiding this comment.
The calendar property from resolvedOptions() is included here but is not included in the Node.js implementation (packages/node-core/src/integrations/context.ts:190-193). Consider checking whether calendar is consistently available across all browsers, as it may not be present in older browsers or certain browser configurations. If consistency with the Node.js implementation is desired, or if browser compatibility is a concern, the calendar property could be made optional or omitted. The Node.js implementation only includes locale and timezone.
| calendar: options.calendar, | |
| }; | |
| } catch { | |
| // Ignore errors | |
| }; | |
| } catch { | |
| // Ignore errors | |
| // Ignore errors |
| linkedErrorsIntegration(), | ||
| dedupeIntegration(), | ||
| httpContextIntegration(), | ||
| cultureContextIntegration(), |
There was a problem hiding this comment.
q: Is that the only sdk where we need to add this separately?
There was a problem hiding this comment.
Yep, I checked the other SDKs and they don't override the browser's default integrations. Only the angular one does.
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
I think this is a nice addition, thanks for taking care of it. One more context to convert to attributes for Spans though 😅 but that's a me/span-first problem to worry about.
I'm not approving yet because we should first check with Ingest if anything gets inferrerd from the request. LGTM besides that.
cleptric
left a comment
cleptric
left a comment
There was a problem hiding this comment.
Please open a Conventions PR prior merging this.
4 participants
This PR includes the
culturecontext containing thelocale,timeZoneandcalendarinformation as outlined by the Contexts interface doc.I had this lying in my stash since the last Triage, just got some time to tidy it up and PR it.
closes #18801