Update integrations with new continue_trace callback usage #3486
Conversation
* aiohttp * asgi * aws_lambda * celery * gcp * huey * rq * sanic - this uses manual enter/exit on the contextmanager so we'll have to test properly * tornado * ray
❌ 3774 Tests Failed:
View the top 3 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
| "[ASGI] Created transaction (continuing trace): %s", | ||
| transaction, | ||
| ) | ||
| else: |
There was a problem hiding this comment.
We're removing this else logic for non-http and non-websocket scope types, was this intentional? I'm not sure why it was there in the first place (maybe future proofing for new asgi scope types?), but it seems deliberate
There was a problem hiding this comment.
Ok this is why: 44c43a8 Apparently fetching headers might be problematic for other scope types. So we should probably keep this
There was a problem hiding this comment.
thanks for digging, made the accessor safe to avoid the if
02efbe6
There was a problem hiding this comment.
I am still very much unconvinced that the breaking change to continue_trace which necessitates these changes is a good idea.
I would like to revisit the continue_trace changes (#3460) and reach a consensus in the team that making this breaking change is truly necessary before we go updating usages everywhere.
There was a problem hiding this comment.
During an offline discussion the team agreed that #3460 makes sense even independent of the POTel project, since the API is clearer and easier to understand.
In the new API, continue_trace has the responsibility of setting propagation context from the remote span (linking the trace together), and start_span has the responsibility of actually creating and starting the span. In the old API, these responsibilities were confused with each other. We find that breaking the old API in order to get the more understandable new API is worth it.
Given this clarification, I am dropping my objections.
| with sentry_sdk.continue_trace(headers): | ||
| with sentry_sdk.start_transaction( |
There was a problem hiding this comment.
I wonder whether it might make sense to introduce a new convenience function that would continue the trace and start a span in one call, so that these two with context managers could be combined into a single with continue_trace_start_span (perhaps with better naming) statement.
closes #3484