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

Unify instrumentation *Provider options #213

Closed
3 tasks
MrAlias opened this issue Aug 13, 2020 · 8 comments · Fixed by #303
Closed
3 tasks

Unify instrumentation *Provider options #213

MrAlias opened this issue Aug 13, 2020 · 8 comments · Fixed by #303
Assignees
Labels
area: instrumentation Related to an instrumentation package help wanted Extra attention is needed
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Aug 13, 2020

Currently there are many instrumentation packages that are directly configurable with a Tracer or Meter to use in the telemetry they gather.

They should not be configurable to do this. It is the instrumentations job to define the Tracer or Meter they use from a *Provider.

The global package was made to provide this functionality. It provides a shim until an SDK is registered that will make sure all instrumentation use the correct *Providers. However, since these instrumentation packages are expected to be wrapped in other instrumentation that might want the functionality to use a unique *Provider and the possible aversion some uses will have with the idea of globals, the instrumentation needs to be configurable to accept *Providers instead of Tracers and Meters directly.

TODO

  • Remove all configuration for Tracer and Meter in the existing instrumentation.
  • Refactor instrumentation to accept a TraceProvider if it uses a Tracer, a MeterProvider if it uses a Meter, and Propagators if it handles any context propagation. The default for each of these needs to be the one provided by the global package.
  • Update the instrumentation guidelines to include this info.

Originally posted by @XSAM in #209 (comment)

@MrAlias MrAlias added help wanted Extra attention is needed area: instrumentation Related to an instrumentation package release:required-for-ga labels Aug 13, 2020
@MrAlias
Copy link
Contributor Author

MrAlias commented Aug 13, 2020

Optionally, the instrumentation could be configured to accept a *Provider instead that would override the global package. As well maybe the configuration of Propagators could remain as well.

These could be useful to an operator if they wanted to separate out functionality. Though, outside of the propagators, I'm not immediately sure why.

@MrAlias
Copy link
Contributor Author

MrAlias commented Aug 13, 2020

From the SIG meeting today, instrumentation in this repo ...

  1. MUST provide an option to accept a TraceProvider if it uses a Tracer, a MeterProvider if it uses a Meter, and Propagators if it handles any context propagation
  2. MUST use the default TraceProvider, MeterProvider, and Propagators supplied by the global package if no optional one is provided.
  3. MUST not provide an option to accept a Tracer or Meter.
  4. MUST create any used Tracer or Meter with a name matching the instrumentation package name.

@MrAlias MrAlias changed the title Update instrumentation to use global package Unify instrumentation *Provider options Aug 13, 2020
@XSAM
Copy link
Member

XSAM commented Aug 15, 2020

I'd like to take this up.

@reggiemcdonald
Copy link
Contributor

@XSAM yesterday I started a fix for the gocql instrumentation. Did you mind skipping that one?

@XSAM
Copy link
Member

XSAM commented Aug 16, 2020

@reggiemcdonald Not at all, I will skip that part.

@XSAM
Copy link
Member

XSAM commented Aug 21, 2020

@MrAlias

Refactor instrumentation to accept a TraceProvider if it uses a Tracer, a MeterProvider if it uses a Meter, and Propagators if it handles any context propagation. The default for each of these needs to be the one provided by the global package.

Maybe we can use TracerProvider instead of TraceProvider, because it brings more consistency for naming since the other provider is called MeterProvider.

@MrAlias
Copy link
Contributor Author

MrAlias commented Aug 21, 2020

@MrAlias

Refactor instrumentation to accept a TraceProvider if it uses a Tracer, a MeterProvider if it uses a Meter, and Propagators if it handles any context propagation. The default for each of these needs to be the one provided by the global package.

Maybe we can use TracerProvider instead of TraceProvider, because it brings more consistency for naming since the other provider is called MeterProvider.

Yes, wow, that is indeed the correct approach. That was definitely a mistake on my part. 😥

Looks like this mistake was propagated to implementation already. @XSAM if you want to update the existing options that would be awesome, but I feel responsible for this mistake and happy to clean it up myself. Let me know what you think.

@XSAM
Copy link
Member

XSAM commented Aug 22, 2020

if you want to update the existing options that would be awesome

No problem, I would like to update the existing options. It looks like a global replacement command could address that typo. :)

@MrAlias MrAlias added this to the RC1 milestone Sep 3, 2020
plantfansam referenced this issue in plantfansam/opentelemetry-go-contrib Mar 18, 2022
* add RegisterSimpleSpanProcessor() modeled on stackdriver code

* update examples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: instrumentation Related to an instrumentation package help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants