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

Merging stable API packages #3368

Closed
1 task done
legendecas opened this issue Oct 31, 2022 · 6 comments · Fixed by #3374
Closed
1 task done

Merging stable API packages #3368

legendecas opened this issue Oct 31, 2022 · 6 comments · Fixed by #3374

Comments

@legendecas
Copy link
Member

legendecas commented Oct 31, 2022

  • This only affects the JavaScript OpenTelemetry library

Multiple stable API packages for each signal can create confusion for end-user and maintenance burdens for duplications. I can see one of the main concerns of merging the API packages, specifically @opentelemetry/api and @opentelemetry/api-metrics, is that unused signals can burden the final bundle size.

I verified tree-shaking on the API packages and found that we can actually drop the unused API package code pieces in the bundle with common tools, webpack, and rollup: https://github.com/legendecas/tree-shaking-otel-api

I talked to other maintainers and many of us agreed that merging the API packages can alleviate the cognitive load on end-users, but we'd also like to address concerns about having a unified API package.

/cc @open-telemetry/javascript-approvers your opinions and insights on this idea are much appreciated!

@dyladan
Copy link
Member

dyladan commented Oct 31, 2022

The @open-telemetry/javascript-maintainers discussed this in slack and it was generally agreed that the DX of a single API package is significantly better and less confusing than multiple API packages. If the tree shaking is effective, I don't see a strong advantage of separate packages.

We agreed to create this issue in order to gather other opinions as we don't want to make such a change without at least considering everyone's opinion.

/cc @MSNev as you have had the opposite opinion in the past. is the tree shaking argument sufficient for you?

@MSNev
Copy link
Contributor

MSNev commented Oct 31, 2022

I am against combining (and linking) all of the logs / metrics (and eventually logs) into a single combined common namespace for the api as legendecas calls out in their tree-shaking-otel-api test

/**
 * If this line is uncommented, all apis (including DiagAPI, TraceAPI,
 * PropagationAPI) are included in the final bundle.
 */
// import { context } from '@opentelemetry/api';

That is because if everything is linked to the same namespace tree-shaking cannot safely determine what can be dropped.

This does not preclude "including" metrics within the api package -- just as long as it has its' own namespace eg something link api-metrics and it's NOT linked or used by anything by the default api namespace.
So api-metrics can reference and use anything in api but api cannot reference or use anything in api-metrics

This is one of the things I wanted to have done already (via the sandbox) where the plan (still) is to have the main branch

  • create browser "bundles" (I was using rollup)
  • run browser and worker tests for all targeted packages

That way we would have somewhere to have solid numbers on tests which would break on any size regressions (based on the size of the generated browser bundles.

So....
If you are just going to "include" the api-metrics namespace in the "@opentelemetry/api" npm package then I don't have any direct issue as lon as it conforms to the above ( 'api' does not import api-metrics) and all metrics are in their own separate namsepace.

@dyladan
Copy link
Member

dyladan commented Oct 31, 2022

So api-metrics can reference and use anything in api but api cannot reference or use anything in api-metrics

Some version of this rule is required no matter what to avoid circular dependencies.

If you are just going to "include" the api-metrics namespace in the "@opentelemetry/api" npm package then I don't have any direct issue as lon as it conforms to the above ( 'api' does not import api-metrics) and all metrics are in their own separate namsepace.

If I am understanding this right, you think this would be ok:

import { metrics, trace } from '@opentelemetry/api';

const meter = metrics.getMeter(name, version);
const tracer = trace.getTracer(name, version);

As long as metrics never references trace (and likely vice versa is also never going to happen) and the patch suggested by @legendecas is applied.

@MSNev
Copy link
Contributor

MSNev commented Oct 31, 2022

Yes, it should be

@dyladan
Copy link
Member

dyladan commented Nov 1, 2022

#3329 did not mark the API packages. I believe they should also be safe to mark because simply loading the files does not cause any global state change. A function must be called in order to modify global state.

@legendecas legendecas self-assigned this Nov 1, 2022
@legendecas
Copy link
Member Author

I'm going to merge the two api packages. I'll try to find a way to add a regression test to guarantee that using a single signal should not include the other signals in the final bundle.

I can mark the API package as side-effect-free too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants