Revert "Switch to OpenTracing's ContextVarsScopeManager (#18849)"
#19007
Conversation
This reverts commit 27fc338. Conflicts: synapse/logging/context.py tests/logging/test_opentracing.py
…_available` This test is still expected to fail with our current `LogContextScopeManager` implementation but there were some flaws in how we were handling the logcontext before (like pumping the reactor in a non-sentinel logcontext)
…pe_still_available`" This reverts commit 500a187.
| own previous custom `_LogContextScope.close(...)` would clear | ||
| `LoggingContext.scope` preventing further tracing spans from having the correct | ||
| parent. | ||
| """ |
There was a problem hiding this comment.
While I think ideally this test should work and it would be nice to keep.
It's expected to fail with our current implementation of LogContextScopeManager so it makes sense to remove in the revert.
There was a problem hiding this comment.
Exploring if we can keep this test and get this functionality with LogContextScopeManager in #19008
This functionality was originally introduced in #18932 but previously worked fine because we were using the `ContextVarsScopeManager` which had no problem tracking the tracing scope across logcontext boundaries. Now that we've switched back to our own custom `LogContextScopeManager` which tracks the tracing scope in the logcontext itself, we need to get the active tracing span before we switch logcontext. (we switch to the sentinel logcontext because of the `PreserveLoggingContext()`)
| # Since we track the tracing scope in the `LoggingContext`, before we move to the | ||
| # sentinel logcontext (or a new new `LoggingContext`), grab the currently active | ||
| # tracing span (if any) so that we can create a cross-link to the background process | ||
| # trace. | ||
| original_active_tracing_span = active_span(tracer=test_only_tracer) |
There was a problem hiding this comment.
In order to maintain the functionality introduced in #18932 (cross-links between the background process trace and currently active trace), we need this change.
Previously, when we were using ContextVarsScopeManager, it tracked the tracing scope across the logcontext changes without issue. Now that we're using our own custom LogContextScopeManager again, we need to capture the active span from the logcontext before we reset to the sentinel context because of the PreserveLoggingContext() below
| ["fixture_awaitable_return_func"], | ||
| ) | ||
|
|
||
| async def test_run_as_background_process_standalone(self) -> None: |
There was a problem hiding this comment.
Added some tests to ensure we maintain the run_as_background tracing behavior regardless of the tracing scope manager we use.
Revert #18849
Go back to our custom
LogContextScopeManagerafter trying OpenTracing'sContextVarsScopeManager.Fix #19004
Why revert?
For reference, with the normal reactor,
ContextVarsScopeManagerworked just as good as our customLogContextScopeManageras far as I can tell (and even better in some cases). But since Twisted appears to not fully supportContextVar's, it doesn't work as expected in all cases. Compounding things,ContextVarsScopeManagerwas causing errors with the experimentalSYNAPSE_ASYNC_IO_REACTORoption.Since we're not getting the full benefit that we originally desired, we might as well revert and figure out alternatives for extending the logcontext lifetimes to support the use case we were trying to unlock (c.f. #18804).
See #19004 (comment) for more info.
Does this require backporting and patch releases?
No. Since
ContextVarsScopeManageroperates just as good with the normal reactor and was only causing actual errors with the experimentalSYNAPSE_ASYNC_IO_REACTORoption, I don't think this requires us to backport and make patch releases at all.Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.