fix(core): Set sentry.source attribute to custom when calling span.updateName on SentrySpan
#14251
Conversation
This file contains 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
size-limit report 📦
|
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Lms24
force-pushed
the
lms/fix-core-source-custom-span-updatename
branch
from
14aa37c
to
ddbf336
Compare
Lms24
changed the title
sentry.source attribute to custom when calling span.updateNamesentry.source attribute to custom when calling span.updateName on SentrySpan
Lms24
force-pushed
the
lms/fix-core-source-custom-span-updatename
branch
from
08a8882
to
0f12ced
Compare
Lms24
commented
Nov 13, 2024
| expect(transaction.transaction).toBe('parent_span'); | ||
| expect(transaction.spans).toBeDefined(); | ||
| }); | ||
| sentryTest( |
There was a problem hiding this comment.
adjusted this test to check that we by default start a span with source "custom"
Lms24
commented
Nov 13, 2024
| import { sentryTest } from '../../../../utils/fixtures'; | ||
| import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers'; | ||
|
|
||
| sentryTest('should create a pageload transaction', async ({ getLocalTestPath, page }) => { | ||
| sentryTest('creates a pageload transaction with url as source', async ({ getLocalTestPath, page }) => { |
There was a problem hiding this comment.
no behaviour change here, just to explicitly check that our default browserTracingIntegration starts spans with source "url"
Lms24
commented
Nov 13, 2024
Lms24
force-pushed
the
lms/fix-core-source-custom-span-updatename
branch
from
bbbca89
to
ab779bb
Compare
andreiborza
reviewed
Nov 13, 2024
| @@ -193,6 +193,7 @@ export class SentrySpan implements Span { | |||
| */ | |||
| public updateName(name: string): this { | |||
| this._name = name; | |||
| this.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'custom'); | |||
There was a problem hiding this comment.
l: Wondering if we would want to mention this in the jsdoc?
There was a problem hiding this comment.
Good point, let me add a note
There was a problem hiding this comment.
On second thought, I'll add this but only in #14280 (which is the PR to fix this and a couple of depending things in Node). Since we always pass the Span interface, I need to upadet the JSDoc of the interface which we should only do once the behaviour is adjusted everywhere.
andreiborza
approved these changes
Nov 13, 2024
Open
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR fixes a "regression" introduced in v8 where we no longer updated the source of the span when calling
span.updateName. We had this behaviour in our v7Transactionclass (IIRC viatransaction.setName(name, source)but most likely forgot to port it to theSpanclass as theevent.transaction_info.sourcefield is only relevant for transactions (i.e. root spans in today's SDK lingo).The PR also adds and improves some unit and browser integration tests so that we check against the default source as well as that updating the name updates the source.
Important: While this is a fix in the core package it has no effect on spans created in the Node SDK, since these are created from OpenTelemetry. I'll open a second PR for Node, as the changes there are more involved. This fix only addresses the behaviour of our
SentrySpanclass, which is used in browser SDKs (and none-Otel SDKs if any others use tracing)The issue addressed in this PR shouldn't be a major issue because it does not concern manually created root spans. Probably the only relevant scenario where this behaviour had user impact is something like:
url(likely viabrowserTracingIntegrationfrom@sentry/browser)transaction_info.source: "url""url"source is low qualityWhich again is unlikely an issue in framework SDKs because most of our routing instrumentations update the span name and set the source to
"route". The described example would for example happen in@sentry/browserwith the defaultbrowserTracingIntegrationenabled and users running their own enhancement for e.g. a yet unsupported router.