fix(instrumentation): do not export Node.js types for Browser users #4378

pichlermarc · 2023-12-18T13:21:54Z

Which problem is this PR solving?

InstrumentationNodeModuleFile and InstrumentationNodeModuleDefinition were exported even if the @opentelemetry/instrumentation package is used in a browser context. This PR changes it so that the Node type are not exported for Browser users anymore.

Fixes #4373

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Testing locally against a project using webpack and including the setup provided in [Instrumentation] Cannot build web project using webpack due to reference to nodejs core package (path) #4373. I installed the @opentelemetry/instrumentation dependency as a packaged file that includes the changes from this PR.

codecov · 2023-12-18T13:46:35Z

Codecov Report

Merging #4378 (acd8fd8) into main (3cf2cf6) will increase coverage by 0.08%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4378      +/-   ##
==========================================
+ Coverage   92.23%   92.32%   +0.08%     
==========================================
  Files         332      330       -2     
  Lines        9467     9454      -13     
  Branches     2011     2010       -1     
==========================================
- Hits         8732     8728       -4     
+ Misses        735      726       -9

see 3 files with indirect coverage changes

pichlermarc · 2023-12-18T14:10:19Z

I changed the test file back to what it was before to get the tests to run. As I felt like I'm just hiding a problem by doing that, I double checked if it actually works in a new ESM project with Node.js v20, and it does not.

SyntaxError: The requested module '@opentelemetry/instrumentation' does not provide an export named 'InstrumentationBase'

But still, looking at the transpiled files, I'm exporting them (and it's working with older vesions). @dyladan since you worked on #4336 - do you know the reason why this wouldn't work on Node.js 20 but works fine on older versions?

pichlermarc · 2023-12-18T15:04:19Z

Ah this seems to be related to the loader changes that made it into Node 18.19.0.

So that's why the test also failed for Node 18 (https://github.com/open-telemetry/opentelemetry-js/actions/runs/7248698310/job/19745127352#step:8:4690). When I create an ESM app and import it there, it works with Node.js 18.18.2.

Maybe it has something to do with the loader or how it is set up? 🤔 If I don't use it, it works fine. (nothing is instrumented, obviously - but the import works)

pichlermarc · 2023-12-18T15:31:55Z

Ah, I see - this may be due to nodejs/import-in-the-middle#29
Looks like it has not been released yet (I'll test locally, then put the results here).

So it looks like we're in kind of a tough spot at the moment:

Browser users are broken until we fix it
Fixing it makes browser usage work again, but breaks users that instrument ESM on Node.js v18.19 and Node.js v20

An alternative may be #3989 (switching to functional ESM output likely means we can circumvent the ITTM issue, but I've not tested it yet - I'll test it if the IITM fix does not do what I think it does)

Flarna · 2023-12-18T15:39:28Z

An alternative may be #3989 (switching to functional ESM output likely means we can circumvent the ITTM issue, but I've not tested it yet - I'll test it if the IITM fix does not do what I think it does)

I doubt. Switching to ESM would break all CJS users as CJS can't use ESM.
The ESM instrumentation topics would persist because ESM customization hooks in node.js are still an ongoing topic. As a result breaking changes might happen.

pichlermarc · 2023-12-18T16:13:36Z

An alternative may be #3989 (switching to functional ESM output likely means we can circumvent the ITTM issue, but I've not tested it yet - I'll test it if the IITM fix does not do what I think it does)

I doubt. Switching to ESM would break all CJS users as CJS can't use ESM.

Fair point. I did not word that correctly: I meant to say double publishing CJS and ESM (not switching to) so that ESM users may use an ESM version of the @opentelemetry/instrumentation package (thinking of the approach from this comment here: #3989 (comment)), but CJS users may continue to use the CJS version.

The ESM instrumentation topics would persist because ESM customization hooks in node.js are still an ongoing topic. As a result breaking changes might happen.

True. Right now, I think the way to go would be to fix browser users.

From what I can tell, even with an ESM version double-published, using IITM right now would break when a

CJS library is used and
that library uses * re-exports (getExports used for node v20, v18.19.0 doesn't handle reexports nodejs/import-in-the-middle#29)

which may be more than a handful, not just @opentelemetry/instrumentation.

I've actually tried the changes from nodejs/import-in-the-middle#30 in the meantime, that'd fix the issue of handling re-exports.

dyladan · 2023-12-19T19:58:33Z

experimental/packages/opentelemetry-instrumentation/src/platform/node/index.ts

+export { InstrumentationNodeModuleFile } from './instrumentationNodeModuleFile';
+export { InstrumentationNodeModuleDefinition } from './instrumentationNodeModuleDefinition';


The platform folder should not have any exports for 1 platform but not another. All exports should be used exactly identically. The same code that uses the node platform exports will also use the browser platform exports.

dyladan · 2023-12-19T19:59:08Z

experimental/packages/opentelemetry-instrumentation/src/index.ts

+// Exports for Node.js and Browser are different, therefore exporting *
+export * from './platform/index';


This is not the way the platform-specific exports work and will cause things to break

Edit: some background info first: the problem is that we use path.normalize in InstrumentationNodeModuleDefinition, which does not exist in the browser.

@dyladan I see. We actually rely on this (different exports for different platforms) quite a bit in other packages in this repo (exporter packages, mostly), so we should likely also get on top of this there. It is also (in spirit) the way we had it before it was before we merged #4336.

From what I can tell there are only three other ways for us to fix this then:

provide and maintain our own normalize implementation

rely on a third-party package that provides an agnostic implementation

normalize in Node.js while using a no-op in the browser (this does not seem like a safe option)

WDYT?

Before #4336 we actually had the bad situation I'm trying to avoid and I had to fix it to get that PR to pass.

normalize in Node.js while using a no-op in the browser (this does not seem like a safe option)

Why do you think this is unsafe? There is no valid reason to use a NodeModuleFile in browser anyway and it would be completely broken in other ways. The only thing we need to fix is that we shouldn't import path in browser contexts.

Looking closer into this, I agree that it's likely fine if we do that (no-op in browser). Closing this PR, will re-open another one with that approach 👍

JamieDanielson · 2024-01-10T17:26:24Z

For reference, new PR is #4386

fix(instrumentation): do not export Node.js types for Browser users

b2da7f1

pichlermarc requested a review from a team December 18, 2023 13:21

fix(changelog): add changelog entry

d0d2c07

pichlermarc added bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc pkg:instrumentation labels Dec 18, 2023

fix(instrumentation): undo import change to test file

83ab4db

Merge branch 'main' into fix/instrumentation-node-types

acd8fd8

dyladan requested changes Dec 19, 2023

View reviewed changes

pichlermarc closed this Dec 20, 2023

pichlermarc mentioned this pull request Jan 10, 2024

chore: prepare release 1.20.0/0.47.0 #4410

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(instrumentation): do not export Node.js types for Browser users #4378

fix(instrumentation): do not export Node.js types for Browser users #4378

pichlermarc commented Dec 18, 2023

codecov bot commented Dec 18, 2023 •

edited

Loading

pichlermarc commented Dec 18, 2023

pichlermarc commented Dec 18, 2023

pichlermarc commented Dec 18, 2023

Flarna commented Dec 18, 2023

pichlermarc commented Dec 18, 2023

dyladan Dec 19, 2023

dyladan Dec 19, 2023

pichlermarc Dec 20, 2023 •

edited

Loading

dyladan Dec 20, 2023

pichlermarc Dec 20, 2023

JamieDanielson commented Jan 10, 2024

		export { InstrumentationNodeModuleFile } from './instrumentationNodeModuleFile';
		export { InstrumentationNodeModuleDefinition } from './instrumentationNodeModuleDefinition';

		// Exports for Node.js and Browser are different, therefore exporting *
		export * from './platform/index';

fix(instrumentation): do not export Node.js types for Browser users #4378

fix(instrumentation): do not export Node.js types for Browser users #4378

Conversation

pichlermarc commented Dec 18, 2023

Which problem is this PR solving?

Type of change

How Has This Been Tested?

codecov bot commented Dec 18, 2023 • edited Loading

Codecov Report

pichlermarc commented Dec 18, 2023

pichlermarc commented Dec 18, 2023

pichlermarc commented Dec 18, 2023

Flarna commented Dec 18, 2023

pichlermarc commented Dec 18, 2023

dyladan Dec 19, 2023

Choose a reason for hiding this comment

dyladan Dec 19, 2023

Choose a reason for hiding this comment

pichlermarc Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

dyladan Dec 20, 2023

Choose a reason for hiding this comment

pichlermarc Dec 20, 2023

Choose a reason for hiding this comment

JamieDanielson commented Jan 10, 2024

codecov bot commented Dec 18, 2023 •

edited

Loading

pichlermarc Dec 20, 2023 •

edited

Loading