Repository navigation
feat(opentelemetry): add streaming span lifecycle and operation metrics - #1236
Conversation
acd33c1 to
3c2318d
Compare
kabir
left a comment
There was a problem hiding this comment.
Found two blocking telemetry gaps:
-
gRPC client streams never produce the terminal span or duration metric. The new wrapper relies on its
errorConsumerbeing called withnullon successful completion, but gRPCEventStreamObserver.onCompleted()is a no-op. Successful gRPC streams therefore omit<method>-endandgen_ai.client.operation.duration. Please signal completion through the callback (or add a completion callback to the client transport contract) and cover it with a gRPC integration test. -
gRPC server streams commonly bypass the new terminal instrumentation. The decorator forwards the underlying subscription directly. The gRPC handler cancels that subscription when it sends a final task-status event, then closes the wire stream without invoking the decorator subscriber’s
onComplete(). This skips the server<method>-endspan and streaming-duration histogram for the normal final-status path, as well as client cancellation. Please interceptSubscription.cancel()or ensure the handler signals completion through the decorated subscriber.
kabir
left a comment
There was a problem hiding this comment.
- P1 — REST streams never finish the new client telemetry. OpenTelemetryClientTransport.java:173 relies on errorConsumer.accept(null) to create the closing span and record the duration. RestTransport currently discards its SSE completion callback for both streaming methods, so a normally completed REST stream produces neither the -end span nor gen_ai.client.operation.duration. Update REST completion handlers to signal the listener/consumer with null (as JSON-RPC now does), and add a REST completion test.
- Per-event child spans and terminal closing span for streaming methods - Duration histograms for client (gen_ai.client.operation.duration) and server (gen_ai.agent.a2a.streaming.duration) - Signal REST stream completion to terminal consumer for proper span closing Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
REST streams must deliver one terminal outcome: `null` for normal completion, or the original error when parsing fails. The production fix from this PR is already on `main` through a2aproject#1236. After rebasing, this PR retains the contributor's listener tests and strengthens the malformed JSON and missing-payload cases to verify that a later `onComplete()` neither delivers a second callback nor replaces the error. The duplicate transport completion test has been removed because `main` already covers that behavior. Validation: - `mvn -B -pl client/transport/rest -am test` - Temporarily restoring the old direct parsing-error callbacks makes both strengthened tests fail with two terminal callbacks; restoring `signalTerminal` makes them pass. This fixes a2aproject#1194
REST streams must deliver one terminal outcome: `null` for normal completion, or the original error when parsing fails. The production fix from this PR is already on `main` through #1236. After rebasing, this PR retains the contributor's listener tests and strengthens the malformed JSON and missing-payload cases to verify that a later `onComplete()` neither delivers a second callback nor replaces the error. The duplicate transport completion test has been removed because `main` already covers that behavior. Validation: - `mvn -B -pl client/transport/rest -am test` - Temporarily restoring the old direct parsing-error callbacks makes both strengthened tests fail with two terminal callbacks; restoring `signalTerminal` makes them pass. This fixes #1194
The 0.3 REST transport never reported a normally finished stream back to the caller: both completion callbacks handed to postAsyncSSE were empty lambdas and RestSSEEventListener_v0_3 had no onComplete() at all, so a client could not tell a finished stream apart from one that is still running. Do what the native REST transport already does since a2aproject#1236, and what the 0.3 JSON-RPC transport has always done: turn the connection-end callback into a single terminal errorHandler(null) call. signalTerminal() keeps that to exactly one terminal outcome, so a completion that arrives after a parsing error is dropped instead of producing a second callback. The listener contract is covered directly, including error-then-completion, repeated completion and concurrent terminal signals, all of which fail when the atomic guard is removed. The errorConsumer Javadoc in ClientTransport_v0_3 now spells out the null-on-success convention, matching the native interface. Fixes a2aproject#1194
The 0.3 REST transport never reported a normally finished stream back to the caller: both completion callbacks handed to postAsyncSSE were empty lambdas and RestSSEEventListener_v0_3 had no onComplete() at all, so a client could not tell a finished stream apart from one that is still running. Do what the native REST transport already does since a2aproject#1236, and what the 0.3 JSON-RPC transport has always done: turn the connection-end callback into a single terminal errorHandler(null) call. signalTerminal() keeps that to exactly one terminal outcome, so a completion that arrives after a parsing error is dropped instead of producing a second callback. The listener contract is covered directly, including error-then-completion, repeated completion and concurrent terminal signals, all of which fail when the atomic guard is removed. The errorConsumer Javadoc in ClientTransport_v0_3 now spells out the null-on-success convention, matching the native interface. Fixes a2aproject#1194
…ort (#1279) The 0.3 REST transport never reported a normally finished stream back to the caller: both completion callbacks handed to postAsyncSSE were empty lambdas and RestSSEEventListener_v0_3 had no onComplete() at all, so a client could not tell a finished stream apart from one that is still running. Do what the native REST transport already does since #1236, and what the 0.3 JSON-RPC transport has always done: turn the connection-end callback into a single terminal errorHandler(null) call. signalTerminal() keeps that to exactly one terminal outcome, so a completion that arrives after a parsing error is dropped instead of producing a second callback. The listener contract is covered directly, including error-then-completion, repeated completion and concurrent terminal signals, all of which fail when the atomic guard is removed. The errorConsumer Javadoc in ClientTransport_v0_3 now spells out the null-on-success convention, matching the native interface. Fixes #1194
Fixes #1216 🦕