fix(instrumentation-node): recursively discover package.json #2206
Conversation
…parent directories
adambartholomew
requested review from
dyladan,
Flarna,
johnbley,
legendecas,
markwolff,
mayurkale22,
mwear,
naseemkullah,
obecny,
OlivierAlbertini and
vmarchaud
as code owners
|
|
packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts
Outdated
Show resolved
Hide resolved
| protected getModuleVersion(directory: string): string { | ||
| const modulePackage = this.findModulePackage(directory); | ||
| if (!modulePackage) { | ||
| return '0.0.0'; |
There was a problem hiding this comment.
i don't think we should return a invalid version if the version isn't found, i believe module.moduleVersion can already be undefined
There was a problem hiding this comment.
version is used further down with isSupported and ultimately semver.satisfies(). I'm not familiar enough to determine the behaviour if we were to let undefined into these methods.
packages/opentelemetry-instrumentation/test/common/Instrumentation.test.ts
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #2206 +/- ##
==========================================
- Coverage 92.72% 92.49% -0.24%
==========================================
Files 141 124 -17
Lines 5089 4146 -943
Branches 1047 848 -199
==========================================
- Hits 4719 3835 -884
+ Misses 370 311 -59 |
…-error within tests
| @@ -56,4 +57,62 @@ describe('BaseInstrumentation', () => { | |||
| assert.strictEqual(called, true); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('_onRequire', () => { | |||
There was a problem hiding this comment.
it should not be added here but in node test only. Common is run for browser and for node
There was a problem hiding this comment.
This explains the CI failure. The browser tests won't run on my machine. I'll move these tests. Apologies for being new, appreciate all the help.
There was a problem hiding this comment.
No need to apology, we are here to help :), in tests you have folders: browser, node, common, just move your tests to folder node. This way your tests will be run only in node environment
|
Tell me if i'm wrong but recursively crawling up the node_modules directory tree is quite error prone. Picture an application which has 2 instrumentations which each depend on a not-quite-equal version of
If you try to crawl up from either of these instrumentation modules, you will hit the instrumentation itself before you hit the user application. If you have 2 instrumentations which agree on an instrumentation version though, you might have this:
If you crawl up from this instrumentation (deduplicated by npm) you will hit the user application, not the instrumentation you were looking for. This is assuming you don't have an even more complex installation potentially involving metapackages which install many instrumentations. |
|
I tend to agree this could be problematic. Is there a better approach for this? In the bug the issue was for a require within the application, not the instrumentation. Is the application package.json version number important for this tooling? Is there a way to safely bypass looking for a package file, or capture the exception? |
|
Maybe we can change the logic to look through the supported module versions to see if the package name matches any of them before we even attempt to look for a version. We only need to look for the version if the name matches (and there is a version to check against). |
|
Lots of good discussions in the github issue. This doesn't seem like the correct approach. |
Recursively discover package.json from module parent directories
Which problem is this PR solving?
Fixes #2193
Short description of the changes
0.0.0when undiscovered rather than a hard crash