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

Return the same agg for identical instruments #4201

Closed
MrAlias opened this issue Jun 5, 2023 · 5 comments · Fixed by #4337
Closed

Return the same agg for identical instruments #4201

MrAlias opened this issue Jun 5, 2023 · 5 comments · Fixed by #4337
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Jun 5, 2023

From the specification:

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

Since identical "describes instances where all identifying fields are equal" and the identifying fields are:

  • The name of the Instrument
  • The kind of the Instrument - whether it is a Counter or one of the other kinds, whether it is synchronous or asynchronous
  • An optional unit of measure
  • An optional description
  • note: we do not currently support advice so it is not considered here

And the cache look up key:

cv := i.aggregators.Lookup(id, func() aggVal[N] {

is missing the instrument kind, and contains the additional aggregation name, monotonicity, temporality, and number:

// streamID are the identifying properties of a stream.
type streamID struct {
// Name is the name of the stream.
Name string
// Description is the description of the stream.
Description string
// Unit is the unit of the stream.
Unit string
// Aggregation is the aggregation data type of the stream.
Aggregation string
// Monotonic is the monotonicity of an instruments data type. This field is
// not used for all data types, so a zero value needs to be understood in the
// context of Aggregation.
Monotonic bool
// Temporality is the temporality of a stream's data type. This field is
// not used by some data types.
Temporality metricdata.Temporality
// Number is the number type of the stream.
Number string
}

The OTel Go implementation is not compliant:

  • If two different instruments with the same, name, unit, kind, and description but of different number types (i.e. int64 and float64) are created they will use different aggregators

Originally posted by @MrAlias in #3653 (comment)

@MadVikingGod
Copy link
Contributor

The spec says:

Language-level features such as the distinction between integer and floating point numbers SHOULD be considered as identifying.

Wouldn't an int counter be different from a float counter of the same name, and should get different aggregators because of that?

@MrAlias
Copy link
Contributor Author

MrAlias commented Jul 6, 2023

For the API sure, but the data-model conflicts with this:

Within certain data point types (e.g., Sum and Gauge) there is variation permitted in the numeric point value; in this case, the associated variation (i.e., floating-point vs. integer) is not considered identifying.

And the SDK spec references the data-model not the API here.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jul 6, 2023

I think the specification needs to be changed here.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jul 6, 2023

@MrAlias
Copy link
Contributor Author

MrAlias commented Jul 12, 2023

open-telemetry/opentelemetry-specification#3585 clarified the identifying fields to be:

  • name
  • instrument kind
  • unit
  • description
  • language-level features such as the number type (int64 and float64)

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

@MrAlias MrAlias self-assigned this Jul 18, 2023
@MrAlias MrAlias moved this from Todo to In Progress in Go: Metric SDK (GA) Jul 18, 2023
MrAlias added a commit to MrAlias/opentelemetry-go that referenced this issue Jul 18, 2023
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.
MrAlias added a commit to MrAlias/opentelemetry-go that referenced this issue Jul 18, 2023
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.
@github-project-automation github-project-automation bot moved this from In Progress to Done in Go: Metric SDK (GA) Jul 19, 2023
MrAlias added a commit that referenced this issue Jul 19, 2023
* 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
@MrAlias MrAlias added this to the v1.17.0/v0.40.0 milestone Aug 3, 2023
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:SDK Related to an SDK package
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants