-
Notifications
You must be signed in to change notification settings - Fork 104
Interceptor header and OpenTelemetry support #63
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
Conversation
# Conflicts: # poetry.lock # pyproject.toml
@@ -1298,7 +1288,7 @@ class StartWorkflowInput: | |||
cron_schedule: str | |||
memo: Optional[Mapping[str, Any]] | |||
search_attributes: Optional[temporalio.common.SearchAttributes] | |||
header: Optional[Mapping[str, Any]] | |||
headers: Optional[Mapping[str, temporalio.api.common.v1.Payload]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for changing this to headers
# on that interceptor since they can't accept custom constructor values. | ||
self.header_key: str = "_tracer-data" | ||
self.text_map_propagator: opentelemetry.propagators.textmap.TextMapPropagator = ( | ||
default_text_map_propagator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In TS we use the global propagator so users can decide whether they want to propagate baggage or use the jaeger propagator (default when using opentracing).
https://docs.temporal.io/typescript/logging#context-propagation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't realize there was a global, but I see it now at https://opentelemetry-python.readthedocs.io/en/latest/api/propagate.html#opentelemetry.propagate.get_global_textmap. I will switch to that (but still keep it a class member so the class can be extended to customize it that way).
I will make sure to test, when sandboxing, which approaches to set the global one work (probably just some top-level Python code in the workflow file though that is ugly, otherwise I may bring over the env var if possible or just keep propagator use on non-sandbox side)
__temporal_opentelemetry_end_span: Callable[[bytes], None] | ||
|
||
|
||
class TracingWorkflowInboundInterceptor(temporalio.worker.WorkflowInboundInterceptor): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work during replay? What happens if a span is created in one worker and ends in another? Won't we end up with a bunch of orphaned spans since the span ID is not created deterministically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how span ID matters. Most of these calls do not create spans on replay. Are you speaking specifically about the even_on_replay=True
for execute_workflow
? I basically just copied the code verbatim from the Go interceptor code.
My tests aren't showing any orphaned spans during replay (done via query after completion), but maybe it is not properly testing after eviction? Can you describe the exact test to write to confirm behavior here?
After discussion, this is not replay safe. I am closing this and will re-open with whatever alternative approach decided. |
What was changed
Checklist