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

[#1962] Wrap returned promises in OpenTracingOutboundCallsInterceptor #2366

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

ghaskins
Copy link
Contributor

@ghaskins ghaskins commented Jan 13, 2025

This change allows the OpenTracing interceptor to transfer any active trace/span from
the point of invocation to the point of promise callback execution, thus preserving
the span within the callback.

Without this, spans become disjoint across Promise.thenCompose, due
to the nature of the deferring the execution of the .thenCompose to a callback thread.

Fixes #1962

Checklist

  1. Closes Tracing context does not propagate into .thenCompose #1962

  2. How was this tested:
    A unit test was developed first to confirm the issue. The solution was iterated over until the unit test passed satisfactorily. The unit-test is included in the MR for inclusion with the code-base

  3. Any docs updates needed?
    No.

@ghaskins ghaskins requested a review from a team as a code owner January 13, 2025 18:52
@cretz
Copy link
Member

cretz commented Jan 13, 2025

Hrmm, this is a very specific interceptor-oriented solution to a general problem of contextual state across promises. I wonder if a WorkflowAsyncLocal is best which behaves like thread local, but also propagates across promises. This is the common solution to this problem in other languages, make a way to propagate state across async (Go is context, Python is contextvars, .NET is AsyncLocal, etc).

@ghaskins
Copy link
Contributor Author

@cretz, if I understand what you are saying, I think we'd still need to use interception at some level unless OTEL can be made to utilize the WorkflowAsyncLocal instead of ThreadLocal because something would need to initialize the state in the callback context, which currently runs outside of the RootThread complex. I'm not saying OTEL can't use WorkflowAsyncLocal; I just don't know

@ghaskins ghaskins force-pushed the issue-1962 branch 3 times, most recently from e714fc3 to 5bd97da Compare January 14, 2025 15:05
@ghaskins ghaskins changed the title [#1962] Add newCallbackExecutor to interceptors and override for OpenTracing [#1962] Wrap returned promises in OpenTracingOutboundCallsInterceptor Jan 15, 2025
This change allows the OpenTracing interceptor to transfer any active trace/span from
the point of invocation to the point of promise callback execution, thus preserving
the span within the callback.

Without this, spans become disjoint across Promise.thenCompose, due
to the nature of the deferring the execution of the .thenCompose to a callback thread.

Fixes temporalio#1962

Signed-off-by: Greg Haskins <[email protected]>
@Quinn-With-Two-Ns
Copy link
Contributor

@ghaskins Looks like part of CI is failing because your branch is missing the version tags, can you make sure you fetched tags when fetched? I will try to prioritize adding a fallback in our CI but for now the quickest would be just to push make sure tags are present.

@@ -51,13 +126,16 @@ public OpenTracingWorkflowOutboundCallsInterceptor(
@Override
public <R> ActivityOutput<R> executeActivity(ActivityInput<R> input) {
if (!WorkflowUnsafe.isReplaying()) {
Span capturedSpan = tracer.scopeManager().activeSpan();
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a lot of futures in the interceptor that are not wrapped, I will need to go and wrap them, that is one big drawback with this approach, but given the other constraints I can't think of a better approach.

@ghaskins
Copy link
Contributor Author

@Quinn-With-Two-Ns pushed tags

@Quinn-With-Two-Ns Quinn-With-Two-Ns merged commit b593b35 into temporalio:master Jan 16, 2025
8 checks passed
@ghaskins ghaskins deleted the issue-1962 branch January 16, 2025 21:12
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.

Tracing context does not propagate into .thenCompose
4 participants