-
Notifications
You must be signed in to change notification settings - Fork 832
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(instrumentation): add support for esm module #2846
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
47 changes: 47 additions & 0 deletions
47
experimental/packages/opentelemetry-instrumentation/test/node/esmInstrumentation.test.mjs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
/* | ||
* 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. | ||
*/ | ||
|
||
import * as assert from 'assert' | ||
import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '../../build/src/index.js'; | ||
|
||
describe('when loading esm module', () => { | ||
it('should patch module file', async () => { | ||
class TestInstrumentation extends InstrumentationBase { | ||
constructor(onPatch, onUnpatch) { | ||
super('my-esm-instrumentation', '0.1.0'); | ||
} | ||
|
||
init() { | ||
return [ | ||
new InstrumentationNodeModuleDefinition( | ||
'my-esm-module', | ||
['*'], | ||
(exports, version) => { | ||
exports.myConstant = 43; | ||
exports.myFunction = () => 'another'; | ||
} | ||
) | ||
]; | ||
} | ||
} | ||
|
||
const instrumentation = new TestInstrumentation(); | ||
instrumentation.enable(); | ||
const exported = await import('my-esm-module'); | ||
assert.deepEqual(exported.myConstant, 43); | ||
assert.deepEqual(exported.myFunction(), 'another'); | ||
}); | ||
}); |
Empty file.
6 changes: 6 additions & 0 deletions
6
.../packages/opentelemetry-instrumentation/test/node/node_modules/my-esm-module/package.json
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
6 changes: 6 additions & 0 deletions
6
...packages/opentelemetry-instrumentation/test/node/node_modules/my-esm-module/src/index.mjs
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 would recommend creating a wrapper for the hook so you can have a loader hook name that's clearly in the otel namespace rather than the unfamiliar import-in-the-middle naming. That's what we do at Datadog. We tell users to use dd-trace/loader-hook.js which just loads the IITM hook: https://github.com/DataDog/dd-trace-js/blob/master/loader-hook.mjs
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.
Thanks for the suggestion, i would like other opinion on this since this would result in using
--experimental-loader=@opentelemetry-instrumentation/hook.mjs
instead. Also since the instrumentation is built for multiple platforms i'm not sure this would not create more problems as we support more (deno comes to my mind)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.
The hook could be a separate package. My suggestion is just to make the connection to otel clearer in some way.
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.
@open-telemetry/javascript-maintainers I would like in put on this, i'm not sure what's the best way to do this:
/plaform/node
? at the root ? Since user would need to require it directly, that means we need it to be at the root ofbuild
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.
Between those options i'd rather have a dedicated package. Honestly though I think it's fine to just use RITM directly. It's just one more thing for us to build, maintain, and publish
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 can guess that @Qard being at the other end by maintaining ITTM doesn't want to get questions about ESM instrumentation within otel but maybe i'm mistaken ? I agree that its more complicated to have one empty package on our end just for this.
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.
--experimental-loader=@opentelemetry-instrumentation/hook.mjs
looks good to me. I didn't find a new dedicated package necessary here. The hook entry files to be published will be rather small. Reducing the number of OTEL packages can alleviate the burden of setting up an OTEL application.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.
Do you mean
--experimental-loader=@opentelemetry/instrumentation/hook.mjs
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.
My concern is more just users being confused about needing
--experimental-loader=import-in-the-middle/hook.mjs
which is loading some other module than opentelemetry which they are likely unfamiliar with. Having an opentelemetry-specific proxy to it makes it clearer that this is a thing opentelemetry wants from the user. It also doesn't leave you tied to some specific external library you don't control. If at some point in the future IITM no longer does what you need or maintenance dies or whatever else, you can simply change the contents of your hook file without needing to ask all your users to update their CLI args. Changing CLI args sounds like it should be a trivial thing, but stuff like that often gets encoded deep in infra configs and places like that which creates unnecessary friction if changes need to be made in the future. It's just a better idea to have an entrypoint for that which you control.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 would also choose the way of having own wrapper for loading a non-core external library. If we were speaking about browser, it would probably make sense to keep the external dep as is to reduce the generated client's bundle size in case this lib is reused outside of opentelemetry as well. For node it's irrelevant, there are no cons of wrapping it