fix(instr-aws-sdk): @smithy/[email protected] change broke aws-sdk-v3 instrumentation #1913
Conversation
…dk-v3 instrumentation As of smithy-lang/smithy-typescript#1146 (details at smithy-lang/smithy-typescript#1113) the CommonJS export for many (all?) `@smithy/*` packages is now an esbuild bundle -- all in `dist-cjs/index.js`. That means that subfile patching like this no longer works: ```js const v3SmithyMiddlewareStackFile = new InstrumentationNodeModuleFile( '@smithy/middleware-stack/dist-cjs/MiddlewareStack.js', ['>=1.0.1'], this.patchV3ConstructStack.bind(this), this.unpatchV3ConstructStack.bind(this) ); ``` In our case this breaks as of `@smithy/[email protected]` released 2024-01-17T22:26:42.432Z. This is considered a non-breaking change, so the dependency ranges for earlier released versions of `@smithy/smithy-client` will pick this up. A fix is to change the `@smithy/middleware-stack` patching to be on the top-level module exports. Because the `constructStack` field is only available as a getter we cannot use shimmer (InstrumentationBase._wrap). Instead this returns a new `moduleExports` object with a new getter that shims the call.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1913 +/- ##
==========================================
- Coverage 91.50% 91.02% -0.49%
==========================================
Files 145 146 +1
Lines 7431 7478 +47
Branches 1489 1497 +8
==========================================
+ Hits 6800 6807 +7
- Misses 631 671 +40
|
There was a problem hiding this comment.
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.
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 @opentelemetry/instrumentation package.
| } | ||
| ); | ||
| return newExports; | ||
| } |
There was a problem hiding this comment.
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.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 @opentelemetry/instrumentation package.
plugins/node/opentelemetry-instrumentation-aws-sdk/src/propwrap.ts
Outdated
Show resolved
Hide resolved
| } | ||
| ); | ||
| return newExports; | ||
| } |
There was a problem hiding this comment.
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).
|
FWIW, here is a test-all-versions run (locally) showing it passing for current versions of This was before I updated .tav.yml to reduce the number of versions tested. |
|
Ready for review again, @pichlermarc PTAL if you have time. Thanks. |
There was a problem hiding this comment.
Thanks @trentm for taking the time to fix this 👍
Let's get this merged 🙂
…dk-v3 instrumentation (open-telemetry#1913) As of smithy-lang/smithy-typescript#1146 (details at smithy-lang/smithy-typescript#1113) the CommonJS export for many (all?) `@smithy/*` packages is now an esbuild bundle -- all in `dist-cjs/index.js`. That means that subfile patching like this no longer works: ```js const v3SmithyMiddlewareStackFile = new InstrumentationNodeModuleFile( '@smithy/middleware-stack/dist-cjs/MiddlewareStack.js', ['>=1.0.1'], this.patchV3ConstructStack.bind(this), this.unpatchV3ConstructStack.bind(this) ); ``` In our case this breaks as of `@smithy/[email protected]` released 2024-01-17T22:26:42.432Z. This is considered a non-breaking change, so the dependency ranges for earlier released versions of `@smithy/smithy-client` will pick this up. A fix is to change the `@smithy/middleware-stack` patching to be on the top-level module exports. Because the `constructStack` field is only available as a getter we cannot use shimmer (InstrumentationBase._wrap). Instead this returns a new `moduleExports` object with a new getter that shims the call This PR also updates .tav.yml to reduce the number of aws-sdk package versions tested.
TAV tests for instrumentation-aws-sdk are currently breaking. E.g.: https://github.com/open-telemetry/opentelemetry-js-contrib/actions/runs/7617703422/job/20747285913
details
As of smithy-lang/smithy-typescript#1146
(details at smithy-lang/smithy-typescript#1113)
the CommonJS export for many (all?)
@smithy/*packages is now an esbuildbundle -- all in
dist-cjs/index.js. That means that subfile patching like thisno longer works:
In our case this breaks as of
@smithy/[email protected]released2024-01-17T22:26:42.432Z. This is considered a non-breaking change, so the
dependency ranges for earlier released versions of
@smithy/smithy-clientwillpick this up.
A fix is to change the
@smithy/middleware-stackpatching to be on the top-levelmodule exports. Because the
constructStackfield is only available as a getterwe cannot use shimmer (InstrumentationBase._wrap). Instead this returns a new
moduleExportsobject with a new getter that shims the call.