-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
feat(core): Add updateSpanName
helper function
#14291
base: develop
Are you sure you want to change the base?
Conversation
size-limit report 📦
|
70674de
to
c7b6718
Compare
❌ 2 Tests Failed:
View the top 2 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
@@ -172,6 +296,26 @@ describe('descriptionForHttpMethod', () => { | |||
source: 'url', | |||
}, | |||
], | |||
[ | |||
'works with prefetch request', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just an additional test I added because we didn't test the condition to add .prefetch
to the op
.
...tegration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/subject.js
Outdated
Show resolved
Hide resolved
@@ -91,6 +91,14 @@ export function createEventEnvelope( | |||
// have temporarily added, etc. Even if we don't happen to be using it at some point in the future, let's not get rid | |||
// of this `delete`, lest we miss putting it back in the next time the property is in use.) | |||
delete event.sdkProcessingMetadata; | |||
try { | |||
// @ts-expect-error - for bundle size we try/catch the access to this property | |||
delete event.contexts.trace.data._sentry_span_name_set_by_user; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: Instead of doing this here, can we handle this in otel? We have a function removeSentryAttributes
there already that gets rid of "internal" attributes, IMHO we can just add to this? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately I'm afraid we have to do this in core because people can call updateSpanName
in browser SDKs as well.
if (shouldSkipTracingTest()) { | ||
sentryTest.skip(); | ||
} | ||
sentryTest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this PR supposed to include changes to browser tracing (tests)? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. We have to export updateSpanName
also from browser for people who'd use this function in code that runs both on browser and on server. So I added an integration test for it. This test, I adjusted. The changes look scarier in the diff than they actually are. Just updated the name and added some additional assertions.
// if http.method exists, this is an http request span | ||
// eslint-disable-next-line deprecation/deprecation | ||
const httpMethod = attributes[ATTR_HTTP_REQUEST_METHOD] || attributes[SEMATTRS_HTTP_METHOD]; | ||
if (httpMethod) { | ||
return descriptionForHttpMethod({ attributes, name, kind }, httpMethod); | ||
return descriptionForHttpMethod({ attributes, name: getOriginalName(spanName, attributes), kind }, httpMethod); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: Instead of repeating getOriginalName
, can we just do this at the very top of this method and then use this processed name everywhere below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to do this initially but then we'd potentially lose the inferred op
, no?
TL;DR: This PR Adds Sentry.updateSpanName() to reliably update span names in OpenTelemetry environments; prevents overwrites from both OpenTelemetry HTTP instrumentation and Sentry's span inference logic; preserves custom names by marking them with 'custom' source and storing in temp field which takes precedence over the actual name and gets deleted at the end
"Why not use
span.updateName()
?", the avid SDK user might ask. Here's why:In our Node SDK, most/all spans are created by OpenTelemetry instrumentation. OpenTelemetry has no concept of a transaction
source
and only stores the name of the span in theSpan
instance. Some instrumentation, most importantly,@opentelemetry/instrumentation-http
starts a span (in this case root span) and later on updates the name. In the http case, this update happens at the end of the request lifecycle. Consequently, any span name updates prior to this call, regardless of them coming from our SDK or from users, are overwritten.In addition to this limitation within Otel instrumentation, we also have our own span name, ob and source inferral logic which we apply when exporting the span to add Sentry-specific data (like a better span name, an op, a transaction source and additional data) to otel-generated spans. So even if Otel didn't call
updateName()
in its instrumentation, user-madeupdateName
calls to e.g. http.server spans were also overwritten by our logic.So we need some way of 1. bypassing our span inferral logic and 2. ensuring a user-update span name has precedence over otel's span name updates.
1 we can achieve with setting the source attribute to
custom
and ensuring we don't change the span name if we encounter acustom
source.2 we can only achieve if we write the user-updated span name to temporary field on the span and apply it during our span name processing logic.
This is exactly what
updateSpanName
does:span.updateName()
SEMANTIC_ATTRIBUTES_SENTRY_SOURCE: 'custom
to the spanFurther changes in this PR:
parseSpanDescription
custom
inparseSpanDescription
span.updateName
has no effect on http.server spans, even if you set sourcecustom
This PR supersedes #14280 where I thought we could monkey-patch
span.updateName
directly to make updating the span name AND the source more consistent without a new top-level function. Turns out, we cannot do this because then, Otel instrumentation would also call the monkey-patched version ofupdateName
, meaning we can no longer distinguish between user-made and otel-made span name updates.