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.

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

test(browser): Add test for current DSC transaction name updating behavior #13953

Merged
merged 7 commits into from
Oct 14, 2024

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Oct 11, 2024

This PR adds a browser integration test that describes the current behaviour of how we update the transaction field in the DSC which is propagated via the baggage header and added to envelopes as the trace header.

Specifically, this test shows a somewhat typical scenario for browser-side routing instrumentation where we initially have a low-quality URL txn name and only after a while update the name with a parameterized route. This test however also shows that we continue to update (i.e. not freeze) the DSC for subsequent transaction name updates. This is something we might want to change in the future, so that we only once set the transaction field when we have a high-quality txn name and freeze it afterwards, as discussed with @cleptric.

For more information on the test scenario, take a look at the comment block in the test

Will open another PR with a Node integration test soon.

@Lms24 Lms24 changed the title test(browser): Add test for current DSC transaction name updating behaviour test(browser): Add test for current DSC transaction name updating behavior Oct 11, 2024
@Lms24 Lms24 requested review from mydea and cleptric October 11, 2024 09:53
@Lms24 Lms24 self-assigned this Oct 11, 2024
7. Update span name again with HQ name (source: route)
8. Make request and check that baggage has updated HQ txn name
9. Capture error and check that envelope trace header has updated HQ txn name
10. End span and check that envelope trace header has updated HQ txn name
Copy link
Member

Choose a reason for hiding this comment

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

can we add 11. to send a request after the span has ended, and make sure what is sent then? as we keep the trace alive after that (but not on the active span anymore, but the propagation context) this is also a scenario, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, good point. Let me add that

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm so it looks like at the moment, the DSC is also re-generated and no span information is added at all, meaning we not only loose transaction name but also sample_rate and sampled

Copy link
Member Author

Choose a reason for hiding this comment

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

argh this is also problematic for the sentry-trace header. In the same scenario, we omit the sampling decision:

image

(first 3 requests are happening while the span is active, the last one afterwards. Same traceId but different sampled flags)

Copy link
Member

Choose a reason for hiding this comment

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

let's reflect this in this PR I'd say and then see if we can fix/improve this in a follow up...?

Copy link
Member Author

@Lms24 Lms24 Oct 11, 2024

Choose a reason for hiding this comment

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

@mydea I think the behavior in this test is caused by the constructed scenario of us starting the trace by creating a custom span as opposed to a pageload or navigation span. I will look into what exactly is causing the transaction info to be missing while the traceId remains the same in this scenario. However, I added tests for the usual pageload transaction scenario where after the transaction ends we make requests/send additional spans. They have consistent DSC data from the initial pageload transaction.

==> we should have consistent behaviour today as long as browserTracingIntegration is used with default options for pageload/navigation.

Copy link

codecov bot commented Oct 11, 2024

❌ 6 Tests Failed:

Tests completed Failed Passed Skipped
631 6 625 33
View the top 3 failed tests by shortest run time
tracing/trace-lifetime/pageload-meta/test.ts create a new trace for a navigation after the <meta> tag pageload trace
Stack Traces | 0.18s run time
test.ts:18:11 create a new trace for a navigation after the <meta> tag pageload trace
tracing/trace-lifetime/pageload-meta/test.ts error during <meta> tag pageload has pageload traceId
Stack Traces | 0.238s run time
test.ts:136:11 error during <meta> tag pageload has pageload traceId
tracing/trace-lifetime/pageload-meta/test.ts outgoing XHR request during <meta> tag pageload has pageload traceId in headers
Stack Traces | 0.264s run time
test.ts:247:11 outgoing XHR request during <meta> tag pageload has pageload traceId in headers

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@Lms24 Lms24 force-pushed the lms/browser-dsc-transaction-name-frozen branch from 06c31d9 to 079417d Compare October 14, 2024 12:34
@Lms24 Lms24 enabled auto-merge (squash) October 14, 2024 12:39
@Lms24 Lms24 merged commit 50ab948 into develop Oct 14, 2024
142 checks passed
@Lms24 Lms24 deleted the lms/browser-dsc-transaction-name-frozen branch October 14, 2024 12:53
Lms24 added a commit that referenced this pull request Oct 15, 2024
…ior (#13961)

Add tests for DSC updating behaviour in the Node SDK,
analogously to #13953
billyvg pushed a commit that referenced this pull request Oct 17, 2024
…avior (#13953)

Add a browser integration test that describes the current
behaviour of how we update the `transaction` field in the DSC which is
propagated via the `baggage` header and added to envelopes as the
`trace` header.
billyvg pushed a commit that referenced this pull request Oct 17, 2024
…ior (#13961)

Add tests for DSC updating behaviour in the Node SDK,
analogously to #13953
billyvg pushed a commit that referenced this pull request Oct 17, 2024
…ior (#13961)

Add tests for DSC updating behaviour in the Node SDK,
analogously to #13953
billyvg pushed a commit that referenced this pull request Oct 17, 2024
…ior (#13961)

Add tests for DSC updating behaviour in the Node SDK,
analogously to #13953
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants