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

Align view configuration with specification #3241

Closed
MrAlias opened this issue Sep 28, 2022 · 4 comments · Fixed by #3387
Closed

Align view configuration with specification #3241

MrAlias opened this issue Sep 28, 2022 · 4 comments · Fixed by #3387
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Sep 28, 2022

Currently, the specification defines view configuration to be associated with a MeterProvider:

The SDK MUST provide the means to register Views with a MeterProvider

Furthermore, it implies this registration is a one-to-one mapping between views and MeterProvider:

If the MeterProvider has one or more View(s) registered

Our current implementation registers views in conjunction with a Reader. There is a one-to-one mapping of views to Reader. There can be multiple of these pairs registered with a MeterProvider.

This issue is to serve as block to releasing the metric SDK as stable until the specification is updated (see open-telemetry/opentelemetry-specification#2288) or we change how we register views.

@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made labels Sep 28, 2022
@MrAlias MrAlias added this to the Metric SDK: Beta milestone Sep 28, 2022
@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 28, 2022

The current proposal in #3210 is to move the view registration into the Reader directly. If the resolution to this issue to modify the specification, that modification should take that approach into consideration.

@MadVikingGod
Copy link
Contributor

This was decided to take this form for two reasons:

  1. This allows for independent views per reader. They can be reused, because they don't have state, but if I need to rename something when exporting to prometheus but not to otlp that is possible in this configuration.
  2. We don't control the constructor for ALL Readers, just ManualReader, PeriodicReader, and now Prometheus. We can't enforce that Readers will take views into account.

We do comply with the wording of the spec, Views are "registered" with a MeterProvider via only an Option, something the MeterProvider takes and uses.

Furthermore, this was the intent of views, as expressed in spec meetings on this subject.

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 12, 2022

Furthermore, this was the intent of views, as expressed in spec meetings on this subject.

This sounds good 👍, we just need to ensure the specification is updated to reflect this intention before we can release a stable package.

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 19, 2022

It doesn't seem like open-telemetry/opentelemetry-specification#2288 is going to be resolved any time soon.

I think we need to move this issue to Todo and work to refactor our implantation to conform with the specification so we do not block our v1 release.

@MrAlias MrAlias removed the blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made label Oct 19, 2022
@MrAlias MrAlias removed this from the Metric SDK: Beta milestone Oct 20, 2022
@MrAlias MrAlias self-assigned this Oct 21, 2022
@MrAlias MrAlias moved this from Todo to In Progress in Go: Metric SDK (Beta) Oct 21, 2022
@MrAlias MrAlias added this to the Metric v0.34.0 milestone Oct 27, 2022
Repository owner moved this from In Progress to Done in Go: Metric SDK (Beta) Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants