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

fix(astro): Fix astro trace propagation issues #14501

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Conversation

mydea
Copy link
Member

@mydea mydea commented Nov 27, 2024

Noticed this while working on #14481.

This PR fixes a bug in our opentelemetry code that lead to invalid empty span IDs being generated in certain cases.
Instead, we now generate valid (but random) span IDs in this case.

Additionally, I also noticed that the way we try-catched the astro server request code lead to the http.server span not being attached to servers correctly - we had a try-catch block outside of the startSpan call, where we sent caught errors to sentry. but any error caught this way would not have an active span (because by the time the catch part triggers, startSpan is over), and thus the http.server span would not be attached to the error. By moving this try-catch inside of the startSpan call, we can correctly assign the span to errors. I also tried to add some tests to this - there is still a problem in there which the tests show, which I'll look at afterwards (and/or they may get fixed by #14481)

UPDATE: Turns out part of this, the generation of an '' span ID, has other implications - otel ignores this as it is invalid, so changing this breaks the pure otel E2E apps. I'll tackle this separately, this PR only handles better try-catching of error traces now.

@mydea mydea requested review from lforst and Lms24 November 27, 2024 08:53
@mydea mydea self-assigned this Nov 27, 2024
Copy link

codecov bot commented Nov 27, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
655 1 654 31
View the top 1 failed tests by shortest run time
errors.server.test.ts server-side errorscaptures SSR error
Stack Traces | 0.441s run time
errors.server.test.ts:5:3 captures SSR error

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

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