opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

chore: mark packages as side effects free for tree shaking

Open ogxd opened this issue 3 years ago • 4 comments
trafficstars

Which problem is this PR solving?

Fixes #2855

For instance, when using the exporter-trace-otlp-http package for a simple create/send trace, having sideEffects set to false for all packages it depends on results in a 70kb smaller bundle (minified)

Short description of the changes

Marked all package as side-effect free with "sideEffects": false.
This is supposed to be safe:

We talked about this in the SIG meeting and agreed that this is safe for us. Even our modules which have side-effects do not execute those side-effects at load time and require a user to explicitly call a function. This will cause the static analysis of webpack to realize the module is used and not tree-shake it out. @dyladan

Type of change

Please delete options that are not relevant.

  • [x] New feature (non-breaking change which adds functionality)
  • [x] This change could benefit from a mention in the documentation

How Has This Been Tested?

Tested manually using samples and setting sideEffects: false manually in node_modules

Checklist:

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

ogxd avatar Jul 11 '22 20:07 ogxd

CLA Missing ID CLA Not Signed

I am going to mark this as a draft until the CLA is signed. Feel free to mark it as ready when the checks are passing and it is ready for review.

dyladan avatar Jul 11 '22 20:07 dyladan

Codecov Report

Merging #3087 (bccd2ad) into main (b4707e4) will decrease coverage by 0.41%. The diff coverage is n/a.

:exclamation: Current head bccd2ad differs from pull request most recent head 54f7240. Consider uploading reports for the commit 54f7240 to get more accurate results

@@            Coverage Diff             @@
##             main    #3087      +/-   ##
==========================================
- Coverage   93.08%   92.66%   -0.42%     
==========================================
  Files         188      173      -15     
  Lines        6261     5523     -738     
  Branches     1323     1175     -148     
==========================================
- Hits         5828     5118     -710     
+ Misses        433      405      -28     
Impacted Files Coverage Δ
packages/opentelemetry-resources/karma.worker.js 0.00% <0.00%> (-100.00%) :arrow_down:
...kages/opentelemetry-sdk-trace-base/karma.worker.js 0.00% <0.00%> (-100.00%) :arrow_down:
...lemetry-resources/src/detectors/BrowserDetector.ts 53.33% <0.00%> (-46.67%) :arrow_down:
...lemetry-resources/src/detectors/ProcessDetector.ts 95.45% <0.00%> (-4.55%) :arrow_down:
...entelemetry-sdk-trace-web/src/WebTracerProvider.ts
packages/opentelemetry-sdk-trace-web/src/utils.ts
...ation-xml-http-request/src/enums/AttributeNames.ts
...es/opentelemetry-context-zone-peer-dep/src/util.ts
...-sdk-trace-web/src/enums/PerformanceTimingNames.ts
...telemetry-sdk-trace-web/src/StackContextManager.ts
... and 9 more

codecov[bot] avatar Jul 11 '22 20:07 codecov[bot]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] avatar Sep 12 '22 06:09 github-actions[bot]

This PR was closed because it has been stale for 14 days with no activity.

github-actions[bot] avatar Oct 03 '22 06:10 github-actions[bot]

Any plan to include this in future releases ? The size of the library is a show stopper for us to adopt it.

msimone-human avatar Oct 12 '22 13:10 msimone-human

Yes this is planned. I would have accepted this PR had the CLA been signed. If you'd like to work on it, you can comment on #2855. We're aware that the bundle is prohibitively large for many web users. The early days (and honestly the current days) of this SDK were quite focused on NodeJS as most of the maintainers have significantly more node experience. Also, OTel in general (all languages) tends to be more server-side focused than client-side. We recognize that is a shortcoming and focus is shifting but progress is slow.

If you're interested, there is an effort by @MSNev to make the JS SDK more browser-friendly, which may even result in a browser-specific JS SDK. There is also a RUM working group which is working on additions/changes to the spec to make it more client friendly.

dyladan avatar Oct 12 '22 13:10 dyladan

@pkanal was faster on the mark to grab this :-)

msimone-human avatar Oct 12 '22 15:10 msimone-human