-
Notifications
You must be signed in to change notification settings - Fork 542
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(instr-aws-sdk): @smithy/middleware-stack@2.1.0 change broke aws-sdk-v3 instrumentation #1913
Changes from 1 commit
9069a82
a3b1ec1
0fcca66
b66545e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
…e TAV versions tested
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,18 @@ | ||
"aws-sdk": | ||
# A small subset of releases in the range [2.308.0, 3) to reduce testing time. | ||
versions: "2.308.0 || 2.548.0 || 2.785.0 || 2.1025.0 || 2.1265.0 || 2.1506.0 || >=2.1508.0" | ||
versions: "2.308.0 || 2.556.0 || 2.801.0 || 2.1049.0 || 2.1297.0 || 2.1546.0 || >=2.1548.0" | ||
commands: | ||
- npm run test | ||
|
||
"@aws-sdk/client-s3": | ||
# A small subset of releases in the range [3.6.1, 4) to reduce testing time. | ||
# - 3.377.0 was a bad release (see issue #1828). | ||
versions: "3.6.1 || 3.53.0 || 3.163.0 || 3.266.0 || 3.354.0 || 3.458.0 || >=3.462.0" | ||
versions: "3.6.1 || 3.55.0 || 3.180.0 || 3.289.0 || 3.385.0 || 3.498.0 || >=3.503.1" | ||
commands: | ||
- npm run test | ||
|
||
"@aws-sdk/client-sqs": | ||
# A small subset of releases in the range [3.24.0, 4) to reduce testing time. | ||
versions: "3.24.0 || 3.85.0 || 3.194.0 || 3.278.0 || 3.357.0 || 3.461.0 || >=3.462.0" | ||
versions: "3.24.0 || 3.94.0 || 3.202.0 || 3.296.0 || 3.388.0 || 3.496.0 || >=3.503.1" | ||
commands: | ||
- npm run test |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note for reviewers: I'm open to suggestions if we want this in a more shareable place. Or perhaps we consider that if/when there is some other instrumentation that requires a similar facility for wrapping. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's definitely useful to have it in a more shareable place. I think if we run into the same problem somewhere else, we can consider moving that to the |
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.
Note for reviewers: There is now no unwrapping. I'm not sure if that has possible adverse effects.
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 have to admit i'm not as knowledgeable on this as I'd like. IIUC we use the unwrapping mostly for testing and ensuring we don't double wrap a library (using auto-instrumentations and aws-sdk instrumentation directly and registering the same one twice, or registering it twice, but with different instrumentation versions). It could also be that users want to disable the instrumentation on runtime for some reason (though, I've never seen such a thing done IRL).
Hoping that maybe @blumamir or @dyladan have more insights.
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.
This was discussed in the OTel JS SIG today (https://docs.google.com/document/d/1tCyoQK49WVcE-x8oryZOTTToFm7sIeUhxFPm9g-qL1k/edit#heading=h.7y4kcwttgzlh). Dan suggested not having an unwrap would be fine. In his opinion (I hope I'm not misrepresenting), instrumentation unwrap functionality in general is really there for testing and probably isn't something to rely heavily on for production usage.