-
Notifications
You must be signed in to change notification settings - Fork 110
[aws-sdk]Added Initial auto-instrumentation library for aws-sdk #361
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
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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #361 +/- ##
============================================
- Coverage 82.14% 82.13% -0.01%
- Complexity 1801 1808 +7
============================================
Files 137 138 +1
Lines 7594 7647 +53
============================================
+ Hits 6238 6281 +43
- Misses 1356 1366 +10 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:
|
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.
Please can you also copy one of the .gitattributes
across from another contrib package and tweak appropriately, if required?
Done! |
@@ -60,6 +60,8 @@ splits: | |||
target: "https://${GH_TOKEN}@github.com/opentelemetry-php/contrib-auto-yii.git" | |||
- prefix: "src/Instrumentation/Doctrine" | |||
target: "https://${GH_TOKEN}@github.com/opentelemetry-php/contrib-auto-doctrine.git" | |||
- prefix: "src/Instrumentation/AwsSdk" | |||
target: "https://${GH_TOKEN}@github.com/opentelemetry-php/contrib-auto-aws-sdk.git" |
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've created this empty repo in preparation.
The tests aren't running for this one yet, @AsakerMohd can you add it to the top-level |
Added! |
@AsakerMohd now that checks+tests are running, you can address those failures. It will be fastest if you get them all passing locally ( |
Fixed the style issue and the build issue for php 8.1 workflow. The other failures are for instrumentation packages that aren't related to this change. |
Description:
There currently only exists a manual instrumentation library for the AWS SDK. It would be very helpful to also have an auto instrumentation library where customers only need to add one line in composer.json like
"open-telemetry/opentelemetry-auto-aws-sdk": "*",
to instrument their AWS SDK calls.This is an initial version of the auto instrumentation agent where it sets attributes such as
rpc.system, rpc.method, rpc.service, aws.region and aws.requestId.
. In the future, it should also support aws specific attributes for specific services such as bedrock, sns, sqs, etc. that have attributes such asgen_ai.agent.id, gen_ai.system, etc
Another thing that needs to be supported is propagation which I'm having difficulty figuring out where to add. I see it's being added to libraries such as Slim here:
opentelemetry-php-contrib/src/Instrumentation/Slim/src/SlimInstrumentation.php
Line 108 in 3835977
Enabling http instrumentation like slim produces duplicate spans. Lets say we make a call to S3. In this case, we have an aws-sdk span with
rpc.service = S3
and an outgoing http span with"url.full": "https://s3.amazonaws.com/"
. Is there a way to suppress the http instrumentation for this specific scenario? I know this is doable in other languages such as Java/DotNet.For testing:
I have this sample app: https://github.com/AsakerMohd/phpSample
I added the instrumentation path to that repo in the
composer.json
file:Then running the app through docker and making an API call, this is an example trace where only the aws-sdk instrumentation is enabled (notice the parent_id is empty. This is because this is the only instrumentation enabled. When something like slim is also enabled, the id is populated correctly):
For the integration test: Running
vendor/bin/phpunit tests/Integration/AwsSdkInstrumentationTest.php
runs the test there that asserts for the attributes and span and it passes as expected.