Conversation
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Attributes
Other
Bug Fixes 🐛Attributes
Other
Documentation 📚
Internal Changes 🔧Attributes
Deps Dev
Repo
Other
Other
🤖 This preview updates automatically when you update the PR. |
There was a problem hiding this comment.
Pull request overview
This PR adds semantic conventions for the culture context, which captures localization and regional settings. The changes introduce 5 new culture-related attributes that align with Sentry's culture context specification.
Changes:
- Added 5 new culture attributes:
culture.calendar,culture.display_name,culture.locale,culture.is_24_hour_format, andculture.timezone - Generated corresponding code in Python and JavaScript/TypeScript libraries
- Updated documentation to include the new culture attributes namespace
Reviewed changes
Copilot reviewed 7 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| model/attributes/culture/culture__calendar.json | Defines the calendar system attribute |
| model/attributes/culture/culture__display_name.json | Defines the human-readable culture name attribute |
| model/attributes/culture/culture__is_24_hour_format.json | Defines the 24-hour time format preference attribute |
| model/attributes/culture/culture__locale.json | Defines the locale identifier attribute |
| model/attributes/culture/culture__timezone.json | Defines the timezone attribute |
| python/src/sentry_conventions/attributes.py | Adds culture attributes to Python library with proper alphabetical ordering |
| javascript/sentry-conventions/src/attributes.ts | Adds culture attributes to JavaScript/TypeScript library with proper alphabetical ordering |
| generated/attributes/index.md | Adds culture namespace to attribute index |
| generated/attributes/culture.md | Generated documentation for culture attributes |
| generated/attributes/all.md | Updates the total attribute count and includes culture attributes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,10 @@ | |||
| { | |||
| "key": "culture.locale", | |||
| "brief": "The locale identifier following RFC 4646.", | |||
There was a problem hiding this comment.
The reference to RFC 4646 is outdated. RFC 4646 was obsoleted by RFC 5646 in 2009. The brief description should reference RFC 5646 or BCP 47 instead, which is the current standard for language tags. The example "en-US" correctly follows this standard.
| "brief": "The locale identifier following RFC 4646.", | |
| "brief": "The locale identifier following BCP 47 (RFC 5646).", |
Lms24
left a comment
There was a problem hiding this comment.
I would classify all of these attributes as pii: 'maybe'. Not because they strictly contain PII but because setting maybe allows users to define scrubbing rules in Relay (AFAIK).
There was a problem hiding this comment.
OTel defines browser.language which we could use instead.
That being said, scoping a value of sentry's culture context to browser isn't a great global strategy so I think we can just add this attribute as well for all SDKs. For the browser SDK, we could consider double-writing.
There was a problem hiding this comment.
I wasn't sure how to classify them, but makes sense. I will update that in rely definitions as well.
For OTEL's browser.language, does that block this PR or does it need a re-thinking in terms of including this whole context?
There was a problem hiding this comment.
No blocker from my PoV, just a thought!
e21d899 to
f1b1b42
Compare
Lms24
left a comment
There was a problem hiding this comment.
One more request, otherwise LGTM otherwise from my end
generated/attributes/all.md
Outdated
There was a problem hiding this comment.
let's remove the markdown files. We no longer need them after merging the docs website (#238)
If you rerun yarn:generate after rebaseing, these files shouldn't be generated anymore. Let me know in case they still show up.
There was a problem hiding this comment.
Weird, I did re-run them and it generated them again. I removed them anyways!
f1b1b42 to
ca8f3fb
Compare
Description
Add semantic conventions for the culture context, which captures localization and regional settings.
culture.calendarculture.display_nameculture.localeculture.is_24_hour_formatculture.timezonePR Checklist
yarn testand verified that the tests pass.yarn generate && yarn formatto generate and format code and docs.If an attribute was added:
culture.timezone)pii(We were sending those already so PII isfalseto all of them but happy to discuss and re-evaluate)Related to getsentry/sentry-javascript#19148 and getsentry/relay#5615