ref(tracing): Remove startChild calls from fetch/xhr instrumentation#10236
Merged
AbhiPrasad merged 2 commits intodevelopfrom Jan 24, 2024
Merged
ref(tracing): Remove startChild calls from fetch/xhr instrumentation#10236AbhiPrasad merged 2 commits intodevelopfrom
AbhiPrasad merged 2 commits intodevelopfrom
Conversation
Contributor
size-limit report 📦
|
mydea
reviewed
Jan 18, 2024
| : undefined; | ||
|
|
||
| const span = shouldCreateSpanResult | ||
| ? startInactiveSpan({ |
Member
There was a problem hiding this comment.
I guess there is no easy way to make these active spans 🤔 They aren't now either, so I guess it's OK, but just thinking, for the future!
Contributor
Author
There was a problem hiding this comment.
I think we don't want to make them active spans because they are async, it's not ideal, but something we can come back to later on.
mydea
approved these changes
Jan 18, 2024
Lms24
approved these changes
Jan 18, 2024
Comment on lines
+282
to
+284
| type: 'xhr', | ||
| 'http.method': sentryXhrData.method, | ||
| url: sentryXhrData.url, |
Member
There was a problem hiding this comment.
just to make sure: Do these attribute keys need sentry. prefixes?
If not, is there a source where we define which attributes need a prefix vs which don't? I assume it's somewhere here?
Contributor
Author
There was a problem hiding this comment.
These attribute keys do not need sentry.X because they are not specific to sentry product or schema. The conventions you linked are the list so far!
These unit tests are brittle, and given we have browser integration tests we can rely on those instead.
2178b2c to
dde8811
Compare
AbhiPrasad
commented
Jan 24, 2024
| }); | ||
|
|
||
| describe('callbacks', () => { | ||
| let hub: Hub; |
Contributor
Author
There was a problem hiding this comment.
I ended up deleting these tests because they are so bound to hub/transaction/span current implementations.
We have playwright tests that cover testing these spans, those are good enough to validate that we have the expected behaviour.
anonrig
approved these changes
Jan 24, 2024
AbhiPrasad
added a commit
that referenced
this pull request
Jan 29, 2024
This was a regression introduced with #10236, we shouldn't arbitrarily call `startInactiveSpan` given we create transactions under the hood currently.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These are pretty easy.
The rest of the
startChildcalls in this package are either node integrations (which we don't want to touch) or part ofBrowserTracing, which needs independent look at in general.