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

Add sideEffects: false to package.json for packages to enable tree shaking #2855

Closed
domasx2 opened this issue Mar 23, 2022 · 9 comments · Fixed by #3329
Closed

Add sideEffects: false to package.json for packages to enable tree shaking #2855

domasx2 opened this issue Mar 23, 2022 · 9 comments · Fixed by #3329
Assignees
Labels
enhancement New feature or request never-stale

Comments

@domasx2
Copy link
Contributor

domasx2 commented Mar 23, 2022

Apologies if this is not supposed to be a bug type, wasn't sure which issue type fits best!

What version of OpenTelemetry are you using?

1.1.1

What version of Node are you using?

16.14.2

Please provide the code you used to setup the OpenTelemetry SDK

import { registerInstrumentations } from '@opentelemetry/instrumentation';
//import { DocumentLoadInstrumentation } from '@opentelemetry/instrumentation-document-load';
import { ConsoleSpanExporter, SimpleSpanProcessor } from '@opentelemetry/sdk-trace-base';
import { WebTracerProvider } from '@opentelemetry/sdk-trace-web';

const provider = new WebTracerProvider();
provider.addSpanProcessor(new SimpleSpanProcessor(new ConsoleSpanExporter()));

registerInstrumentations({
  instrumentations: [/*new DocumentLoadInstrumentation()*/],
});

What did you do?

I'm PoCing using opentelemetry-js in a library aimed at web application. It's rather hefty and produces a large js bundle when compiled with parcel or webpack. It seems that tree-shaking does not work with it.

What did you expect to see?

Smaller bundle size :)

What did you see instead?

Huge bundle, all of the code is included, even what isn't used by the library.

Additional context

It seems most packages do not have "sideEffects" property set. Manually going to node_modules and adding "sideEffects": false to every @opentelemetry/* package reduced bundle size by ~40kb! Please consider adding this property to every package.

More about it: https://webpack.js.org/guides/tree-shaking/

@domasx2 domasx2 added the bug Something isn't working label Mar 23, 2022
@dyladan dyladan added enhancement New feature or request and removed bug Something isn't working labels Mar 23, 2022
@legendecas
Copy link
Member

legendecas commented Mar 25, 2022

I think the problem for us to adopt the sideEffects property to be shipped with package.json is that we need a reliable tool to verify our settings are correct -- we can not declare a side-effectful file as side-effect-free, that will break consumers. Hand-tailored list of side-effect-free files is highly-likely to be broken from day-to-day maintenance.

@github-actions
Copy link

This issue 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 github-actions bot added the stale label May 30, 2022
@dyladan
Copy link
Member

dyladan commented Jun 8, 2022

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 dyladan added up-for-grabs Good for taking. Extra help will be provided by maintainers never-stale and removed stale labels Jun 8, 2022
@ogxd
Copy link

ogxd commented Jul 8, 2022

That would be awesome! Currently open telemetry packages are a no go for my company because it more than double our bundle size just for the sake of being able to trace 😥

@dyladan
Copy link
Member

dyladan commented Jul 8, 2022

@ogxd if you have time a PR would be welcome

@ogxd
Copy link

ogxd commented Jul 11, 2022

I'll do that :)
By the way, I notice that exporter-trace-otlp-http references otl-exporter-base which references otlp-transformer which references sdk-metrics-base. This sdk-metrics-base represents 50% of the bundle size according to this, while exporter-trace-otlp-http is only about traces. It seems that this dependency was added in 0.29.
I'm wondering if the tree shaking is able to get rid of that, I still see some metrics related things in my production bundle (OTEL_EXPORTER_OTLP_METRICS_TIMEOUT, AWS_DYNAMODB_ITEM_COLLECTION_METRICS, ...)

@dyladan
Copy link
Member

dyladan commented Jul 11, 2022

I'm working on refactoring the otlp exporters right now. they're a mess

@pkanal
Copy link
Contributor

pkanal commented Oct 12, 2022

I would like to pick this up if it's up for grabs.

@pichlermarc pichlermarc removed the up-for-grabs Good for taking. Extra help will be provided by maintainers label Oct 12, 2022
@pichlermarc
Copy link
Member

It's yours 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request never-stale
Projects
None yet
6 participants