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

TC Review Request: Go Metric API #1355

Closed
MrAlias opened this issue Feb 2, 2023 · 7 comments
Closed

TC Review Request: Go Metric API #1355

MrAlias opened this issue Feb 2, 2023 · 7 comments
Assignees

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Feb 2, 2023

The Go SIG is looking to make a stable release of the OpenTelemetry metric API in Go. We are looking for a TC member familiar with the metric API specification to review our offering and help ensure our compliance with the specification.

We are still working to ensure the compliance of the metric SDK. Therefore, this request is only scoped to the API.

Prior work

We have conducted our own internal review of the specification here. That review identified one outstanding compliance issue remains:

However, we believe this to be an specification issue. open-telemetry/opentelemetry-specification#3171 is opened to address this at the specification level.

Inventory

We would like to evaluate the v1.12.0/v0.35.0 Release, links should be related to this tag.

@reyang
Copy link
Member

reyang commented Feb 6, 2023

I can probably take it if it's okay to wait till mid Feb (13th-17th), please let me know if it is okay.

@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 6, 2023

I can probably take it if it's okay to wait till mid Feb (13th-17th), please let me know if it is okay.

Yeah, that timeline works. Thanks 👍

@reyang reyang self-assigned this Feb 8, 2023
@reyang
Copy link
Member

reyang commented Feb 15, 2023

I've scanned through all the links provided in the PR description and confirmed that they are compliant to the API specification.
I've also confirmed that the API spec has been well covered by the OTel Go API package, except for the following section (which is okay since we're targeting 1.12):

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#get-a-meter

[since 1.13.0] attributes: Specifies the instrumentation scope attributes to associate with emitted telemetry.

Users can provide attributes to associate with the instrumentation scope, but it is up to their discretion. Therefore, this API MUST be structured to accept a variable number of attributes, including none.

One minor thing I've noticed:

https://github.com/open-telemetry/opentelemetry-go/blob/6cb5718eaaed5c408c3bf4ad1aecee5c20ccdaa9/trace/trace.go#L550 we seem to use name as the parameter for a provider to get a Tracer, https://github.com/open-telemetry/opentelemetry-go/blob/6cb5718eaaed5c408c3bf4ad1aecee5c20ccdaa9/metric/meter.go#L34 we use instrumentationName which is not very consistent with trace as well as the spec https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#get-a-meter.

@reyang
Copy link
Member

reyang commented Feb 15, 2023

@MrAlias I'm closing this issue as completed.

@reyang reyang closed this as completed Feb 15, 2023
@jmacd
Copy link
Contributor

jmacd commented Feb 15, 2023

I completely support the Go metrics API.
For the record, I maintain an alternate Go metrics SDK and have completed upgrading my code to use the new APIs. I found no difficulties and am pleased with the new APIs. lightstep/otel-launcher-go#381

@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 16, 2023

One minor thing I've noticed:

https://github.com/open-telemetry/opentelemetry-go/blob/6cb5718eaaed5c408c3bf4ad1aecee5c20ccdaa9/trace/trace.go#L550 we seem to use name as the parameter for a provider to get a Tracer, https://github.com/open-telemetry/opentelemetry-go/blob/6cb5718eaaed5c408c3bf4ad1aecee5c20ccdaa9/metric/meter.go#L34 we use instrumentationName which is not very consistent with trace as well as the spec https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#get-a-meter.

Addressed by open-telemetry/opentelemetry-go#3734

@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 16, 2023

I've scanned through all the links provided in the PR description and confirmed that they are compliant to the API specification. I've also confirmed that the API spec has been well covered by the OTel Go API package, except for the following section (which is okay since we're targeting 1.12):

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#get-a-meter

[since 1.13.0] attributes: Specifies the instrumentation scope attributes to associate with emitted telemetry.
Users can provide attributes to associate with the instrumentation scope, but it is up to their discretion. Therefore, this API MUST be structured to accept a variable number of attributes, including none.

Addressing this with open-telemetry/opentelemetry-go#3738

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants