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

Enabling/Disabling of ScopeManager #758

Closed
1 of 2 tasks
Flarna opened this issue Feb 3, 2020 · 3 comments
Closed
1 of 2 tasks

Enabling/Disabling of ScopeManager #758

Flarna opened this issue Feb 3, 2020 · 3 comments

Comments

@Flarna
Copy link
Member

Flarna commented Feb 3, 2020

  • This only affects the JavaScript OpenTelemetry library
  • This may affect other libraries, but I would like to get opinions here first

Followup on #752 (comment)

Enabling of ScopeManger is done quite inconsistent:

  • NodeTracerRegistry enables it in case none is provided via config and it creates it's own AsyncHooksScopeManager
  • WebTracerRegistry enables it unconditionally
  • BasicTracerRegistry doesn't enable it

Not sure if this is specified anywhere but I think it should be consistent.

Most likely the best solution is that each TracerRegistry enables the ScopeManager (if present) and the managers must be able to handle multiple calls to enable().

I'm also not sure about disable. In general it's not good if e.g. instances of AsyncHooksScopeManager are leaking as GC will not disable the underlying async-hooks instance.

@Flarna Flarna changed the title Enabling/Dsiabling of ScopeManager Enabling/Disabling of ScopeManager Feb 3, 2020
@dyladan
Copy link
Member

dyladan commented Feb 3, 2020

I think the reason for this is that in Node, you usually always want the AsyncHooksScopeManager. In web, it is more difficult to automatically choose a scope manager for the user. Maybe @obecny can weigh in on if we should have a default for web?

WRT enabling them unconditionally, I think the TracerProvider constructor should enable whichever scope manager is configured. If the same scope manager is passed to multiple Tracer Providers, then yes this would mean the scope managers need to be able to handle the case where they are enabled multiple times. I think this is not too much of a burden for scope managers though.

I'm also not sure about disable. In general it's not good if e.g. instances of AsyncHooksScopeManager are leaking as GC will not disable the underlying async-hooks instance.

What do you mean by "not sure"? Are you not sure we should allow disabling them, or not sure that it is implemented in a correct way? Or both?

@obecny
Copy link
Member

obecny commented Feb 3, 2020

I think all is fine here. The BasicTracerRegistry is meant to be extended and then you decide about behaviour so you might not want to enable that by default. Then node.js by default enable if you don't provide anythign - again fine for me - if you provide some then you decide. And finally the web is enabling that always as in fact you want this to be working with minimum line of code too. Either using default or just Zone Scope.
Does such behaviour cause any problem / bug (in real scenario) so far ?

@Flarna
Copy link
Member Author

Flarna commented Feb 4, 2020

Maybe it's just a documentation topic to clearly write down which registry requires an enabled ScopeManager and which one enables it internally.

Regarding Bugs see #752. One problem there is that the scope manager has not been enabled.

What do you mean by "not sure"? Are you not sure we should allow disabling them, or not sure that it is implemented in a correct way? Or both?

I'm not sure who is responsible to disable them.

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

No branches or pull requests

3 participants