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.

Sign up for GitHub

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

Jump to bottom

fix(spans): check sdk platform for user.geo.subregion tag #4023

Merged
merged 3 commits into from
Sep 11, 2024
Merged

fix(spans): check sdk platform for user.geo.subregion tag #4023

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
27fbb4d
check platform
DominikB2014 Sep 10, 2024
03188a3
changelog
DominikB2014 Sep 10, 2024
8c46667
Update CHANGELOG.md
DominikB2014 Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
- Remove the `generate-schema` tool. Relay no longer exposes JSON schema for the event protocol. Consult the Rust type documentation of the `relay-event-schema` crate instead. ([#3974](https://github.com/getsentry/relay/pull/3974))
- Allow creation of `SqliteEnvelopeBuffer` from config, and load existing stacks from db on startup. ([#3967](https://github.com/getsentry/relay/pull/3967))
- Only tag `user.geo.subregion` on frontend and mobile projects. ([#4013](https://github.com/getsentry/relay/pull/4013))
- Check if sdk platform is `javascript` for `user.geo.subregion` tag. ([#4023](https://github.com/getsentry/relay/pull/4023))
DominikB2014 marked this conversation as resolved.
Show resolved Hide resolved

## 24.8.0

Expand Down
8 changes: 6 additions & 2 deletions relay-event-normalization/src/normalize/span/tag_extraction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,8 +320,9 @@ fn extract_shared_tags(event: &Event) -> BTreeMap<SpanTagKey, String> {
}

// We only want this on frontend or mobile modules.
let should_extract_geo =
event.context::<BrowserContext>().is_some() || MOBILE_SDKS.contains(&event.sdk_name());
let should_extract_geo = (event.context::<BrowserContext>().is_some()
&& event.platform.as_str() == Some("javascript"))
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this still be a javascript backend client? Or is python the only backend SDK that sets browser context?

Copy link
Contributor Author

@DominikB2014 DominikB2014 Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jjbayer Yes but this is not a super common case, because most javascript backends have platform set to "node" instead of javascript. This is a pretty easy change to add, so I think we can see how much of an improvement it makes before moving onto more improvements.

|| MOBILE_SDKS.contains(&event.sdk_name());

if should_extract_geo {
if let Some(country_code) = user.geo.value().and_then(|geo| geo.country_code.value()) {
Expand Down Expand Up @@ -2745,6 +2746,9 @@ LIMIT 1
"trace": {
"trace_id": "ff62a8b040f340bda5d830223def1d81",
"span_id": "bd429c44b67a3eb4"
},
"browser": {
"name": "Chrome"
}
},
"user": {
Expand Down
Loading