-
Notifications
You must be signed in to change notification settings - Fork 655
fix(py): implement SigV4 signing for AWS X-Ray OTLP exporter #4402
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 @yesudeep, 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 addresses a critical authentication gap in the AWS X-Ray OTLP exporter. Previously, the exporter failed to sign requests with AWS SigV4, leading to authentication errors when traces were sent directly to X-Ray. The changes introduce a robust mechanism using a custom Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 effectively implements AWS SigV4 signing for the X-Ray OTLP exporter, which is a crucial fix for direct-to-endpoint tracing without a collector. The approach of using a custom requests.HTTPAdapter is clean and well-integrated with the existing OTLPSpanExporter. The new SigV4SigningAdapter and _create_sigv4_session helper are well-designed and documented. The accompanying tests are comprehensive, covering initialization, signing logic, and session creation, which provides good confidence in the change. I have one minor suggestion regarding import style in the test file to align with PEP 8 conventions. Overall, this is an excellent contribution that significantly improves the AWS telemetry plugin.
f184864 to
b3c71e6
Compare
7c1e3ce to
4a9fdca
Compare
4a9fdca to
4aaeda8
Compare
The AwsXRayOtlpExporter was not signing requests with AWS SigV4, which would cause authentication failures when sending traces to AWS X-Ray endpoints without a collector. This fix: - Adds SigV4SigningAdapter that intercepts HTTP requests and signs them with AWS SigV4 authentication using botocore - Creates _create_sigv4_session() helper that builds a requests Session with the signing adapter mounted for HTTPS - Passes the signing session to OTLPSpanExporter so all requests are automatically signed - Removes the unused _sign_request() method The implementation uses botocore's SigV4Auth to add Authorization, X-Amz-Date, and X-Amz-Security-Token headers to each request. Added tests: - SigV4SigningAdapter initialization and signing - Session creation with adapter mounting - Exporter session configuration verification Fixes: gemini-code-assist review on PR #4390
4aaeda8 to
484c231
Compare
Pull request was converted to draft
The AwsXRayOtlpExporter was not signing requests with AWS SigV4, which would cause authentication failures when sending traces to AWS X-Ray endpoints without a collector.
This PR implements proper SigV4 signing logic:
Tests added to verify:
This fixes the issue reported in PR #4390 code review.