Add entity tracing support - #296
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances distributed tracing for Durable Task entities by propagating incoming entity operation trace context to any outgoing signals/orchestration-start actions, and by extending TracingHelper with entity-specific OpenTelemetry span helpers plus accompanying unit tests.
Changes:
- Propagate
OperationRequest.trace_contextthrough entity-producedSendSignalActionandStartNewOrchestrationAction. - Add entity span utilities to
TracingHelper(span naming, processing spans, client/producer spans, schema-aligned attributes). - Add/extend unit tests validating entity trace propagation and entity span emission behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| client/src/main/java/com/microsoft/durabletask/TaskEntityExecutor.java | Captures per-operation trace context and attaches it to entity-produced signal/orchestration-start actions. |
| client/src/main/java/com/microsoft/durabletask/TracingHelper.java | Adds entity-specific tracing constants and helper methods for emitting entity-related spans. |
| client/src/test/java/com/microsoft/durabletask/TaskEntityExecutorTest.java | Adds tests asserting trace context propagation (and omission when absent) for entity-produced actions. |
| client/src/test/java/com/microsoft/durabletask/TracingHelperTest.java | Adds tests covering entity span naming and emission semantics (kinds, attributes, timestamps, error handling). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
client/src/main/java/com/microsoft/durabletask/TracingHelper.java:514
- These entity tracing helpers currently have no production callers (repository search finds only these definitions and
TracingHelperTest).DurableTaskGrpcWorkerstill invokesTaskEntityExecutor.executedirectly, while the orchestration and entity-client paths never call the new client/producer helpers, so enabling OpenTelemetry emits no entity spans at all. Please wire the processing, call, signal, and entity-start spans into those execution paths (and propagate the created span contexts), rather than leaving the implementation test-only.
static Span startEntityProcessingSpan(
String entityName,
String operation,
boolean signal,
String entityInstanceId,
@Nullable TraceContext parentContext) {
wangbill (YunchuWang)
left a comment
There was a problem hiding this comment.
Second-pass review focused on end-to-end entity tracing behavior and parity with the .NET implementation. I found two must-address completeness gaps, one timestamp issue that should be addressed, and one optional simplification. The context propagation logic itself otherwise looks safe and consistent with the .NET worker shim.
|
|
||
| // Parent context for any signals/orchestrations this operation produces, so the host can link them. | ||
| context.setCurrentOperationTraceContext( | ||
| opRequest.hasTraceContext() ? opRequest.getTraceContext() : null); |
There was a problem hiding this comment.
Must address or explicitly scope — the native orchestration-to-entity path cannot currently provide this context. In the ENTITYREQUESTV2 path, TaskOrchestrationExecutor creates EntityOperationCalledEvent / EntityOperationSignaledEvent, but neither protobuf message has a trace-context field. DurableTaskGrpcWorker can consequently copy only request ID, operation, and input into OperationRequest, making hasTraceContext() false for this path. The flattened request also loses whether the operation was a call or signal, so a processing span cannot select SERVER versus CONSUMER correctly.
Please define which component owns entity span creation and carry the required context/invocation kind through the protocol (with backend coordination), or explicitly limit this PR to propagation for backend-supplied V1 requests. An end-to-end test should cover both an orchestration call and signal.
There was a problem hiding this comment.
Rechecked at current head 27f6ea2a: this remains unresolved. The V2 conversion still builds both OperationRequest variants without a trace context, and TaskEntityExecutor now hardcodes signal=false when starting the processing span. Therefore the default native path still cannot link the incoming trace and can never produce a CONSUMER processing span for signals. The new test also explicitly expects zero CLIENT spans as "deferred pending protocol support". Please either coordinate the protobuf/backend fields needed for context + invocation kind, or explicitly re-scope and track this limitation before merging.
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
wangbill (YunchuWang)
left a comment
There was a problem hiding this comment.
Follow-up review of the remediation commits. The requestTime and Span.end(Instant) comments are correctly addressed, and processing/producer wiring has been added. The native V2 protocol limitation remains open (updated in the existing thread). I found three correctness gaps in the new wiring plus the still-deferred call/external-client coverage below. This is a comment-only review; it does not change the PR review state.
Also, GrpcDurableEntityClient.signalEntity() remains unchanged and still captures only the ambient context without emitting the new PRODUCER span, so external-client signals are not covered by the advertised entity tracing support. Please wire that path or explicitly document/re-scope it as deferred.
| this.currentOperationTraceContext, | ||
| null, | ||
| signalScheduledTime); | ||
| if (producerSpan != null) { |
There was a problem hiding this comment.
Must address — do not export spans for actions that are later rolled back. This PRODUCER span is ended immediately inside signalEntity() (and similarly in startNewOrchestration()), before the surrounding entity operation commits. If the entity signals and then throws, context.rollback() correctly removes the pending action, but this already-finished span remains exported and appears to show work that was never sent.
Please defer emission until the action is known to be committed, and add a regression test where an entity queues an action and then throws: the result should contain zero actions and export zero corresponding PRODUCER spans. Keep the existing success assertion to prove committed actions still emit exactly once.
|
|
||
| // Entity processing span parented on the incoming operation trace context; | ||
| // suppressed when the host emits its own (emitTraceSpans is false). | ||
| Span processingSpan = this.emitTraceSpans |
There was a problem hiding this comment.
Should address — make the processing span current while user entity code runs. The span is created and its context is copied manually to Durable actions, but entity.run(operation) is never executed under processingSpan.makeCurrent(). As a result, spans created by user code or automatic HTTP/database instrumentation inside the entity will not be children of this entity processing span and will appear disconnected.
Please mirror the existing activity execution pattern: open a Scope from the processing span around entity.run(...), close it on every success/failure path, and then end the span. Add a test that creates a normal nested span inside an entity operation and verifies its parent is the processing span.
| .build()); | ||
| } | ||
|
|
||
| if (this.currentOperationTraceContext != null) { |
There was a problem hiding this comment.
Must address — propagate the PRODUCER span context, not its parent processing context. The action is stamped with currentOperationTraceContext before the PRODUCER span is created, and the new PRODUCER span's context is discarded. A downstream CONSUMER/orchestration span will therefore be parented directly to the processing span, making it a sibling of the PRODUCER instead of its child. The .NET dispatcher starts the producer activity and puts that activity's context on the outgoing message.
After creating the committed PRODUCER span, set the action's parentTraceContext from that span's context (for both signal and start-orchestration actions). Add an assertion that the action's propagated span ID equals the exported PRODUCER span ID, not the enclosing SERVER span ID.
| long producers = spans.stream().filter(s -> s.getKind() == SpanKind.PRODUCER).count(); | ||
| long clients = spans.stream().filter(s -> s.getKind() == SpanKind.CLIENT).count(); | ||
| assertEquals(1L, producers, "signalEntity should emit exactly one PRODUCER span"); | ||
| assertEquals(0L, clients, "callEntity CLIENT span is deferred pending protocol support"); |
There was a problem hiding this comment.
Must address or explicitly re-scope — this test currently codifies incomplete call tracing. emitEntityCallClientSpan() still has no production caller, so callEntity() never emits the promised CLIENT span. The CLIENT span should cover request-to-response (or timeout/cancellation), use the synthetic span ID that parents the SERVER processing span, and be suppressed during replay.
Please implement and assert the CLIENT span lifecycle rather than permanently expecting zero here. If protocol/backend work makes that impossible in this PR, remove/re-scope the unused helper and state the limitation prominently in the PR description with a tracked follow-up.
This pull request introduces enhancements to entity operation trace context propagation and adds comprehensive OpenTelemetry tracing support for Durable Task entities. The main changes ensure that when an entity operation includes a trace context, it is correctly propagated to any signals or orchestrations the entity produces. Additionally, the tracing helper now provides a full set of utilities for emitting and testing entity-related spans, aligning with the .NET SDK schema.
Entity Operation Trace Context Propagation:
TraceContextfrom an incoming entity operation is captured and propagated to outgoing signals and orchestrations, allowing for proper trace linkage in distributed tracing scenarios. (TaskEntityExecutor.java)TaskEntityExecutorTest.java)OpenTelemetry Tracing Support for Entities:
TracingHelper, following the .NET SDK schema. This includes methods for creating span names, starting/ending processing spans, and emitting client/producer spans for entity operations, signals, and orchestration starts.Pull request checklist
CHANGELOG.md