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

Proposal: Package Structure #6

Closed
mayurkale22 opened this issue May 29, 2019 · 15 comments
Closed

Proposal: Package Structure #6

mayurkale22 opened this issue May 29, 2019 · 15 comments
Labels
Discussion Issue or PR that needs/is extended discussion.
Milestone

Comments

@mayurkale22
Copy link
Member

mayurkale22 commented May 29, 2019

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

  • opentelemetry/types
    A separate package with only TS interfaces and enums. Originates from Request: separate package with only TS interfaces and enums #3

  • opentelemetry or opentelemetry/api or opentelemetry/core
    This is the core npm package for OpenTelemetry. It contains the public API which is used by the various plugins. Also contains interfaces for traces, metrics, tags, etc.
    Q: What do we want to name the new packages: opentelemetry or opentelemetry/api or opentelemetry/core?

  • opentelemetry/basic-tracer
    With this users will have a full control over instrumentation and span creation. The base package doesn't load Continuation Local Storage (CLS) or any instrumentation plugin by default. Originates from Exporting TracerBase as a separate @opencensus/nodejs-base package census-instrumentation/opencensus-node#495 (comment)

  • opentelemetry/context-propagation or opentelemetry/context-cls
    Separate package for CLS (Continuation Local Storage). The primary objective of CLS is to implement a transparent context API, that is, we don't need to pass around a ctx variable everywhere in application code.
    Q: What do we want to name the new packages: opentelemetry/context-propagation or opentelemetry/context-cls?

  • opentelemetry/cls-tracer or opentelemetry/automatic-tracer
    Automatic tracer with CLS: opentelemetry/basic-tracer wrapped with opentelemetry/context-propagation.
    Q: What do we want to name the new packages: opentelemetry/cls-tracer or opentelemetry/automatic-tracer?

  • opentelemetry/exporter-*
    Contains standard trace and stats exporters. For instance, Jaeger, Zipkin, Stackdriver, Instana, Prometheus, oc-agent, z-pages etc.

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

  • opentelemetry/transport-*
    gRPC, HTTP/2/s, kafka etc.

  • opentelemetry/propagation-*
    B3, BinaryFormat, TraceContext

  • opentelemetry/resource-util
    It contains a collection of utilities for auto detecting monitored resource when exporting stats, based on the environment where the application is running.

Any thoughts about this?


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

  • api and sdk (basic-tracer and automatic-tracer) are separate packages
  • stats has been merged into metrics

New Update (30th May)

  • opentelemetry/core (2 votes)
  • opentelemetry/automatic-tracer (2 votes)
@SergeyKanzhelev
Copy link
Member

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

There is a discussion on how to call it: open-telemetry/opentelemetry-specification#30

@mayurkale22 mayurkale22 added the Discussion Issue or PR that needs/is extended discussion. label May 29, 2019
@draffensperger
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member

rochdev commented May 30, 2019

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 opentracing/opentracing-javascript#112 (comment), this package should be split into multiple different packages.

For example:

  • opentelemetry/context-base: The base implementation that doesn't propagate the context but handles things like binding.
  • opentelemetry/context-async-listener: Implementation based on @datadog/async-listener for Node <8. If our fixes for promise binding + memory leak + interoperability can be merged back into async-listener then that should be used instead.
  • opentelemetry/context-async-hooks: Implementation based on async_hooks for Node >=8.
  • opentelemetry/context-zones: Implementation based on Zones for the browser.

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
Copy link
Member

rochdev commented May 30, 2019

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
Copy link
Member

rochdev commented May 30, 2019

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
Copy link
Contributor

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
Copy link
Member Author

mayurkale22 commented May 30, 2019

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
Copy link
Contributor

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
Copy link
Member

rochdev commented May 31, 2019

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
Copy link
Contributor

draffensperger commented Jun 3, 2019

@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: census-instrumentation/opencensus-web#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
Copy link
Member

rochdev commented Jun 3, 2019

@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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issue or PR that needs/is extended discussion.
Projects
None yet
Development

No branches or pull requests

4 participants