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

Verify compliant metric API specification implementation: Instrument Asynchronous UpDownCounter #3379

Closed
2 tasks done
Tracked by #3383
MrAlias opened this issue Oct 20, 2022 · 10 comments
Closed
2 tasks done
Tracked by #3383
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics pkg:API Related to an API package
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Oct 20, 2022

  • Identify all the normative requirements, recommendations, and options the specification defines as comments to this issue
  • Ensure the current metric API implementation is compliant with these normative requirements, recommendations, and options in those comments.
@MrAlias MrAlias added area:metrics Part of OpenTelemetry Metrics pkg:API Related to an API package labels Oct 20, 2022
@jmacd
Copy link
Contributor

jmacd commented Oct 25, 2022

See the discussion in #3278.

I'm concerned that the fix in #3350 will break when filters are applied. In the comment here (#3278 (comment)), there is a suggestion not to set() the aggregator to the last value, but to initialize the output aggregator to zero and then add() the result from each attribute set. That way when filters are applied, you get the sum not the last value.

I picked up on this because #3350 (review) should not be true -- that is counters should not be behaving exactly like gauges.

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 25, 2022

See the discussion in #3278.

I'm concerned that the fix in #3350 will break when filters are applied. In the comment here (#3278 (comment)), there is a suggestion not to set() the aggregator to the last value, but to initialize the output aggregator to zero and then add() the result from each attribute set. That way when filters are applied, you get the sum not the last value.

I picked up on this because #3350 (review) should not be true -- that is counters should not be behaving exactly like gauges.

This issues is about the compliance of the metric API, go.opentelemetry.io/ote/metric. This comment seems to be about the compliance of the SDK. Is there more to it? Is this meant to say our API implementation needs to be addressed?

@jmacd
Copy link
Contributor

jmacd commented Oct 25, 2022

Sorry, I thought this was about end-to-end compliance testing. I'm not sure whether a new issue is needed for the comment in #3379 (comment), seems to fall in with #3278.

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 8, 2022

#3453

@MrAlias MrAlias self-assigned this Jan 7, 2023
@MrAlias MrAlias moved this from Todo to In Progress in Go: Metric API (GA) Jan 7, 2023
@MrAlias
Copy link
Contributor Author

MrAlias commented Jan 7, 2023

There MUST NOT be any API for creating an Asynchronous UpDownCounter other than with a Meter.

With #3454 now complete, our implementation is compliant.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jan 7, 2023

This MAY be called CreateObservableUpDownCounter. If strong type is desired, OpenTelemetry API authors MAY decide the language idiomatic name(s), for example CreateUInt64ObservableUpDownCounter, CreateDoubleObservableUpDownCounter, CreateObservableUpDownCounter<UInt64>, CreateObservableUpDownCounter<double>.

We do not follow this optional criteria. We instead follow more common Go method naming and use Int64ObservableUpDownCounter and Float64ObservableUpDownCounter method names.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jan 7, 2023

It is highly recommended that implementations use the name ObservableUpDownCounter (or any language idiomatic variation, e.g. observable_updowncounter) unless there is a strong reason not to do so.

I'm going to assume the authors intended "recommended" to be the normative key word "RECOMMENDED".

We do not follow this recommendation. #3453 is tracking this issue, and #3575 should address this.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jan 7, 2023

For callback functions registered after an asynchronous instrument is created, the API is required to support a mechanism for unregistration.

I'm going to assume the authors intended "recommended" to be the normative key word "RECOMMENDED".

We are compliant with this, as explained here: #3373 (comment)

@MrAlias
Copy link
Contributor Author

MrAlias commented Jan 7, 2023

@MrAlias MrAlias moved this from In Progress to Blocked in Go: Metric API (GA) Jan 7, 2023
@MrAlias
Copy link
Contributor Author

MrAlias commented Jan 12, 2023

Every normative clause from the Asynchronous UpDownCounter section of the v1.16.0 specification has been identified and our API has been validated that it complies. Closing.

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:API Related to an API package
Projects
No open projects
Status: Done
Development

No branches or pull requests

3 participants