Skip to content

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

Closed
wants to merge 6 commits into from

Conversation

cretz
Copy link
Member

@cretz cretz commented Jul 5, 2022

What was changed

  • Make sure all interceptors can properly mutate headers
  • Full support for proper OpenTelemetry span creation and serialization

Checklist

  1. Closes Interceptor header and OpenTelemetry support #60

@cretz cretz requested a review from a team July 5, 2022 20:02
# Conflicts:
#	poetry.lock
#	pyproject.toml
@cretz cretz marked this pull request as ready for review July 5, 2022 21:09
@@ -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]]
Copy link
Member

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
Copy link
Member

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

Copy link
Member Author

@cretz cretz Jul 8, 2022

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):
Copy link
Member

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?

Copy link
Member Author

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?

@cretz
Copy link
Member Author

cretz commented Jul 11, 2022

After discussion, this is not replay safe. I am closing this and will re-open with whatever alternative approach decided.

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.

Interceptor header and OpenTelemetry support
2 participants