ref(tracing): Remove startChild calls from fetch/xhr instrumentation #10236
Conversation
AbhiPrasad
requested review from
a team,
anonrig and
Lms24
and removed request for
a team
size-limit report 📦
|
| : undefined; | ||
|
|
||
| const span = shouldCreateSpanResult | ||
| ? startInactiveSpan({ |
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!
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.
| type: 'xhr', | ||
| 'http.method': sentryXhrData.method, | ||
| url: sentryXhrData.url, |
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?
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.
AbhiPrasad
force-pushed
the
abhi-tracing-internal
branch
from
2178b2c
to
dde8811
Compare
| @@ -59,363 +41,6 @@ describe('instrumentOutgoingRequests', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('callbacks', () => { | |||
| let hub: Hub; | |||
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.
This was a regression introduced with #10236, we shouldn't arbitrarily call `startInactiveSpan` given we create transactions under the hood currently.
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.