-
Notifications
You must be signed in to change notification settings - Fork 534
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
feat(aws-sdk): add s3 and kinesis service extensions for aws-sdk instrumentation #2361
feat(aws-sdk): add s3 and kinesis service extensions for aws-sdk instrumentation #2361
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2361 +/- ##
==========================================
+ Coverage 90.83% 90.85% +0.01%
==========================================
Files 159 161 +2
Lines 7831 7859 +28
Branches 1610 1614 +4
==========================================
+ Hits 7113 7140 +27
- Misses 718 719 +1
|
Pinging @blumamir for review! Unrelated heads-up, I'm looking to take over component ownership of AWS JS components from @carolabadeer. I think I just need to make a PR like #1464. |
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.
Thank you for the contribution and the info in the PR.
Left few comments. Some of them I wrote on s3 implementation but they also apply to kinesis, didn't want to duplicate the same comments twice :)
plugins/node/opentelemetry-instrumentation-aws-sdk/src/utils.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/s3.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/s3.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/s3.ts
Outdated
Show resolved
Hide resolved
import { AwsInstrumentation } from '../src'; | ||
registerInstrumentationTesting(new AwsInstrumentation()); | ||
|
||
import * as AWS from 'aws-sdk'; |
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.
awesome to have tests for that.
Please note that those tests are for v2 which will soon be end of life.
Consider also adding assertions on these new attributes to v3 tests in test/aws-sdk-v3.test.ts
here
*Issue #, if available:* *Description of changes:* 1. `aws-attribute-keys.ts`: - Update values for keys: `AWS_BUCKET_NAME, AWS_QUEUE_URL, AWS_QUEUE_NAME, AWS_STREAM_NAME, AWS_TABLE_NAME` in order to match the actual attribute collected from AWS SDK auto-instrumentation 2. `aws-metric-attribute-generator.ts`: - [Similarly to Python](https://github.com/aws-observability/aws-otel-python-instrumentation/blob/2c00bd07eaaf703880a24a2fcfe874cdb4196678/aws-opentelemetry-distro/src/amazon/opentelemetry/distro/_aws_metric_attribute_generator.py#L378-L380), accommodates the fact that AWS_ATTRIBUTE_KEYS.AWS_TABLE_NAMES [has an array of table names](https://github.com/open-telemetry/opentelemetry-js-contrib/blob/931318cac21ee3707f3735a64ac751566ee37182/plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/dynamodb.ts#L99-L105) from aws-sdk dynamodb client instrumentation 3. `aws-opentelemetry-configurator.ts`: - Takes and sets instrumentations in constructor. 4. `instrumentation-patch.ts`: - Applies patches for AWS SDK Instrumentation 5. `register.ts`: - Applies patches from (4.) by default, unless `AWS_APPLY_PATCHES` env var is not `'true'` The following files are copied from upstream: ``` aws-distro-opentelemetry-node-autoinstrumentation/src/patches/aws/services/ServiceExtension.ts ``` The following files are being contributed to upstream: See: open-telemetry/opentelemetry-js-contrib#2361 ``` aws-distro-opentelemetry-node-autoinstrumentation/src/patches/aws/services/kinesis.ts aws-distro-opentelemetry-node-autoinstrumentation/src/patches/aws/services/s3.ts ``` *Testing:* - Add unit tests By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
7794f36
to
ed94e6c
Compare
f382a46
to
b97d73a
Compare
Hi, looking for another round of review! I removed some of the aws-sdk v2 tests I had previously given their end-of-life and soon-to-be-deprecated statuses, and removed the latency with the Kinesis Client test (enforced HTTP instead of HTTP2 which isn't supported by |
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.
LGTM 💯
Thank you for the contribution, the code and tests looks great and the description on the PR is very helpful.
Which problem is this PR solving?
Short description of the changes
Regarding some added constants to the
utils
file:_AWS_S3_BUCKET
should be added to OTel JS Semantic Attributes in the future._AWS_KINESIS_STREAM_NAME
, but some AWS repos are already usingaws.kinesis.stream.name
Testing
'aws.kinesis.stream.name': myNewKinesisStream
'aws.s3.bucket': test-bucket-not-exist-or-accessible