-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Return the same agg for identical instruments #4201
Comments
The spec says:
Wouldn't an int counter be different from a float counter of the same name, and should get different aggregators because of that? |
For the API sure, but the data-model conflicts with this:
And the SDK spec references the data-model not the API here. |
I think the specification needs to be changed here. |
open-telemetry/opentelemetry-specification#3585 clarified the identifying fields to be:
Based on this, the cache key needs to be updated to include the instrument kind and drop the aggregation name, monotonicity, and temporality. note: the name matching is still being done in a case-sensitive manner. This is still an open question: open-telemetry/opentelemetry-specification#3539 |
Resolve open-telemetry#4201 The specification requires the duplicate instrument conflicts to be identified based on the instrument identifying fields: - name - instrument kind - unit - description - language-level features such as the number type (int64 and float64) Currently, the conflict detection and aggregation caching are done based on the stream IDs which include an aggregation name, monotonicity, and temporality instead of the instrument kind. This changes the conflict detection and aggregation caching to use the OpenTelemetry specified fields. This is effectively a no-op given there is a 1-to-1 mapping of aggregation-name/monotonicity/temporality to instrument kind (they are all resolved based on the instrument kind). Additionally, this adds a stringer representation of the `InstrumentKind`. This is needed for the logging of duplicate instrument conflicts.
Resolve open-telemetry#4201 The specification requires the duplicate instrument conflicts to be identified based on the instrument identifying fields: - name - instrument kind - unit - description - language-level features such as the number type (int64 and float64) Currently, the conflict detection and aggregation caching are done based on the stream IDs which include an aggregation name, monotonicity, and temporality instead of the instrument kind. This changes the conflict detection and aggregation caching to use the OpenTelemetry specified fields. This is effectively a no-op given there is a 1-to-1 mapping of aggregation-name/monotonicity/temporality to instrument kind (they are all resolved based on the instrument kind). Additionally, this adds a stringer representation of the `InstrumentKind`. This is needed for the logging of duplicate instrument conflicts.
* Use inst ID for agg cache key Resolve #4201 The specification requires the duplicate instrument conflicts to be identified based on the instrument identifying fields: - name - instrument kind - unit - description - language-level features such as the number type (int64 and float64) Currently, the conflict detection and aggregation caching are done based on the stream IDs which include an aggregation name, monotonicity, and temporality instead of the instrument kind. This changes the conflict detection and aggregation caching to use the OpenTelemetry specified fields. This is effectively a no-op given there is a 1-to-1 mapping of aggregation-name/monotonicity/temporality to instrument kind (they are all resolved based on the instrument kind). Additionally, this adds a stringer representation of the `InstrumentKind`. This is needed for the logging of duplicate instrument conflicts. * Add changes to changelog
From the specification:
Since identical "describes instances where all identifying fields are equal" and the identifying fields are:
And the cache look up key:
opentelemetry-go/sdk/metric/pipeline.go
Line 326 in 135c64f
is missing the instrument kind, and contains the additional aggregation name, monotonicity, temporality, and number:
opentelemetry-go/sdk/metric/instrument.go
Lines 152 to 171 in 135c64f
The OTel Go implementation is not compliant:
int64
andfloat64
) are created they will use different aggregatorsOriginally posted by @MrAlias in #3653 (comment)
The text was updated successfully, but these errors were encountered: