opentelemetry-js
opentelemetry-js copied to clipboard
chore: mark packages as side effects free for tree shaking
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
- :x: The commit (54f7240b6c5b532737b4778c4586b6c8f9460bb7). This user is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.
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.
Codecov Report
Merging #3087 (bccd2ad) into main (b4707e4) will decrease coverage by
0.41%. The diff coverage isn/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 |
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.
This PR was closed because it has been stale for 14 days with no activity.
Any plan to include this in future releases ? The size of the library is a show stopper for us to adopt it.
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.
@pkanal was faster on the mark to grab this :-)