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 central implementation "registry"/global tracer #15

Closed
Oberon00 opened this issue Jun 11, 2019 · 8 comments
Closed

Add central implementation "registry"/global tracer #15

Oberon00 opened this issue Jun 11, 2019 · 8 comments
Assignees
Labels
api Affects the API package.
Milestone

Comments

@Oberon00
Copy link
Member

Oberon00 commented Jun 11, 2019

As the discussion about this on #11 is getting quite long (#11 (comment)), I opened this issue.

Basically, opentelemetry-python should provide some API that decouples code that just wants to use instrumentation/tracing APIs from any particular tracer implementation. Without such an API, one is forced (or at least tempted) to write code like:

from opentelemetry.sdk.trace import tracer # *** Hard-coded dependency on implemenation

# Create a new root span, set it as the current span in context
with tracer.start_span("parent"):
    pass # Do work

I think it is clear that we want to avoid that, as it would force a whole dependency tree of an application and its libraries to coordinate on a particular implementation of the API, which kinda defeats the purpose of having a separate API layer.

OpenTracing handles this by providing a global tracer object: https://github.com/opentracing/opentracing-python/blob/master/opentracing/__init__.py
OpenTelemetry-java also has a global OpenTelemetry instance that, among other things, holds a reference to a global tracer: https://github.com/open-telemetry/opentelemetry-java/blob/fb0f5339cd1d665a98996806f45c60c65f47d7b6/api/src/main/java/io/opentelemetry/OpenTelemetry.java#L49-L58
There are other options, like having libraries themselves manage their "library-global" tracers. This was also discussed in a different thread #11 (comment).

@Oberon00
Copy link
Member Author

Actually, I just noticed that the spec covers this point and puts that central component in the API component: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/library-guidelines.md#language-library-generic-design

@carlosalberto
Copy link
Contributor

So in general it would be nice to have something along the lines of OpenTelemetry.getTracer() and related members.

Now, now - in Java this is straightforward to do, as you can load at startup the Tracer and other members, and they get registered at that time, but for Python we might need to let users register that manually.

Maybe something like:

import opentelemetry

opentelemetry.set_tracer(my_tracer)
...

opentelemetry.tracer().start_span("etc"):
   ...

A few questions would be:

  1. Do we need to have a single instance containing Tracer and the other members as Java does?
  2. Do we allow users to re-register Tracer and other members?

Let me know what you think ;)

@c24t
Copy link
Member

c24t commented Jun 19, 2019

All good points, and in any case the spec does suggest the real implementation should replace the no-op implementation that ships with the api package (although "exact substitution mechanism is language dependent" leaves some flexibility). My only complaint here is mostly aesthetic -- we're asking application code to import a full implementation from a package named "api".

Since we have the option of making the global tracer a module-level variable (or module-level getter/setters as in OpenTracing) we may want to use something like opentelemetry.trace instead of opentelemetry.api.trace and load the appropriate implementation at import time.

This issue seems like a good place to track the implementation of the actual mechanic that loads a specific implementation.

@c24t
Copy link
Member

c24t commented Jun 19, 2019

Do we need to have a single instance containing Tracer and the other members as Java does?

What about a single module instead of a singleton? opentelemetry.trace could hold Tracer, opentelemetry.metrics could hold Meter, etc.

Do we allow users to re-register Tracer and other members?

What's a good use case for replacing the global tracer at run time? Would it solve the problem if we made it easy to swap out the tracer implementation (but not the actual Tracer instance) at application start time instead?

@carlosalberto
Copy link
Contributor

carlosalberto commented Jun 19, 2019

What about a single module instead of a singleton? opentelemetry.trace could hold Tracer, opentelemetry.metrics could hold Meter, etc.

My personal taste would be to have all these in a single module (and not through a singleton value). Similarly to how io.opentelemetry.OpenTelemetry holds the three of them. I don't know whow friendly it is for users to be importing 3 modules when using, well, the 3 global objects ;)

(There's also the question of how many io.opentelemetry*trace* modules we want, aside the api and sdk, for a start ;) )

What's a good use case for replacing the global tracer at run time?

My feeling is that it's not needed the vast majority of time, but we need to add a note there.

@Oberon00
Copy link
Member Author

What's a good use case for replacing the global tracer at run time?

The only use I can think of for replacing the global tracer is a unit test. That is, if the selection mechanism for the initial tracer object is solid enough that it is not needed to overwrite the selected object right after initialization.

What about a single module instead of a singleton? opentelemetry.trace could hold Tracer, opentelemetry.metrics could hold Meter, etc.

So there is a module opentracing that has trace: Tracer and metrics: Meter as globals? Generally that sounds like the simplest approach to me, which is good. But I have a nitpick on the syntax: it seems OpenTracing-python started with a global and and later changed to using getters: opentracing/opentracing-python#95. I gather that the reason was that import tracer from opentracing would make a copy of the variable that does not get updated when the global tracer changes (including changes from None to something). @carlosalberto, I notice that's your PR, so can you confirm?

I also wanted to initially object that a class/singleton would make it easier to swap out the whole set of meter, tracer, ... (that may need to be synchronized to the same vendor's implementation), but on second thought, it doesn't really make a difference if passing a module instead of a class-based object to some function that wants to be flexible about that, and its an edge case anyway.

@carlosalberto
Copy link
Contributor

The only use I can think of for replacing the global tracer is a unit test. That is, if the selection mechanism for the initial tracer object is solid enough that it is not needed to overwrite the selected object right after initialization.

Yeah, that's the only case I could think of as well.

I gather that the reason was that import tracer from opentracing would make a copy of the variable that does not get updated when the global tracer changes (including changes from None to something).

That's accurate ;)

@Oberon00 Oberon00 self-assigned this Jun 20, 2019
@Oberon00 Oberon00 added the api Affects the API package. label Jun 25, 2019
@c24t c24t added this to the Tracing API milestone Jun 27, 2019
Oberon00 added a commit that referenced this issue Jul 10, 2019
* Global tracer registry: First somewhat satisfying iteration.

* Add API docs for backend module

* Fix documentation (and overlong lines within).

* Sort imports.

* Remove _UNIT_TEST_IGNORE_ENV.

We should be able to ensure clean envvars for the test run.

* Misc cleanup.

* Fix backend.py not being included in wheel.

* Rewrite backend/loader.

* Rewrite backend/loader more, fix pylint, mypy.

* Ditch 'default' in `_load_default_impl`, it's just wrong now.

* Fix docs.

* Apply new package structure.

* Remove unit tests (for now).

* Document the factory type aliases.

* Store factory functions in respective modules.

Gets rid of the dictionary in loader.py.

* Fix factory return type (Optional) & improve docs.

* Revert accidental changes to setup.py.

* Remove unused global.
@Oberon00
Copy link
Member Author

See #44, #45.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the API package.
Projects
None yet
Development

No branches or pull requests

3 participants