fix(AwsSdk): handle missing x-amz-request-id in metadata#524
Conversation
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
| $span->setAttribute(TraceAttributes::HTTP_RESPONSE_STATUS_CODE, $result['@metadata']['statusCode']); // @phan-suppress-current-line PhanTypeMismatchDimFetch | ||
| $span->setAttribute(TraceAttributes::AWS_REQUEST_ID, $result['@metadata']['headers']['x-amz-request-id']); // @phan-suppress-current-line PhanTypeMismatchDimFetch | ||
|
|
||
| $awsRequestId = $result['@metadata']['headers']['x-amz-request-id'] ?? null; |
There was a problem hiding this comment.
This will fix the bug. Should we address the other options later? I was not 100% sure but based on the AWS docs this information can have several headers
There was a problem hiding this comment.
@scprek I think we should fix the bug in this PR. The use of other headers should be added in another PR, I think.
There was a problem hiding this comment.
The additional header checks are fairly trivial, so no objections to adding them in this PR from me!
There was a problem hiding this comment.
Yes, of course. I added it))
There was a problem hiding this comment.
Amazing, thanks for your efforts!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #524 +/- ##
=========================================
Coverage 82.15% 82.15%
- Complexity 2297 2298 +1
=========================================
Files 156 156
Lines 8547 8553 +6
=========================================
+ Hits 7022 7027 +5
- Misses 1525 1526 +1 Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Thanks @adiafora - please can you address the failing build pipelines for the changed instrumentation? |
|
@ChrisLightfootWild CI is failing on |
Leaving that to be fixed separately is good for that one. 👍 |
|
@bobstrecansky can you merge this one if you are happy please? |
4 participants
Fixes open-telemetry/opentelemetry-php#1897