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

Insert OpenTelemetry phase before Setup #13239

Merged
merged 5 commits into from
Feb 11, 2025
Merged

Conversation

rolaca11
Copy link
Contributor

@rolaca11 rolaca11 commented Feb 6, 2025

Fixes #13237

The root cause of the issue is that the tracing context is applied too late in the pipeline (Monitoring phase). I moved it before the Setup phase so that the tracing setup becomes the first thing that happens during processing.

The Monitoring phase is still important because it allows us to check if the call finished without errors. If this got moved to the Setup phase along with the tracing setup, it'd mean that exception handling in CallFailed hooks would mask the error, and it wouldn't be reported.

@rolaca11 rolaca11 requested a review from a team as a code owner February 6, 2025 18:29
Copy link

linux-foundation-easycla bot commented Feb 6, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@rolaca11
Copy link
Contributor Author

rolaca11 commented Feb 6, 2025

I noticed an issue, where the Span status isn't set in case of an error

@rolaca11
Copy link
Contributor Author

rolaca11 commented Feb 6, 2025

Fixed

@laurit
Copy link
Contributor

laurit commented Feb 7, 2025

@rolaca11 would it be possible to add a regression test for this?
@e5l could you check whether this makes sense?

@e5l
Copy link

e5l commented Feb 7, 2025

LGTM

This commit installs a route-scoped plugin that may mask exceptions from OpenTelemetry, if the latter is configured at the wrong phase
@rolaca11
Copy link
Contributor Author

rolaca11 commented Feb 7, 2025

I expanded one of the current test cases so that it'd fail on master, but pass with this PR. I don't like that I needed to install the plugin where I did, but I didn't find a better place for it. If you have any suggestions, pls fire away

@trask trask merged commit 9903a0e into open-telemetry:main Feb 11, 2025
60 checks passed
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.

Ktor 3: CallLogging and StatusPages don't have Trace IDs
4 participants