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

feat(nodejs): configureAwsInstrumentation for downstream #1375

Merged
merged 3 commits into from
Jun 13, 2024

Conversation

mzl-md
Copy link
Contributor

@mzl-md mzl-md commented Jun 11, 2024

Enables downstream repos to configure @opentelemetry/instrumentation-aws-sdk using the global function configureAwsInstrumentation(). This is required for setting custom user attributes as described in the documentation.

@mzl-md mzl-md requested a review from a team June 11, 2024 09:49
Copy link

linux-foundation-easycla bot commented Jun 11, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

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

Would you mind adding a unit test to validate this code path is working properly and doesn't have regressions in the future?

@mzl-md
Copy link
Contributor Author

mzl-md commented Jun 12, 2024

Currently there are no unit tests at all. Setting up a test framework is not really the scope of this PR.

Also, I can't find a good way to do this. Sinon fails to stub registerInstrumentations() or the constructor of AwsInstrumentation and I see no other way to check what arguments are passed to AwsInstrumentation. I added a trivial test that checks if the global function is called, but I think that is not really helpful.

In my opinion, meaningful tests require restructuring the wrapper.ts in separate files.

Copy link
Member

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

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

That's a reasonable criticism. I'd love to see contributions to help us improve testing across each language, but I understand that's not reasonable scope in a PR like this. I appreciate you making an effort to at least add something.

@tylerbenson tylerbenson merged commit e6ef326 into open-telemetry:main Jun 13, 2024
10 checks passed
@tylerbenson tylerbenson added the javascript Pull requests that update Javascript code label Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants