Repository navigation
[Diagnostics] Give every batch item its own request span - #10393
Conversation
A GraphQL over HTTP batch shares a single transport span across items that execute concurrently, so folding each item's operation details into that shared span produced interleaved, meaningless tags. Mark the transport span as a batch when a batch or operation batch request starts, and have the execution listeners fall through to a dedicated request span per item instead of reusing it.
Patch coverage69.2% of changed lines covered (18/26)
Uncovered changed lines (JSON){
"sha": "9604bb089f4ccc75ba72391aa98732b24245dfbf",
"files": [
{ "path": "src/HotChocolate/Diagnostics/src/Diagnostics/Listeners/ActivityServerDiagnosticListener.cs", "ranges": [[47, 48], [65, 66]] },
{ "path": "src/HotChocolate/Fusion/src/Fusion.Diagnostics/Listeners/FusionActivityServerDiagnosticListener.cs", "ranges": [[48, 49], [66, 67]] }
]
}Project coverage: 57.9% (288533/497963 lines) |
There was a problem hiding this comment.
🟡 Changes recommended
Variable-batching handling and operation-batch regression coverage remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates standard and Fusion diagnostics so concurrent HTTP batch items receive independent request spans.
Changes:
- Marks transport spans as batch-aware.
- Creates dedicated spans for batch items.
- Adds regression tests and runtime snapshots.
File summaries
| File | Summary |
|---|---|
src/HotChocolate/Fusion/test/Fusion.Diagnostics.Tests/FusionActivityServerDiagnosticListenerTests.cs |
Adds Fusion batch tracing coverage. |
src/HotChocolate/Fusion/test/Fusion.Diagnostics.Tests/__snapshots__/FusionActivityServerDiagnosticListenerTests.Http_Post_BatchRequest_Should_Give_Every_Item_Its_Own_Request_Span_Default.snap |
Captures default Fusion activity output. |
src/HotChocolate/Fusion/test/Fusion.Diagnostics.Tests/__snapshots__/FusionActivityServerDiagnosticListenerTests.Http_Post_BatchRequest_Should_Give_Every_Item_Its_Own_Request_Span_Default_NET11_0.snap |
Captures .NET 11 Fusion activity output. |
src/HotChocolate/Fusion/src/Fusion.Diagnostics/Listeners/FusionActivityServerDiagnosticListener.cs |
Marks Fusion HTTP batches. |
src/HotChocolate/Fusion/src/Fusion.Diagnostics/Listeners/FusionActivityExecutionDiagnosticEventListener.cs |
Creates per-item Fusion request spans. |
src/HotChocolate/Diagnostics/test/Diagnostics.Tests/ActivityServerDiagnosticListenerTests.cs |
Adds standard batch tracing coverage. |
src/HotChocolate/Diagnostics/test/Diagnostics.Tests/__snapshots__/ActivityServerDiagnosticListenerTests.Http_Post_BatchRequest_Should_Give_Every_Item_Its_Own_Request_Span_Default.snap |
Captures default activity output. |
src/HotChocolate/Diagnostics/test/Diagnostics.Tests/__snapshots__/ActivityServerDiagnosticListenerTests.Http_Post_BatchRequest_Should_Give_Every_Item_Its_Own_Request_Span_Default_NET11_0.snap |
Captures .NET 11 activity output. |
src/HotChocolate/Diagnostics/src/Diagnostics/Listeners/ActivityServerDiagnosticListener.cs |
Marks standard HTTP batches. |
src/HotChocolate/Diagnostics/src/Diagnostics/Listeners/ActivityExecutionDiagnosticListener.cs |
Creates per-item request spans. |
src/HotChocolate/Diagnostics/src/Diagnostics.Core/Spans/Http/ExecuteHttpRequestSpan.cs |
Tracks batch state and request details. |
Review details
Suppressed comments (4)
src/HotChocolate/Diagnostics/src/Diagnostics/Listeners/ActivityServerDiagnosticListener.cs:51
- Variable batching does not reach
StartBatchRequest:ExecutorSession.ExecuteSingleAsynccallsStartSingleRequest, then an array-valuedvariablespayload becomes aVariableBatchRequestthatDefaultRequestExecutorruns concurrently. Because this path leavesIsBatchfalse, the default execution listener still reuses the sharedExecuteHttpRequestSpan, so concurrent items can write their operation tags onto the same activity. Detect variable batching inStartSingleRequestand mark the transport span as a batch as well, with a regression test.
span.MarkAsBatch();
src/HotChocolate/Diagnostics/test/Diagnostics.Tests/ActivityServerDiagnosticListenerTests.cs:158
- This new test covers only JSON request batching. The changed
StartOperationBatchRequestpath is also responsible for marking operation batches so each operation gets its own request span, but there is no diagnostic test for a?batch=...request; a regression in that branch would pass unnoticed. Please add an operation-batch snapshot test here as well.
public async Task Http_Post_BatchRequest_Should_Give_Every_Item_Its_Own_Request_Span_Default()
src/HotChocolate/Fusion/src/Fusion.Diagnostics/Listeners/FusionActivityServerDiagnosticListener.cs:52
- Variable batching does not reach
StartBatchRequest:ExecutorSession.ExecuteSingleAsynccallsStartSingleRequest, then an array-valuedvariablespayload becomes aVariableBatchRequestthatDefaultRequestExecutorruns concurrently. Because this path leavesIsBatchfalse, the default execution listener still reuses the sharedExecuteHttpRequestSpan, so concurrent items can write their operation tags onto the same activity. Detect variable batching inStartSingleRequestand mark the transport span as a batch as well, with a regression test.
span.MarkAsBatch();
src/HotChocolate/Fusion/test/Fusion.Diagnostics.Tests/FusionActivityServerDiagnosticListenerTests.cs:93
- This new test covers only JSON request batching. The changed
StartOperationBatchRequestpath is also responsible for marking operation batches so each operation gets its own request span, but there is no Fusion diagnostics test for a?batch=...request; a regression in that branch would pass unnoticed. Please add an operation-batch snapshot test here as well.
public async Task Http_Post_BatchRequest_Should_Give_Every_Item_Its_Own_Request_Span_Default()
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The existing coverage only exercised the JSON request batch path. An operation batch (?batchOperations=[A,B]) reaches StartOperationBatchRequest instead, a separate MarkAsBatch call site, so it needs its own default-scope snapshot. Add one for both the HotChocolate and Fusion listeners.
The code states the condition directly, so the surrounding prose added nothing.
No description provided.