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

Ensure the advice API can be added to the metric SDK #4162

Closed
MrAlias opened this issue Jun 1, 2023 · 7 comments
Closed

Ensure the advice API can be added to the metric SDK #4162

MrAlias opened this issue Jun 1, 2023 · 7 comments
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Jun 1, 2023

Adding the explicit bucket histogram advice API is tracked here: #4003

In order to make a stable release of the metric SDK, it needs to be ensured that that API can be added in a backwards compatible manner. This issue tracks that verification.

Resolving this may result in a complete solution being submitted as a PR, but it could also be resolved with a clear description of how to add a compliant implementation in a backwards compatible manner.

@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Jun 1, 2023
@dashpole
Copy link
Contributor

I'm going to pick this up!

@dashpole dashpole self-assigned this Jul 18, 2023
@dashpole
Copy link
Contributor

dashpole commented Jul 19, 2023

Note on the spec: It is a bit odd, IMO, for advice to be identifying for instruments (see the instrument spec: https://github.com/open-telemetry/opentelemetry-specification/blob/4ac43da9d746fea42523187685f341321a1d6ee1/specification/metrics/api.md#instrument). Does that mean the same instrument could be created, but with different buckets?

@MrAlias
Copy link
Contributor Author

MrAlias commented Jul 19, 2023

Does that mean the same instrument could be created, but with different buckets?

Hmm, yeah, that seems like an incorrect design.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jul 19, 2023

Prototype: #4341

@MrAlias
Copy link
Contributor Author

MrAlias commented Jul 20, 2023

Alt prototype: #4340

@MrAlias
Copy link
Contributor Author

MrAlias commented Jul 20, 2023

Both prototypes have been reviewed to satisfy the issue.

@MrAlias MrAlias closed this as completed Jul 20, 2023
@dashpole
Copy link
Contributor

Other feedback for the spec: Advice seemed unneccessary. I can't see a world in which I would ever want to set histogram bounds on a non-histogram instrument. If someone is configuring the aggregation in a view (from a non-histogram instrument), they need to set the buckets anyways, so the advice can't be used.

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
Development

No branches or pull requests

2 participants