Skip to content
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

fix(Aws): fixes wrong aws client name and region binding when appending middlewares #326

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rtreffler
Copy link

Binding happened outside the foreach loop resulting in binding the same values for all instrumented clients. Similar problem was affecting span and scope object variables, where multiple clients could overwrite each other spans.

…ng middlewares

Binding happened outside the foreach loop resulting in binding the same values for all instrumented clients.
Similar problem was affecting span and scope object variables, where multiple clients could overwrite each other spans.
@rtreffler rtreffler requested a review from a team as a code owner February 4, 2025 16:16
Copy link

welcome bot commented Feb 4, 2025

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.

Copy link

linux-foundation-easycla bot commented Feb 4, 2025

CLA Not Signed

@@ -0,0 +1,149 @@
<?php
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file is copied almost exactly how it was written in AWS PHP SDK: https://github.com/aws/aws-sdk-php/blob/master/tests/UsesServiceTrait.php

not sure that is important, thought its worth mentioning

@bobstrecansky
Copy link
Collaborator

Thank you @rtreffler! Once you've signed the CLA we'll be able to review and merge.

Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.87%. Comparing base (ee99079) to head (a00a991).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #326   +/-   ##
=========================================
  Coverage     85.87%   85.87%           
  Complexity     1081     1081           
=========================================
  Files            73       73           
  Lines          4480     4480           
=========================================
  Hits           3847     3847           
  Misses          633      633           
Flag Coverage Δ
Instrumentation/CakePHP 20.00% <ø> (ø)
Instrumentation/CodeIgniter 73.77% <ø> (ø)
Instrumentation/Curl 90.66% <ø> (ø)
Instrumentation/Guzzle 69.51% <ø> (ø)
Instrumentation/HttpAsyncClient 81.25% <ø> (ø)
Instrumentation/IO 70.68% <ø> (ø)
Instrumentation/MongoDB 72.64% <ø> (ø)
Instrumentation/MySqli 96.10% <ø> (ø)
Instrumentation/OpenAIPHP 87.31% <ø> (ø)
Instrumentation/PDO 89.95% <ø> (ø)
Instrumentation/Psr14 77.14% <ø> (ø)
Instrumentation/Psr15 93.82% <ø> (ø)
Instrumentation/Psr16 97.56% <ø> (ø)
Instrumentation/Psr18 81.15% <ø> (ø)
Instrumentation/Psr6 97.67% <ø> (ø)
Instrumentation/Slim 86.89% <ø> (ø)
Instrumentation/Symfony 88.70% <ø> (ø)
Instrumentation/Yii 77.68% <ø> (ø)
Propagation/ServerTiming 100.00% <ø> (ø)
Propagation/TraceResponse 100.00% <ø> (ø)
ResourceDetectors/Container 93.02% <ø> (ø)
Shims/OpenTracing 92.45% <ø> (ø)
Symfony 87.81% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee99079...a00a991. Read the comment docs.

@rtreffler
Copy link
Author

Thank you @rtreffler! Once you've signed the CLA we'll be able to review and merge.

The company I'm currently working for is already working on the CLA topic.

…strumentation for the same client

Async calls would overwrite spans of different clients, so spl_object_has is used to keep span next to its client.
Also since we can instrument many clients via consecutive calls to `instrumentClient` and `activate` check is added to prevent duplicate middleware being added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants