Add SpanFromWorkflowContext function for OTel #2118
Conversation
|
|
| return trace.ContextWithSpan(ctx, span.(*tracerSpan).Span) | ||
| } | ||
|
|
||
| func SpanFromWorkflowContext(ctx workflow.Context) (trace.Span, bool) { |
There was a problem hiding this comment.
Please add a godoc to this function explaining the functionality
jessefoster
force-pushed
the
otel/span-context
branch
from
3f9e3d5 to
06ebaba
Compare
|
|
||
| // 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) { |
There was a problem hiding this comment.
Why return a bool that is always true?
There was a problem hiding this comment.
Yeah this look like a bug
|
|
||
| // 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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| // 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) { |
There was a problem hiding this comment.
I wonder if SpanContextFromWorkflowContext is needed/better
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // Fallback to OpenTelemetry span extraction behavior | ||
| return trace.SpanFromContext(nil), true |
There was a problem hiding this comment.
This should return false
There was a problem hiding this comment.
Yeah sorry about that, copy/paste mistake as I had to recreate file and run tests on another box. Not allowed to push directly from corporate laptop.
jessefoster
force-pushed
the
otel/span-context
branch
from
06ebaba to
8083b21
Compare
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
LGTM, but will want @Quinn-With-Two-Ns to approve
Quinn-With-Two-Ns
force-pushed
the
otel/span-context
branch
from
8083b21 to
71e7f16
Compare
|
Thank you for yals help with the PR. |
Quinn-With-Two-Ns
force-pushed
the
otel/span-context
branch
from
71e7f16 to
94bcc40
Compare
Quinn-With-Two-Ns
force-pushed
the
otel/span-context
branch
from
94bcc40 to
b734247
Compare
4 participants
What was changed
Added a function which can extract Open Telemetry Span from Workflow Context.
Why?
I saw Datadog had this capability already, see #1711, and my project needs access to Open Telemetry spans.
Checklist
How was this tested:
I've confirmed the feature works within my codebase and added unit tests as well. I've run all automated testing as prescribed here
Any doc updates needed?
None that I'm aware of