-
Notifications
You must be signed in to change notification settings - Fork 296
support HTTP & GRPC protocol in sia otel instrumentation #3105
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
Conversation
1f2f009 to
fa9ab71
Compare
Signed-off-by: Po-Yao Chen <[email protected]>
fa9ab71 to
4b79d12
Compare
|
/gemini review |
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 both HTTP and gRPC protocols for the OpenTelemetry collector endpoint, allowing users to specify the protocol in the endpoint URL. The changes are well-structured, introducing protocol detection logic and conditional exporter creation for gRPC and HTTP. Comprehensive tests for HTTP, HTTPS, and gRPC have been added, which is excellent. I have a few suggestions to improve the robustness of the protocol detection logic and to align error handling with common Go practices. Overall, this is a valuable enhancement to the SIA OTEL instrumentation.
Signed-off-by: Po-Yao Chen <[email protected]>
Signed-off-by: Po-Yao Chen <[email protected]>
|
/gemini review |
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 introduces support for HTTP and gRPC protocols in the SIA OTel instrumentation. It includes changes to go.mod, go.sum, config.go, and introduces new files for metric sending and utility functions. The changes allow users to specify collector endpoints with HTTP/HTTPS/GRPC schemes. I have provided review comments to address potential issues and improve the code.
Signed-off-by: Po-Yao Chen <[email protected]>
|
/gemini review |
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 both HTTP and gRPC protocols for OpenTelemetry metric exporting. The changes include adding the gRPC OTLP exporter dependency, refactoring the metric sender to dynamically choose the exporter based on the collector endpoint URL, and adding utility functions for protocol detection. The implementation is supported by a comprehensive set of new unit tests for HTTP, HTTPS, and gRPC scenarios.
My review focuses on the robustness of the new protocol detection logic. I've identified a potential issue in the isGRPCProtocol utility function that could lead to incorrect protocol detection in some edge cases and have suggested a more robust implementation. I've also recommended adding more test cases to cover these edge cases. Overall, this is a great addition that increases the flexibility of the OTel instrumentation.
Signed-off-by: Po-Yao Chen <[email protected]>
Description
This PR enables SIA users to specify
http://<collector-endpoint>:<port>andgrpc://<collector-endpoint>:<port>in the otel config.Verification (HTTP)
Contribution Checklist:
Attach Screenshots (Optional)