OpenTelemetry tracing support #234
Conversation
| # ... assumes my_otel_tracer_provider is a tracer provider created by the user | ||
| my_tracer = my_otel_tracer_provider.tracer('my-otel-tracer') |
There was a problem hiding this comment.
Maybe an example of one that someone would use would be nice
There was a problem hiding this comment.
Yeah, I should just change this to
| # ... assumes my_otel_tracer_provider is a tracer provider created by the user | |
| my_tracer = my_otel_tracer_provider.tracer('my-otel-tracer') | |
| my_tracer = OpenTelemetry.tracer_provider.tracer('my-otel-tracer') |
Per https://opentelemetry.io/docs/languages/ruby/instrumentation/#acquiring-a-tracer. I guess most people configure the global instead of creating a tracing provider.
| # We are intentionally not pinning OTel versions here so that CI tests the latest. This also means that the OTel | ||
| # contrib library also does not require specific versions, we are relying on the compatibility rigor of OTel. |
There was a problem hiding this comment.
👍 The problem is that Ruby doesn't have a concept of "optional dependency constraints" so I can't put a constraint on users' OTel library, I can only fail to require it or add advanced validation to check the OTel version which I don't want to do. So this is the best way to at least make sure that we always work with the latest. If there was enough concern about breaking support for older OTel API versions, we could make a separate GH step to only test with a specific oldest OTel API version.
There was a problem hiding this comment.
I think we do want to make this an option (and I also opened temporalio/sdk-python#794). I will update this PR.
|
|
||
| # Do nothing if there is no span on the context. We do not want orphan spans coming from workflows, so we | ||
| # require a parent (i.e. a current). | ||
| # TODO(cretz): This matches Python behavior but not .NET behavior (which will create no matter what), is that | ||
| # ok? |
There was a problem hiding this comment.
Why would there be no span? Something like this is running inside a workflow that was started from somewhere not using OTel?
There was a problem hiding this comment.
Something like this is running inside a workflow that was started from somewhere not using OTel?
Exactly. So if you start a workflow from CLI, then there is no stable span on the headers of start-workflow which means no stable parent. It's hard to decide which approach is best. With the Python approach we don't have orphaned spans, but we just don't have any spans if they didn't use an OTel friendly client. But with the .NET approach we have orphaned spans that don't have clear siblings or overarching workflow parent. I have seen slight complaints about both, but nothing significant.
Open to suggestions here. I could make it an option, though I just chose the "no OTel if didn't start with it, meaning no orphans" currently as I thought that was the better approach.
| # @param exception [Exception, nil] Exception to record on the span. | ||
| # @param even_during_replay [Boolean] Set to true to record this span even during replay. Most users should | ||
| # never set this. | ||
| # @return [OpenTelemetry::Trace::Span, nil] Span if one was created. WARNING: It is UNSAFE to use this value. |
There was a problem hiding this comment.
Why return anything at all then?
There was a problem hiding this comment.
Because technically there is value in using it if you do it safely. For instance we use the result in the method right above this one and I don't want to necessarily prevent others from writing similar things if they know what they are doing. For example, some people may want to log the span ID only if the span was created (i.e. non-replay) which is ok.
What was changed
Temporalio::Contrib::OpenTelemetrymoduleTracingInterceptorclass for tracingWorkflowmodule with helpers for replay-safe span creationTemporalio::Workflow.storageakin toFiber#storagewhere arbitrary data can be stored on a workflowChecklist