Skip to content
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: add ESM builds for packages used in browser #459

Merged
merged 2 commits into from
Apr 30, 2021

Conversation

t2t2
Copy link
Contributor

@t2t2 t2t2 commented Apr 28, 2021

Which problem is this PR solving?

Continuation of open-telemetry/opentelemetry-js-api#25 and open-telemetry/opentelemetry-js#2112:

While typescript uses ESM syntax, the code distributed on npm (build dir) has been transpiled to use commonjs require's for better compatibility. However this means that bundlers like webpack and rollup can't do tree shaking to remove code not imported by user, leaving behind unused dead code.

Related issues: open-telemetry/opentelemetry-js#1378, open-telemetry/opentelemetry-js#1253

Short description of the changes

Added config files for tsc to build esm versions into build/esm of each package that is used in browser (as determined by existence of test:browser script in it's package.json

Effect on bundle size

Test was done by doing npm pack on each opentelemetry package used in examples/web (including opentelemetry-js-api and packages in opentelemetry-js as esm builds for those haven't been released in npm yet) and then placed in the right folder inside examples/web/node_modules/@opentelemetry.
Devtool commented out in examples/web/webpack.config.js and built using npx webpack --mode production --profile --json > stats.json to generate stats file for better analysis.

Built asset With "module" removed from package.json for all otel packages (forcing cjs usage) With "module" removed from package.json for packages in this repository With "module" in package.json for all packages (esm)
document-load.js 201122 186096 172791
meta.js 214880 200023 184847
user-interaction.js 204373 189344 176591

Screenshot 2021-04-28 at 16 39 53

Comparison using webpack visualizer

@t2t2 t2t2 requested a review from a team April 28, 2021 13:49
@dyladan
Copy link
Member

dyladan commented Apr 28, 2021

Forgive me I'm less familiar with webpack, but what is meta.js? This seems like a decent size savings but honestly less than I would have guessed. I think the lack of effective tree shaking comes from the fact that instrumentation packages only really have a single entry point so almost all of the code in them should be used even after tree shaking.

@codecov
Copy link

codecov bot commented Apr 28, 2021

Codecov Report

Merging #459 (ae0a541) into main (45e8751) will not change coverage.
The diff coverage is n/a.

❗ Current head ae0a541 differs from pull request most recent head a213703. Consider uploading reports for the commit a213703 to get more accurate results

@@           Coverage Diff           @@
##             main     #459   +/-   ##
=======================================
  Coverage   95.24%   95.24%           
=======================================
  Files         133      133           
  Lines        8252     8252           
  Branches      810      810           
=======================================
  Hits         7860     7860           
  Misses        392      392           

@t2t2
Copy link
Contributor Author

t2t2 commented Apr 28, 2021

Forgive me I'm less familiar with webpack, but what is meta.js?

meta.js is the output file for examples/web/examples/meta/index.js, so it's just the built file name for one of the examples (using opentelemetry/auto-instrumentations-web).

I think the lack of effective tree shaking comes from the fact that instrumentation packages only really have a single entry point so almost all of the code in them should be used even after tree shaking.

Hmm, after doing more research it could be that more packages need to have a sideffects flag to indicate that the files don't include side effects (basically if all you did was import '@opentelemetry/core'; it shouldn't do anything and okay to be optimised away). Currently it is only on @opentelemetry/context-zone and @opentelemetry/context-zone-peer-dep

@opentelemetry/semantic-conventions is definitely the package that needs working tree shaking most - DbCassandraConsistencyLevelValues is likely not used in any web instrumentation so should be shaken away, but SemanticAttributes (where both web and node.js attributes are mixed together) might be too bold for tree shakers to prune down

@t2t2
Copy link
Contributor Author

t2t2 commented Apr 28, 2021

Added as a quick test "sideEffects": false to @opentelemetry/semantic-conventions, result:

built file esm esm + sideEffects: false on semcons
document-load.js 172791 171603
meta.js 184847 183660
user-interaction.js 176591 175402

But searching further:

  • I could still find in both built files snippets like t.OTHER_SQL="other_sql",t.MSSQL="mssql",t.MYSQL="mysql",t.ORACLE="oracle", (so src/resource/SemanticAttributes.ts DbSystemValues)
  • In neither of the built files I could find k8s.statefulset.uid (packages/opentelemetry-semantic-conventions/src/resource/ResourceAttributes.ts ResourceAttributes)

So seems like yes it's able to prune away whole files that aren't imported already (so splitting into more files would help), but also sideEffects: false gets more wins but not yet sure how

But all of this probably worth it's own issue

@dyladan
Copy link
Member

dyladan commented Apr 28, 2021

Splitting into more files might help but it also complicates the build chain. I'm not sure the code generator we use to generate the semantic conventions package even supports it

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, this may be a smaller improvement than the other repos but it seems to still be an improvement so i'll go ahead and approve here.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@obecny obecny merged commit cf178f5 into open-telemetry:main Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants