Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

Return value of setGlobalXXX doesn't match documentation #31

Closed
Flarna opened this issue Apr 6, 2021 · 2 comments · Fixed by #32
Closed

Return value of setGlobalXXX doesn't match documentation #31

Flarna opened this issue Apr 6, 2021 · 2 comments · Fixed by #32
Assignees

Comments

@Flarna
Copy link
Member

Flarna commented Apr 6, 2021

According to the documentation api.context.setGlobalContextManager() Returns the initialized context manager.

But actually it returns the ContextManager passed even if registerGlobal() detects that there is already a context manager installed and the passed ContextManager is not installed as global.

Similar api.propagation.setGlobalPropagator() returns the propagator passed instead that one installed global.

For api.trace.setGlobalTracerProvider() there is even a difference if installation happens as it always returns this._proxyTracerProvider.

I'm not sure what's correct here.

  • Returning the current installed global version will cause problems in case of version mismatch
  • Returning the passed in version may result in having two ContextManagers/TracerProviders/Propagators flying around

I think the best would be to return what one would get if they call api.trace.getTracerProvider() or api.context._getContextManager() (currently private) or api.propagation._getGlobalPropagator() (currently private). So either the global one (version matches) or Noop.

@dyladan
Copy link
Member

dyladan commented Apr 6, 2021

Is there a reason this function should return a context manager at all? (same with other setXXX functions)

We could return void or a boolean value which indicates if registration was successful.

@Flarna
Copy link
Member Author

Flarna commented Apr 6, 2021

Yes, a boolean sounds reasonable.

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

Successfully merging a pull request may close this issue.

2 participants