search

open-telemetry / opentelemetry-js

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

Proposal: Package Structure #6

Closed mayurkale22 closed 5 years ago

mayurkale22 commented 5 years ago

This is my initial proposal to kick of the discussion on npm packages for OpenTelemetry Node.

Any thoughts about this?

New update (29th May): Changes as compared to OC:

New Update (30th May)

SergeyKanzhelev commented 5 years ago

*opentelemetry/instrumentation-** mongodb, redis, ioredis, express, hapi etc.

There is a discussion on how to call it: https://github.com/open-telemetry/opentelemetry-specification/issues/30

draffensperger commented 5 years ago

What's the motivation for having a separate opentelemetry/transport-* package group rather than opentelemetry/instrumentation-[transport] like opentelementry/instrumentation-http, etc.? Are transports interestingly different because of the need to propagate context and so are a different group?

I'm fine with it, just wanted to clarify!

mayurkale22 commented 5 years ago

Are transports interestingly different because of the need to propagate context and so are a different group?

Absolutely. To me, transport sounds more familiar when it comes to HTTP/2 and GRPC protocols. Having said that, I am fine with re-using opentelemetry/instrumentation-[transport] name.

rochdev commented 5 years ago

opentelemetry or opentelemetry/api or opentelemetry/core

It's a bit unclear where the no-op implementations are stored. Should it be a different package or should it be part of this one?

My vote goes to core if it contains any behavior, or api if it's only interfaces.

opentelemetry/context-propagation or opentelemetry/context-cls

As discussed in https://github.com/opentracing/opentracing-javascript/issues/112#issuecomment-433982906, this package should be split into multiple different packages.

For example:

My vote goes to context-* for the various implementations.

opentelemetry/cls-tracer or opentelemetry/automatic-tracer

My vote definitely goes to automatic-tracer as cls would be an implementation detail (which would probably be incorrect too).

opentelemetry/propagation-*

Where would the base classes/interfaces go?

opentelemetry/instrumentation- / opentelemetry/transport-

As discussed above, separating these may require patching the same module multiple times.

In general, I think the structure looks good with a few changes. The most important part I think will be to ensure that each of them can be used on its own, and have many hooks to allow altering the behavior based on vendor needs. This is one area where both OpenCensus and OpenTracing were lacking, which I think is the main reason that prevented adoption.

rochdev commented 5 years ago

Since a lot of the packages above could be used for the browser as well, should the name of this repo be changed to something like opentelemetry-js?

rochdev commented 5 years ago

opentelemetry/context-propagation or opentelemetry/context-cls

Thinking about this a bit more, I think this could be externalized as an external project since context propagation is a generic cross-cutting concern. Maybe opencontext?

The benefits would be that users could use this to store anything, so they could use it as a replacement for continuation-local-storage and cls-hooked. The former is no longer supported with new versions of Node and the latter only supports specific versions and has some very difficult to fix issues. None of them support the browser.

I think providing a universal implementation could be beneficial outside the realm of OpenTelemetry.

draffensperger commented 5 years ago

I like the idea of calling this opentelemetry-js. It will require more complex CI checks since we would want to test out the browser packages in multiple browser and the Node packages on multiple Node versions, but it would enable us to keep everything in sync much better than separate repos.

mayurkale22 commented 5 years ago

opentelemetry or opentelemetry/api or opentelemetry/core

IMO this package will have api interfaces and no-op implementations. This might look like this,

opentelemetry/core
   |__ trace/
      |__ propagation/
      |__ samplers/
   |__ metrics/
      |__ stats/
   |__ tags/
      |__ propagation/
   |__ resources/
   |__ utils/

I am in favor of core, but I think other languages(Java, Python and Go) are going with api.

draffensperger commented 5 years ago

Would you expect the core implementation to be Node-specific or would it be shareable with the browser (similar to the OpenTracing tracer)?

The benefit of making it cross-platform would be code sharing, but then we need separate hooks for the platform specific parts. Alternatively, as long as the api keeps a clean set of types that core implements, we could have a core-web that is optimized for the web platform but implements the same interfaces, and then core could use Node-specific utilities as well.

It seems like with OpenTracing, the API package had shared code but was pretty lean and allowed for plugging it into either Node or web needs (see e.g. https://github.com/elastic/apm-agent-nodejs and https://github.com/elastic/apm-agent-rum-js both of which integrate with OpenTracing)

rochdev commented 5 years ago

I would try to keep as much code as possible shareable, and add more modules for platform specific code. Then some parent module can compose the correct modules together and register them with each other.

For example, say there is a web-tracer. It would use automatic-tracer and register context-zones on it as the context propagator.

I think this approach could allow more different combinations of modules, even unrelated to the platform. This is just a theory however, so I'd like to know if anyone has used this kind of approach for this use case specifically.

draffensperger commented 5 years ago

@crdgonzalezca is doing some experimental work to build a Zone-based tracer for the browser. You can check out some of his very early stage work here: https://github.com/census-instrumentation/opencensus-web/pull/87

Since we haven't built it yet, I'm not really sure what it will take to make Zone.js do what we want. I like the idea of sharing code between the browser and Node, but I think it will require some discipline to make sure Node-specific stuff doesn't creep in since likely the Node use case will be more fully built out sooner than the web-specific one.

I think we could mitigate this by setting up a web build of the intended-to-be-shared packages very early on with some simple integration test that exercises the webpack compile+build process and makes sure it can get spans correctly in the browser.

rochdev commented 5 years ago

@crdgonzalezca is doing some experimental work to build a Zone-based tracer for the browser.

There was also some work done for context propagation specifically in https://github.com/opentracing/opentracing-javascript/pull/113/files#diff-e02a2e69cef841e50802298da59382a3

I think we could mitigate this by setting up a web build of the intended-to-be-shared packages very early on with some simple integration test that exercises the webpack compile+build process and makes sure it can get spans correctly in the browser.

I definitely agree with that. Otherwise, the more we wait, the more difficult it will be to support the browser because of things we won't have thought about. In general I find that tests are a lot more difficult to share between Node and the browser than the actual code, but I'm not sure that it's necessary either.

mayurkale22 commented 5 years ago

As mentioned in the SIG meeting, these are the new package names I would like to advocate.

  1. opentelemetry/basic-tracer -> opentelemetry-basic-sdk By default basic-sdk contains support for all the telemetry signals i.e. tracing, metrics, logs etc. It is possible to have separate packages for each telemetry signal, but currently not covered under default SDKs(basic-sdk and node-sdk) implementation.

  2. opentelemetry/node-tracer -> opentelemetry-node-sdk This package should help users/vendors to load specific plugin(s). Mostly applicable for advanced usres.

  3. opentelemetry-node This package should load all the provided plugins, we expect most of the applications to use this package.

The rest should stay as it is : opentelemetry-types, opentelemetry-core, opentelemetry-exporter-, opentelemetry-plugin-.

/cc @OlivierAlbertini @vmarchaud @rochdev @hekike @draffensperger @bg451 @markwolff @danielkhan

draffensperger commented 5 years ago

What about having opentelemetry-basic-sdk just re-export the exported symbols from opentelemetry/basic-tracer (and perhaps in the future opentelemetry/basic-metrics and opentelemetry/basic-logs, etc.)?

That way if someone wants to use e.g. just traces or just metrics as a power user they still can

mayurkale22 commented 5 years ago

I'm going to close this issue via #408. Do add a comment if you think this is incorrect.