-
Notifications
You must be signed in to change notification settings - Fork 271
Add SpanFromWorkflowContext function for OTel #2118
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ import ( | |
| "go.temporal.io/sdk/interceptor" | ||
| "go.temporal.io/sdk/log" | ||
| "go.temporal.io/sdk/temporal" | ||
| "go.temporal.io/sdk/workflow" | ||
| ) | ||
|
|
||
| // DefaultTextMapPropagator is the default OpenTelemetry TextMapPropagator used | ||
|
|
@@ -174,6 +175,21 @@ func (t *tracer) ContextWithSpan(ctx context.Context, span interceptor.TracerSpa | |
| return trace.ContextWithSpan(ctx, span.(*tracerSpan).Span) | ||
| } | ||
|
|
||
| // SpanFromWorkflowContext extracts an OpenTelemetry span from the given | ||
| // workflow context. If no span is found, a no-op span is returned. | ||
| func SpanFromWorkflowContext(ctx workflow.Context) (trace.Span, bool) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why return a bool that is always true?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this look like a bug
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May need to be clarified in the godoc that this is unsafe/non-deterministic because it can technically return different values when replaying vs not
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So you'd like a comment warning people they should only use the Span data for observability needs? That's generally the assumption with observability tools, but I can make it explicit.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. That assumption is reasonable in code where non-deterministic elements are fine if they chose to use span data for other things, but for Temporal, it's usually better to clearly mark any non-deterministic workflow functions we offer as unsafe if they may be.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We haven't had any need to use SpanContext, but it is another option. The Datadog interceptor only provides Span as well so I followed it's example since it's already in the codebase.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is probably good, as I can't think of a situation where you could get a span context but not a span. I will say that query and update handlers will get a new span on non-replay, or the "run workflow" span on replay since they don't create new spans on replay but outer "run workflow" does IIUC. |
||
| val := ctx.Value(spanContextKey{}) | ||
|
|
||
| if val != nil { | ||
| if span, ok := val.(*tracerSpan); ok { | ||
| return span.Span, true | ||
| } | ||
| } | ||
|
|
||
| // Fallback to OpenTelemetry span extraction behavior | ||
| return trace.SpanFromContext(nil), false | ||
| } | ||
|
|
||
| func (t *tracer) StartSpan(opts *interceptor.TracerStartSpanOptions) (interceptor.TracerSpan, error) { | ||
| // Create context with parent | ||
| var parent trace.SpanContext | ||
|
|
||
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.
Please add a godoc to this function explaining the functionality
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.
Sure, godoc added