-
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
fix(instr-aws-sdk): @smithy/[email protected] change broke aws-sdk-v3 instrumentation #1913
Changes from all commits
9069a82
a3b1ec1
0fcca66
b66545e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,7 @@ | |
normalizeV3Request, | ||
removeSuffixFromStringIfExists, | ||
} from './utils'; | ||
import { propwrap } from './propwrap'; | ||
import { RequestMetadata } from './services/ServiceExtension'; | ||
import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; | ||
|
||
|
@@ -112,19 +113,24 @@ | |
v3MiddlewareStackFileNewVersions, | ||
]); | ||
|
||
// patch for @smithy/middleware-stack for aws-sdk packages v3.363.0+ | ||
const v3SmithyMiddlewareStackFile = new InstrumentationNodeModuleFile( | ||
'@smithy/middleware-stack/dist-cjs/MiddlewareStack.js', | ||
['>=1.0.1'], | ||
this.patchV3ConstructStack.bind(this), | ||
this.unpatchV3ConstructStack.bind(this) | ||
); | ||
// Patch for @smithy/middleware-stack for @aws-sdk/* packages v3.363.0+. | ||
// As of @smithy/[email protected] `constructStack` is only available | ||
// as a getter, so we cannot use `this._wrap()`. | ||
const self = this; | ||
const v3SmithyMiddlewareStack = new InstrumentationNodeModuleDefinition( | ||
'@smithy/middleware-stack', | ||
['>=2.0.0'], | ||
undefined, | ||
undefined, | ||
[v3SmithyMiddlewareStackFile] | ||
(moduleExports, moduleVersion) => { | ||
const newExports = propwrap( | ||
moduleExports, | ||
'constructStack', | ||
(orig: any) => { | ||
self._diag.debug('propwrapping aws-sdk v3 constructStack'); | ||
return self._getV3ConstructStackPatch(moduleVersion, orig); | ||
} | ||
); | ||
return newExports; | ||
} | ||
); | ||
|
||
const v3SmithyClient = new InstrumentationNodeModuleDefinition<typeof AWS>( | ||
|
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,155 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
/* | ||
* This block is derived from esbuild's bundling support. | ||
* https://github.com/evanw/esbuild/blob/v0.14.42/internal/runtime/runtime.go#L22 | ||
* | ||
* License: | ||
* MIT License | ||
* | ||
* Copyright (c) 2020 Evan Wallace | ||
* | ||
* Permission is hereby granted, free of charge, to any person obtaining a copy | ||
* of this software and associated documentation files (the "Software"), to deal | ||
* in the Software without restriction, including without limitation the rights | ||
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
* copies of the Software, and to permit persons to whom the Software is | ||
* furnished to do so, subject to the following conditions: | ||
* | ||
* The above copyright notice and this permission notice shall be included in all | ||
* copies or substantial portions of the Software. | ||
* | ||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
* SOFTWARE. | ||
*/ | ||
const __defProp = Object.defineProperty; | ||
const __getOwnPropDesc = Object.getOwnPropertyDescriptor; | ||
const __hasOwnProp = Object.prototype.hasOwnProperty; | ||
const __getOwnPropNames = Object.getOwnPropertyNames; | ||
const __copyProps = ( | ||
to: any, | ||
from: any, | ||
except: string, | ||
desc?: PropertyDescriptor | undefined | ||
) => { | ||
if ((from && typeof from === 'object') || typeof from === 'function') { | ||
for (const key of __getOwnPropNames(from)) { | ||
if (!__hasOwnProp.call(to, key) && key !== except) { | ||
__defProp(to, key, { | ||
get: () => from[key] as any, | ||
enumerable: !(desc = __getOwnPropDesc(from, key)) || desc.enumerable, | ||
}); | ||
} | ||
} | ||
} | ||
return to; | ||
}; | ||
|
||
/** | ||
* Return a new object that is a copy of `obj`, with its `subpath` property | ||
* replaced with the return value of `wrapper(original)`. | ||
* | ||
* This is similar to shimmer (i.e. `InstrumentationBase.prototype._wrap`). | ||
* However, it uses a different technique to support wrapping properties that | ||
* are only available via a getter (i.e. their property descriptor is `.writable | ||
* === false`). | ||
* | ||
* For example: | ||
* var os = propwrap(require('os'), 'platform', (orig) => { | ||
* return function wrappedPlatform () { | ||
* return orig().toUpperCase() | ||
* } | ||
* }) | ||
* console.log(os.platform()) // => DARWIN | ||
* | ||
* The subpath can indicate a nested property. Each property in that subpath, | ||
* except the last, must identify an *Object*. | ||
* | ||
* Limitations: | ||
* - This doesn't handle possible Symbol properties on the copied object(s). | ||
* - This cannot wrap a property of a function, because we cannot create a | ||
* copy of the function. | ||
* | ||
* @param {object} obj | ||
* @param {string} subpath - The property subpath on `obj` to wrap. This may | ||
* point to a nested property by using a '.' to separate levels. For example: | ||
* var fs = wrap(fs, 'promises.sync', (orig) => { ... }) | ||
* @param {Function} wrapper - A function of the form `function (orig)`, where | ||
* `orig` is the original property value. This must synchronously return the | ||
* new property value. | ||
* @returns {object} A new object with the wrapped property. | ||
* @throws {TypeError} if the subpath points to a non-existant property, or if | ||
* any but the last subpath part points to a non-Object. | ||
*/ | ||
export const propwrap = (obj: any, subpath: string, wrapper: Function): any => { | ||
const parts = subpath.split('.'); | ||
const namespaces = [obj]; | ||
let namespace = obj; | ||
let key; | ||
let val; | ||
|
||
// 1. Traverse the subpath parts to sanity check and get references to the | ||
// Objects that we will be copying. | ||
for (let i = 0; i < parts.length; i++) { | ||
key = parts[i]; | ||
val = namespace[key]; | ||
if (!val) { | ||
throw new TypeError( | ||
`cannot wrap "${subpath}": "<obj>.${parts | ||
.slice(0, i) | ||
.join('.')}" is ${typeof val}` | ||
); | ||
} else if (i < parts.length - 1) { | ||
if (typeof val !== 'object') { | ||
throw new TypeError( | ||
`cannot wrap "${subpath}": "<obj>.${parts | ||
.slice(0, i) | ||
.join('.')}" is not an Object` | ||
); | ||
} | ||
namespace = val; | ||
namespaces.push(namespace); | ||
} | ||
} | ||
|
||
// 2. Now work backwards, wrapping each namespace with a new object that has a | ||
// copy of all the properties, except the one that we've wrapped. | ||
for (let i = parts.length - 1; i >= 0; i--) { | ||
key = parts[i]; | ||
namespace = namespaces[i]; | ||
if (i === parts.length - 1) { | ||
const orig = namespace[key]; | ||
val = wrapper(orig); | ||
} else { | ||
val = namespaces[i + 1]; | ||
} | ||
const desc = __getOwnPropDesc(namespace, key); | ||
const wrappedNamespace = __defProp({}, key, { | ||
value: val, | ||
enumerable: !desc || desc.enumerable, | ||
}); | ||
__copyProps(wrappedNamespace, namespace, key); | ||
namespaces[i] = wrappedNamespace; | ||
} | ||
|
||
return namespaces[0]; | ||
}; |
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.