search

open-telemetry / opentelemetry-go

OpenTelemetry Go API and SDK
https://opentelemetry.io/docs/languages/go
Apache License 2.0
5.29k stars 1.08k forks source link

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

Closed MrAlias closed 1 year ago

MrAlias commented 1 year ago

Adding the explicit bucket histogram advice API is tracked here: https://github.com/open-telemetry/opentelemetry-go/issues/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.

dashpole commented 1 year ago

I'm going to pick this up!

dashpole commented 1 year ago

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 commented 1 year ago

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

Hmm, yeah, that seems like an incorrect design.

MrAlias commented 1 year ago

Prototype: https://github.com/open-telemetry/opentelemetry-go/pull/4341

MrAlias commented 1 year ago

Alt prototype: https://github.com/open-telemetry/opentelemetry-go/pull/4340

MrAlias commented 1 year ago

Both prototypes have been reviewed to satisfy the issue.

dashpole commented 1 year ago

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.