feat(opentelemetry-instrumentation-http, opentelemetry-instrumentation-grpc): generate esm build files too

[serkan-ozal]

Which problem is this PR solving?

As OTEL FAAS SIG, we are working on reducing OTEL Lambda Nodejs layer coldstart overhead and bundling has the biggest impact. But bundling tools like webpack, esbuild don't like CJS modules while tree-shaking, but ESM modules. So, in this PR, we are configuring build process to generate ESM build files for the opentelemetry-instrumentation-http and opentelemetry-instrumentation-grpc modules.

Short description of the changes

To be able to generate ESM build files for opentelemetry-instrumentation-http and opentelemetry-instrumentation-grpc modules too, this PR

• introduces tsconfig.esm.json and tsconfig.esnext.json files
• and updates scripts accordingly in the package.json
• and defines ESM file paths in the files section of the package.json for their discoverability

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

It is observed that ESM builds are generated under /build/esm folder for the opentelemetry-instrumentation-http and opentelemetry-instrumentation-grpc modules.

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.58%. Comparing base (fc0edd8) to head (00079b3).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5351   +/-   ##
=======================================
  Coverage   94.58%   94.58%           
=======================================
  Files         318      318           
  Lines        8069     8069           
  Branches     1701     1701           
=======================================
  Hits         7632     7632           
  Misses        437      437           

[pichlermarc]

Hi, thank you for your contribution. 🙂

A few (naive) questions:

• I'm not very familiar with Lambda beyond the absolute basics. @opentelemetry/instrumentation-grpc don't work when the end-user's app is bundled - what's the use-case for bundling the instrumentation? Is that something that happens when a lambda is invoked? 🤔
• are there any measurements we can look at to verify that this improves things? I suspect that there may not be any code that can be tree-shaken out with these two packages specifically.
• other than verifying that the build files are generated, did you also do manual tests to ensure that this does not yield any extra warnings/errors with bundling tools that may be used in this context? Many instrumentations are not written with bundling in mind and we definitely have some places in the repo where this has been a problem in the past (and it may still be a problem) - exposing an ESM build may surface issues that were previously hidden.

[serkan-ozal]

Hi @pichlermarc

I'm not very familiar with Lambda beyond the absolute basics. @opentelemetry/instrumentation-grpc don't work when the end-user's app is bundled - what's the use-case for bundling the instrumentation? Is that something that happens when a lambda is invoked?

I am not sure, for which case @opentelemetry/instrumentation-grpc instrumentation had been added to AWS Lambda layer (it was there when I had joined to the OTEL FAAS SIG), but it should be for the GRPC client instrumentation, not for GRPC server instrumentation (Lambda calls another API through GRPC client).

are there any measurements we can look at to verify that this improves things? I suspect that there may not be any code that can be tree-shaken out with these two packages specifically.

Yes, as you said, there are not many things in those modules to be tree-shaken out, but the real motivation to bundle these modules as ESM modules too is having proper tree-shaking for their transitive dependencies.

As one example, these modules (@opentelemetry/instrumentation-http and @opentelemetry/instrumentation-grpc packages) have @opentelemetry/semantic-conventions package dependency and when they are used as CJS module by the AWS Lambda layer, bundling tools like webpack and esbuild are not able to tree-shake @opentelemetry/semantic-conventions package, because they cut their optimization at the @opentelemetry/instrumentation-http and @opentelemetry/instrumentation-grpc package levels, so put all the things in the @opentelemetry/semantic-conventions package into the bundle.

other than verifying that the build files are generated, did you also do manual tests to ensure that this does not yield any extra warnings/errors with bundling tools that may be used in this context? Many instrumentations are not written with bundling in mind and we definitely have some places in the repo where this has been a problem in the past (and it may still be a problem) - exposing an ESM build may surface issues that were previously hidden.

I have tested to check whether instrumentation works when bundled with @opentelemetry/instrumentation-aws-lambda, @opentelemetry/instrumentation-aws-sdk and @opentelemetry/instrumentation-http packages, but I can also test with additional instrumentation cases if you describe.

cc @dyladan

[pichlermarc]

Lambda calls another API through GRPC client

That must be the reason then. If it imports @grpc/grpc-js and that code is not included in the bundle, then this will get instrumented.

have @opentelemetry/semantic-conventions package dependency and when they are used as CJS module by the AWS Lambda layer, bundling tools like webpack and esbuild are not able to tree-shake @opentelemetry/semantic-conventions

Ah, I see. I did not think of the transitive ones earlier. I can see CJS introducing a bunch of overhead in that case.

I have tested to check whether instrumentation works when bundled with @opentelemetry/instrumentation-aws-lambda, @opentelemetry/instrumentation-aws-sdk and @opentelemetry/instrumentation-http packages, but I can also test with additional instrumentation cases if you describe.

What I'm mainly concerned about is usage outside of Lambda. The way we export ESM, the ESM entrypoints are used by bundlers like rollup but not in Node.js when the app is non-bundled. In the past we've had a lot of issues that were concerning compatibility of the @opentelemetry/instrumentation package (and others) with bundlers. Since #4744 is not implemented yet, this has to be tested manually which is slow and painful.

As a long-term solution, and if we want to make changes like this for more packages in this repo (the same applies to contrib), then I personally see #4744 as a prerequisite. I think adding more ESM entrypoints on a large scale without proper tests in place will cost us a lot of time down the line. IMO bundler tests and some kind of automated bundle-size tests are what really hold us back in making meaningful changes in improving bundler support and bundle size.

For now I will accept this PR if proof of manual testing is provided (for instance via another repo that links to locally built packages, or branch on a fork of this repo where the manual test package is added in examples/)

If the plan is to do this for more than these two instrumentation then my recommendation would be to consider working on #4744 first, then adding some sort of automated bundle-size reporting. Doing that will make it immediately clear to reviewers that PRs that are opened:

• work/don't work with bundlers
• improve/worsen bundle size (if applicable)

With these two things in place we can improve quicker (shorter review cycle due to automated tests/reporting) on a larger scale (more packages) and it will help us maintain support long-term.

A side note: while I'm stating that this would speed up things, we're currently working on SDK 2.0 as our main focus, reviews may be slow during that time since this does not fall into our focus at the moment. We have identified bundle size improvements as a possible future focus topic. We may pick it if we're done with one of the current focus topics, at which point there will be less friction in getting changes like this in. (Ref #5149)

[serkan-ozal]

@pichlermarc Got it. @dyladan had mentioned some issues about the current approach and I just wanted to follow the same approach. So when the new approach is implemented, all the implementations of the current approach can be migrated all together to the new approach.

[serkan-ozal]

@pichlermarc BTW, I have also been thinking of sending another PR to js-contrib repo for the rest of the instrumentation packages :)

[serkan-ozal]

@pichlermarc and I am also aware of import-in-the-middle hook issue with bundling. I have some plans for it in the AWS Lambda side by using import-in-the-middle still from the bundle itself. But I guess, the basic approach would be keeping import-in-the-middle package and our hook.js as external.