open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client
https://opentelemetry.io
Apache License 2.0
2.56k stars 742 forks source link

add bundler tests #4744

Open trentm opened 1 month ago

trentm commented 1 month ago

Issues come up frequently that are related to bundler usage -- at least with the node-related packages from this and the contrib repo. I think it would be helpful to have tests using recent versions of some bundlers (say: webpack, rollup, esbuild).

These tests would:

I'm not sure exactly where these tests would live. Perhaps in @opentelemetry/instrumentation? Perhaps in sdk-node?

AbhiPrasad commented 1 month ago

I think this should live in https://github.com/open-telemetry/opentelemetry-js/tree/main/integration-tests.

When testing bundling, I think it's also important to fully validate export conditions and built assets. In the Sentry repo we've had a lot of success with using Verdaccio to simulate a fake NPM registry, and then running an e2e test that downloads packages from the fake registry. Testing bundling on the fully built assets that show up in NPM (relying on package.json export conditions) is more robust in my eyes.

pichlermarc commented 1 month ago

perhaps effectively define what bundler usage is actually supported (e.g. require-in-the-middle isn't going to work in a bundler, at least not without some bundler plugin -- like the current open issue to add esbuild support), and

Agree. I think this can be a separate issue and something we should document in @opentelemetry/instrumentation. At the moment, what we actually support is effectively only webpack for use with web instrumentations. As you said, bundled Node.js apps won't be instrumented without some bundler plugin, so I think calling that out in the README.md would make sense.

provide a starting point for maintainers that may not be intimately familiar with the various bundlers to provide support on reported issues

I think this would provide a lot of benefit. I often struggle with that as I don't use them in my daily life. It's often assumed that the bundler use is trivial but there's often many options for a bundler + typescript configs that can vary a lot and lead to different outcomes. Having a base that we can use to narrow stuff down and for people to provide reproducers would be extremely helpful.


I think this should live in https://github.com/open-telemetry/opentelemetry-js/tree/main/integration-tests.

+1 for putting it into ./integration-tests

When testing bundling, I think it's also important to fully validate export conditions and built assets. In the Sentry repo we've had a lot of success with using Verdaccio to simulate a fake NPM registry, and then running an e2e test that downloads packages from the fake registry. Testing bundling on the fully built assets that show up in NPM (relying on package.json export conditions) is more robust in my eyes.

Yes, I agree. This would be a nice-to-have. We've had numerous problems in the past as some package was

AbhiPrasad commented 1 month ago

sweet - I'll take a look at this end of my week (thurs/friday) and open a POC PR to gather thoughts.

if anyone else wants to experiment before then though please feel free! You can ping me on slack if you have questions about this stuff, I consider myself slightly experienced with bundling 😄