-
Notifications
You must be signed in to change notification settings - Fork 96
feat: Add telemetry support. #463
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @ehsavoie, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates OpenTelemetry into the A2A Java SDK, providing robust distributed tracing capabilities. The changes enable automatic span creation for both client-initiated and server-processed A2A requests, offering deeper visibility into the flow of operations. This enhancement is crucial for monitoring, troubleshooting, and understanding the performance characteristics of applications built with the SDK. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds support for OpenTelemetry, which is a great addition for observability. The implementation introduces a new opentelemetry extras module and integrates tracing on both the client and server sides. On the client side, it uses reflection for optional wrapping, and on the server side, it uses CDI interceptors with a new @Trace annotation. My review focuses on improving the robustness and maintainability of this new feature. I've found a few critical issues, such as a ClassCastException in an attribute extractor and silent exception swallowing that could make debugging difficult. I've also identified several opportunities for code cleanup, such as fixing typos, removing duplicated code, and improving the design of the CDI interceptor to better support dependency injection. Addressing these points will make the OpenTelemetry integration more solid and easier to maintain.
transport/rest/src/main/java/io/a2a/transport/rest/handler/RestAttributeExtractor.java
Outdated
Show resolved
Hide resolved
...as/opentelemetry/src/main/java/io/a2a/extras/opentelemetry/OpenTelemetryClientTransport.java
Outdated
Show resolved
Hide resolved
...as/opentelemetry/src/main/java/io/a2a/extras/opentelemetry/OpenTelemetryClientTransport.java
Outdated
Show resolved
Hide resolved
extras/opentelemetry/src/main/java/io/a2a/extras/opentelemetry/SpanInterceptor.java
Outdated
Show resolved
Hide resolved
...as/opentelemetry/src/main/java/io/a2a/extras/opentelemetry/OpenTelemetryClientTransport.java
Show resolved
Hide resolved
...as/opentelemetry/src/main/java/io/a2a/extras/opentelemetry/OpenTelemetryClientTransport.java
Outdated
Show resolved
Hide resolved
extras/opentelemetry/src/main/java/io/a2a/extras/opentelemetry/SpanInterceptor.java
Outdated
Show resolved
Hide resolved
transport/grpc/src/main/java/io/a2a/transport/grpc/handler/GrpcHandler.java
Outdated
Show resolved
Hide resolved
transport/grpc/src/main/java/io/a2a/transport/grpc/handler/GrpctAttributeExtractor.java
Outdated
Show resolved
Hide resolved
brunobat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to decide if you are creating your own instrumentation annotations and API or if you just reuse the ones provided by OTel.
Also, the attribute names, should follow these semantic conventions, as much as possible: https://opentelemetry.io/docs/specs/semconv/gen-ai/gen-ai-agent-spans/
server-common/src/main/java/io/a2a/server/interceptors/AttributeExtractor.java
Outdated
Show resolved
Hide resolved
...as/opentelemetry/src/main/java/io/a2a/extras/opentelemetry/OpenTelemetryClientTransport.java
Outdated
Show resolved
Hide resolved
...as/opentelemetry/src/main/java/io/a2a/extras/opentelemetry/OpenTelemetryClientTransport.java
Outdated
Show resolved
Hide resolved
...as/opentelemetry/src/main/java/io/a2a/extras/opentelemetry/OpenTelemetryClientTransport.java
Outdated
Show resolved
Hide resolved
examples/helloworld/client/src/main/java/io/a2a/examples/helloworld/HelloWorldClient.java
Show resolved
Hide resolved
7661dd6 to
893ce8b
Compare
22a2af4 to
d35543a
Compare
kabir
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things which may or may not be needed :-)
| private final List<BiConsumer<ClientEvent, AgentCard>> consumers = new ArrayList<>(); | ||
| private @Nullable Consumer<Throwable> streamErrorHandler; | ||
| private @Nullable | ||
| Consumer<Throwable> streamErrorHandler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a bit strange :-)
The original looks better, but I think
@Nullable
private Consumer<Throwable> streamErrorHandler;
would look better. But I see we have private @Nullable .... 'everywhere' :-)
| .extractor(); | ||
|
|
||
| String name = jakartaContext.getTarget().getClass().getName(); | ||
| if (name != null && name.endsWith("_Subclass")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name cannot be null
| return Map.of("request", parameters[0].toString(), "extensions", context.getActivatedExtensions().stream().collect(Collectors.joining(",")), "a2a.method", (String) context.getState().get("method")); | ||
| } | ||
| default -> { | ||
| return Collections.emptyMap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could log a warning here. Or if the intent is that all DefaultRequestHandler methods are handled, we could throw an error?
| return Map.of("request", parameters[0].toString(), "extensions", GrpcContextKeys.EXTENSIONS_HEADER_KEY.get(), "a2a.method", GrpcContextKeys.GRPC_METHOD_NAME_KEY.get(currentContext)); | ||
| } | ||
| default -> { | ||
| return Collections.emptyMap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log warn/throw error as above
| return Map.of("body", parameters[0].toString(), "extensions", context.getActivatedExtensions().stream().collect(Collectors.joining(",")), "a2a.method", (String) context.getState().get(METHOD_NAME_KEY)); | ||
| } | ||
| default -> { | ||
| return Collections.emptyMap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warn/error
| return Map.of("contextId", (String) parameters[0], "status", (String) parameters[1], "extensions", context.getActivatedExtensions().stream().collect(Collectors.joining(",")), "a2a.method", (String) context.getState().get(METHOD_NAME_KEY)); | ||
| } | ||
| default -> { | ||
| return Collections.emptyMap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warn/error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, @ehsavoie it looks like you are adding Otel tracing to the example. But I don't know how to enable this etc.
Can you update the README with instructions?
Maybe we could also make this example testable in the CI, like we do for the cloud-deployment one?
Also, it would be good to have some tests in the extras module
* Adding an annotation that can be used by CDI interceptor to create spans on current exchanges. * Adding support for client wrappers to be able to add client side spans. * Updating the helloworld example to use opentelemetry. Fixing issue a2aproject#388 Signed-off-by: Emmanuel Hugonnet <[email protected]>
Adding support for OpenTelemetry
Fixes #388 🦕