Repository navigation
Conversation
Opt-in client and server events cover requests, streams, transport selection, and push notifications, with trace correlation across async callbacks. Payload capture is separately opt-in with configurable masking and size limits. This fixes a2aproject#1221 Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
|
@brunobat May you take a look |
| public interface A2AObservabilityNames { | ||
|
|
||
| // Protocol log events | ||
| String A2A_EVENT_AUTHORIZATION = "a2a.authorization"; |
| if (!budget.consume(inputBytes)) { | ||
| return ""; | ||
| } | ||
| String sanitized = |
There was a problem hiding this comment.
Sanitized runs after truncation which might prevent proper sanitation of chopped data that should have been picked up by the Sanitizer... It's probably an academic possibility with the a custom ProtocolPayloadSanitizer.
| TaskPushNotificationConfig result = delegate.onCreateTaskPushNotificationConfig(params, context); | ||
| logProtocolMetadata(A2A_EVENT_NOTIFICATION_SUBSCRIPTION, | ||
| A2AMethods.SET_TASK_PUSH_NOTIFICATION_CONFIG_METHOD, A2A_LOG_DIRECTION_OUTBOUND, | ||
| A2A_KIND_CREATED, params); |
There was a problem hiding this comment.
Why are not logging anything about the result?
| Span closingSpan = config.tracer().spanBuilder(config.spanName() + STREAMING_END_SUFFIX) | ||
| .setSpanKind(SpanKind.SERVER) | ||
| .addLink(config.parentSpanContext()) | ||
| .setParent(config.context()) |
There was a problem hiding this comment.
Is this on purpose?
A long SSE stream puts every event, possibly hours later, into one huge trace under a parent that closed long before.
| .setAttribute(GENAI_STREAMING_EVENT, item.kind()); | ||
| StreamingEventAttributes.apply(builder, item); | ||
| Span span = builder.startSpan(); | ||
| try (Scope ignored = span.makeCurrent()) { |
| String A2A_PAYLOAD_OMITTED = "a2a.payload.omitted"; | ||
| String A2A_PAYLOAD_PROJECTED_SIZE = "a2a.payload.projected_size"; | ||
| String A2A_PAYLOAD_TRUNCATED = "a2a.payload.truncated"; | ||
| String A2A_TASK_ID = "a2a.task.id"; |
There was a problem hiding this comment.
The new logs use a2a.task.id, a2a.context.id, a2a.message.id, a2a.operation.name, ... The spans in the same module use gen_ai.agent.a2a.task_id, gen_ai.agent.a2a.context_id, gen_ai.agent.a2a.config_id and so on.
Semantic conventions expect one attribute name per concept, independently of the signal.
| String event, | ||
| String operation, | ||
| String direction, | ||
| @Nullable String kind, |
There was a problem hiding this comment.
Kind MUST NOT be a string but an enum, otherwise all kinds of things from different domains can become a kind.
| logDelivery(A2A_KIND_ERROR, event); | ||
| } | ||
| String desc = t.getMessage() != null ? t.getMessage() : t.getClass().getName(); | ||
| span.setAttribute(ERROR_TYPE, desc); |
There was a problem hiding this comment.
Error type must be low carnality. It must not be a message.
| Logger logger, @Nullable ProtocolPayloadFormatter payloadFormatter, Severity severity) { | ||
| this.logger = Assert.checkNotNullParam("logger", logger); | ||
| this.payloadFormatter = payloadFormatter; | ||
| this.severity = Assert.checkNotNullParam("severity", severity); |
There was a problem hiding this comment.
Are you sure that you want to decide the severity in the constructor?
The severity is chosen once, when the emitter is built, and every record uses it whatever the event or outcome.
| .setSeverityText(severity.name()) | ||
| .setEventName(event) | ||
| .setBody(event) | ||
| .setAttribute(A2AObservabilityNames.A2A_EVENT_NAME, event) |
There was a problem hiding this comment.
Are you sure you need to set the event in 3 different fields?
Opt-in client and server events cover requests, streams, transport selection, and push notifications, with trace correlation across async callbacks. Payload capture is separately opt-in with configurable masking and size limits.
This fixes #1221 🦕