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

Multiple Context objects vs using named context #126

Closed
toumorokoshi opened this issue Sep 6, 2019 · 7 comments
Closed

Multiple Context objects vs using named context #126

toumorokoshi opened this issue Sep 6, 2019 · 7 comments
Labels
api Affects the API package.

Comments

@toumorokoshi
Copy link
Member

I was working through #89, which enabled formatters to directly read and write to context objects. One aspect there is that this requires a standard way to set and get context values within a specific namespace.

There are two examples which use that today: DistributedContextManager and Tracer. Both use the Context object under the hood, and allow the passing of a name to access the specific context.

If a user wants to pair the DistributedContext and Tracer, that would require them to create one of each with the same name. This pattern will have to be applied to every unit test that needs to work with one of these contexts.

Instead, I propose the creation of multiple Context objects. Tracers and DistributedContextManager could consume a context object. This simplifies unit testing and also allows one to have a complete context object that could be passed into propagators.

@toumorokoshi
Copy link
Member Author

@a-feld @reyang wanted to get your thoughts on this one.

@reyang
Copy link
Member

reyang commented Sep 6, 2019

I guess Context.with_current_context has already provided the functionality you're looking for?

It'll be helpful to have a concrete code example to explain why we want to do this.

@a-feld
Copy link
Member

a-feld commented Sep 6, 2019

This probably boils down to having a discussion about being more explicit with what is defined in a context within open telemetry. For instance, I would imagine a context could contain both the current default tracer and the current distributed context. We could define interfaces like opentelemetry.get_current_tracer() that retrieve the tracer from the context. Similarly opentelemetry.get_current_span_context() and opentelemetry.get_current_distributed_context().

If a user wants to pair the DistributedContext and Tracer, that would require them to create one of each with the same name.

I still think there may be a misunderstanding about this. As far as I can tell, DistributedContext is not scoped to a tracer; rather it is part of a context.

DistributedContext will be referenced in metrics so accessing the current DistributedContext shouldn't go through a tracer.

Instead, I propose the creation of multiple Context objects. Tracers and DistributedContextManager could consume a context object.

Shouldn't this be the other way around? There should be 1 context that contains the current DistributedContext and the current tracer (I know this isn't exactly how things are currently defined)?

@toumorokoshi
Copy link
Member Author

I agree some global context container with clear methods is the right approach. See: #123 for a proposal around that.

My personal opinion is a global object that we can instantiate multiple of as needed. for example:

opentelemetry.GLOBAL.get_current_tracer()

# opentelemetry/__init__.py
GLOBAL = create_global()

I think regarding the scope of DistributedContext and SpanContext: it's true they are separate, but I think there is some implicit common scoping by virtue of requests being generally the broadest scope.

I think to refine this I want to start a PR around how OTEL will actually be used and integrated with apps. I think I'd like to leave this issue alone until then.

@Oberon00
Copy link
Member

Oberon00 commented Sep 9, 2019

context values within a specific namespace

If a user wants to pair the DistributedContext and Tracer, that would require them to create one of each with the same name.

What's that about naming? Where does it come from? Do you think about open-telemetry/oteps#16? The intent of that PR is (or was at least when I last read it) to allow disabling of individual instrumentations by tracer name. To support this use case, tracers with different names would need to share the same context. E.g., one example there is a tracer named "io.opentelemetry.contrib.mongodb". Of course you'd want a mongodb span to adopt the web request span from the "io.opentelemetry.ext.wsgi.flask" tracer as parent when specifying CURRENT_SPAN.

@Oberon00
Copy link
Member

Oberon00 commented Sep 9, 2019

I'm fine with supporting only one current context, at least initially. I don't see any relevant use cases that would use multiple current contexts, and I think that support would make specification, configuration and implementation of OpenTelemetry significantly more complex.

Just to highlight one point where this would increase implementation complexity: Python contextvars doc says:

Important: Context Variables should be created at the top module level and never in closures. Context objects hold strong references to context variables which prevents context variables from being properly garbage collected.

@Oberon00 Oberon00 added the api Affects the API package. label Sep 24, 2019
@toumorokoshi
Copy link
Member Author

toumorokoshi commented Feb 17, 2020

I believe OTEL66 (Context API) resolves this issue, by defining a specification for context variables.

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this issue Nov 1, 2020
* Add Plugin Loader

* Load simple-module

* gts fix

* Use type variable T instead of any

* Log instead of throwing and remove redundant Plugins suffix

* Add package specific .gitignore

* Add @todo

* Change DEFAULT_PLUGIN_PACKAGE_NAME_PREFIX to plugin

* fix: build pipeline

* fix: remove test functions out of the class

* fix: use private members in test without exposing it

* feat: add PluginConfig during load

* fix: make comment more descriptive

* fix: move the any to the Plugin type

* fix: remove constants container
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

4 participants