open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client
https://opentelemetry.io
Apache License 2.0
2.71k stars 789 forks source link

Rename `@opentelemetry/sdk-metrics-base` to `@opentelemetry/sdk-metrics`? #3137

Closed dyladan closed 2 years ago

dyladan commented 2 years ago

I wanted to get the opinions of others, particularly @open-telemetry/javascript-maintainers and @open-telemetry/javascript-approvers on renaming the metrics SDK. Right now we have base in the name mostly because the trace package had base in the name, but I think it actually doesn't really make sense because there is no metrics-web or metrics-node package. Users in both environments should just use the metrics-base package. I would suggest renaming it to @opentelemetry/sdk-metrics or even just @opentelemetry/metrics.

on a related note: I wonder if we should consider making an @opentelemetry/trace package which combines the web and node tracers as there are very few differences now.

vmarchaud commented 2 years ago

I'm okay to rename metrics since there is only one right now (and that it don't have much platform specific code) but i would prefer to keep tracing as it is (specially as we might accept more specific platform like deno or edge platforms like cloudflare)

legendecas commented 2 years ago

Sounds good to me to rename the metrics SDK.

I would be open to the opportunity to introduce @opentelemetry/sdk-traces. When people are using @opentelemetry/sdk-trace-node or @opentelemetry/sdk-trace-web, they still need to import a lot of stuff from @opentelemetry/sdk-trace-base. It is a bit annoying to name a bunch of dependencies just for one single purpose.

Flarna commented 2 years ago

I'm fine with rename metrics. I'm not really a fan of the term SDK in this context but I guess this is because the name was given by the spec.

Rename sdk-trace-base to sdk-trace sounds also ok. But maybe it would be better to just re-export everything from base in node/web to avoid that users have to tinker with two packages?

vmarchaud commented 2 years ago

Rename sdk-trace-base to sdk-trace sounds also ok. But maybe it would be better to just re-export everything from base in node/web to avoid that users have to tinker with two packages?

Would prefer to re-export everything than to rename

legendecas commented 2 years ago

I'm not really a fan of the term SDK in this context but I guess this is because the name was given by the spec.

¯\(ツ)https://github.com/open-telemetry/opentelemetry-js/issues/3140

dyladan commented 2 years ago

I'm okay to rename metrics since there is only one right now (and that it don't have much platform specific code) but i would prefer to keep tracing as it is (specially as we might accept more specific platform like deno or edge platforms like cloudflare)

What platform specific things would you expect to be in the tracing? Right now it's just weird plugin loading things (and I would ditch that if I could in favor of the api-only instrumentation package).

I'm not really a fan of the term SDK in this context but I guess this is because the name was given by the spec.

That's basically the reason we use that term.

Would prefer to re-export everything than to rename

And maintain 2 packages? Or deprecate the old one but it still works? What is the advantage of re-export over rename?

Flarna commented 2 years ago

Still keep three packages (node + web + common/base/shared/...). If someone uses only base then fine - it contains code working on all platforms and user has a single entry point.

If one uses sdk-trace-node they now end up in code like this:

const { NodeTracerProvider } = require('@opentelemetry/sdk-trace-node');
const { SimpleSpanProcessor } = require('@opentelemetry/sdk-trace-base');

and you have to mention both in package.json (otherwise a good linter will complain).

if we reexport everything from base in node you could simply write

const { NodeTracerProvider, SimpleSpanProcessor  } = require('@opentelemetry/sdk-trace-node');

and one needs to mention only sdk-trace-node in package.json.

helio-frota commented 2 years ago

I would be open to the opportunity to introduce @opentelemetry/sdk-traces. When people are using @opentelemetry/sdk-trace-node or @opentelemetry/sdk-trace-web, they still need to import a lot of stuff from @opentelemetry/sdk-trace-base. It is a bit annoying to name a bunch of dependencies just for one single purpose.

I have a question that sort of relates with that.

If we had to choose a dependency to represent OpenTelemetry tracing for Node.js it should be sdk-trace-base or sdk-trace-node?

I'm asking because we have sdk-trace-node as recommended component in https://github.com/nodeshift/nodejs-reference-architecture/blob/main/docs/operations/distributed-tracing.md#recommended-components, and like @legendecas wrote we still need to import things from sdk-trace-base...

dyladan commented 2 years ago

I would be open to the opportunity to introduce @opentelemetry/sdk-traces. When people are using @opentelemetry/sdk-trace-node or @opentelemetry/sdk-trace-web, they still need to import a lot of stuff from @opentelemetry/sdk-trace-base. It is a bit annoying to name a bunch of dependencies just for one single purpose.

I have a question that sort of relates with that.

If we had to choose a dependency to represent OpenTelemetry tracing for Node.js it should be sdk-trace-base or sdk-trace-node?

I'm asking because we have sdk-trace-node as recommended component in nodeshift/nodejs-reference-architecture@main/docs/operations/distributed-tracing.md#recommended-components, and like @legendecas wrote we still need to import things from sdk-trace-base...

Right now you need both. This issue was about metrics but has sort of evolved to be about tracing too and we're trying to solve that issue.

dyladan commented 2 years ago

Still keep three packages (node + web + common/base/shared/...). If someone uses only base then fine - it contains code working on all platforms and user has a single entry point.

If one uses sdk-trace-node they now end up in code like this:

const { NodeTracerProvider } = require('@opentelemetry/sdk-trace-node');
const { SimpleSpanProcessor } = require('@opentelemetry/sdk-trace-base');

and you have to mention both in package.json (otherwise a good linter will complain).

if we reexport everything from base in node you could simply write

const { NodeTracerProvider, SimpleSpanProcessor  } = require('@opentelemetry/sdk-trace-node');

and one needs to mention only sdk-trace-node in package.json.

Sorry if I wasn't clear. What I meant is what does the NodeTracerProvider do that is node specific (and same question for web)? I think the only thing is the plugin loading which is actually handled by a separate module now (@opentelemetry/instrumentation). I don't see any reason to use NodeTracerProvider over BasicTracerProvider.

Flarna commented 2 years ago

I don't see any reason to use NodeTracerProvider over BasicTracerProvider.

I think the only good reason to use it is it takes care of creating, enabling and installing AsyncLocalStorageContextManager or AsyncHooksContextManager. Additionally it adds support to configure B3/Jeager as propagtors via env. But honestly speaking I'm no a big fan of this as it pulls in dependencies effectively not used in the end (assuming here that none is using jaeger, B3 and W3C propagators at the same time).

WebTracerProvider doesn't add propagators but installs StackContextManager (if nothing else is configured).

dyladan commented 2 years ago

I don't see any reason to use NodeTracerProvider over BasicTracerProvider.

I think the only good reason to use it is it takes care of creating, enabling and installing AsyncLocalStorageContextManager or AsyncHooksContextManager. Additionally it adds support to configure B3/Jeager as propagtors via env. But honestly speaking I'm no a big fan of this as it pulls in dependencies effectively not used in the end (assuming here that none is using jaeger, B3 and W3C propagators at the same time).

WebTracerProvider doesn't add propagators but installs StackContextManager (if nothing else is configured).

TBH this seems like it would be a job better suited to the node-sdk package if it was more stable and complete. Context is also not just a tracing concern. You could have an application which uses other opentelemetry signals but not tracing and still want context propagation. IMO it is a mistake to tie these concerns together in the same package.