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.

Sign up for GitHub

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

Jump to bottom

metric identity and aggregation for duplicate instrument registration #3835

Closed
srikanthccv opened this issue Mar 4, 2023 · 5 comments · Fixed by #4338 or #4428
Closed

metric identity and aggregation for duplicate instrument registration #3835

srikanthccv opened this issue Mar 4, 2023 · 5 comments · Fixed by #4338 or #4428
Assignees
@MrAlias
Labels
area:metrics Part of OpenTelemetry Metrics bug Something isn't working pkg:SDK Related to an SDK package
Milestone
v1.17.0/v0.40.0

Comments

@srikanthccv
Copy link
Member

Description

https://github.com/open-telemetry/opentelemetry-specification/blob/ec84e5dcd77f36e79c0fe9bb1444d62be79dbb38/specification/metrics/sdk.md#duplicate-instrument-registration

Based on the recommendations from the data model, the SDK MUST aggregate data from identical Instruments together in its export pipeline.

The term identical applied to Instruments describes instances where all identifying fields are equal.

Environment

  • OS: [e.g. any]
  • Architecture: [e.g. any]
  • Go Version: [e.g. any]
  • opentelemetry-go version: [e.g. v1.15.0-rc.1/v0.38.0-rc.1]

Steps To Reproduce

  1. Create multiple instruments with same name but different case such as request_counter, request_Counter, Request_Counter
  2. Notice SDK doesn't aggregate the data for identical instruments

Expected behavior

  1. Data must be aggregated for multiple instruments with same identity.
@srikanthccv srikanthccv added the bug Something isn't working label Mar 4, 2023
@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Mar 4, 2023
@MadVikingGod
Copy link
Contributor

In the data model, there is nothing that makes any field case insensitive. Those would be instruments with different names.

Is there something I am missing where these should be treated as the same instrument?

@MadVikingGod MadVikingGod added the response needed Waiting on user input before progress can be made label Mar 20, 2023
@MrAlias
Copy link
Contributor

MrAlias commented Mar 20, 2023

In the data model, there is nothing that makes any field case insensitive. Those would be instruments with different names.

Is there something I am missing where these should be treated as the same instrument?

I think the issue here is the API defines instrument names as case insensitive:

They are case-insensitive, ASCII strings.

And the SDK "MUST aggregate data from identical Instruments together in its export pipeline." Where, "[t]he term identical applied to Instruments describes instances where all identifying fields are equal." With instrument's name, kind, unit, description, and number type are all considered identifying.

It seems like this issue is identifying that given the Go SDK does not aggregate data from instruments with case-insensitive identical names together it is not compliant with the specification.

@MadVikingGod MadVikingGod removed the response needed Waiting on user input before progress can be made label Mar 20, 2023
@MrAlias MrAlias mentioned this issue Jun 5, 2023
Closed
2 tasks
@MrAlias
Copy link
Contributor

MrAlias commented Jun 5, 2023

Specification issue asking what name to use in the above situation: open-telemetry/opentelemetry-specification#3539

@MrAlias MrAlias self-assigned this Jul 18, 2023
MrAlias added a commit to MrAlias/opentelemetry-go that referenced this issue Jul 18, 2023
@MrAlias
Detect dup inst for case-insensitive names
14e552b 
Resolve open-telemetry#3835

Detect duplicate instrument registrations for instruments that have the
same case-insensitive names. Continue to return the instruments with
different names, but log a warning. This is the solution proposed in
open-telemetry/opentelemetry-specification#3606.
@MrAlias MrAlias mentioned this issue Jul 18, 2023
Merged
MrAlias added a commit that referenced this issue Jul 19, 2023
@MrAlias
Detect duplicate instruments for case-insensitive names (#4338)
e08359f 
* Detect dup inst for case-insensitive names

Resolve #3835

Detect duplicate instrument registrations for instruments that have the
same case-insensitive names. Continue to return the instruments with
different names, but log a warning. This is the solution proposed in
open-telemetry/opentelemetry-specification#3606.

* Add changes to changelog

* Reset global logger after test
@MrAlias
Copy link
Contributor

MrAlias commented Jul 26, 2023

Reopening as open-telemetry/opentelemetry-specification#3539 looks to be heading to a resolution other than what was done to resolve this.

Currently we emit two streams, and the specification looks to be heading to a consensus to emit just the first-value and log a warning.

@MrAlias MrAlias reopened this Jul 26, 2023
@MrAlias
Copy link
Contributor

MrAlias commented Jul 26, 2023

Moved to being blocked by the specification issue:

@MrAlias MrAlias mentioned this issue Aug 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MrAlias MrAlias

Labels
area:metrics Part of OpenTelemetry Metrics bug Something isn't working pkg:SDK Related to an SDK package
Projects
No open projects
Go: Metric SDK (GA)
Status: Done
Milestone
v1.17.0/v0.40.0
3 participants
@MadVikingGod @MrAlias @srikanthccv