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

Use inst ID for agg cache key #4337

Merged
merged 5 commits into from
Jul 19, 2023
Merged

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Jul 18, 2023

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.

@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels 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.
@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #4337 (527ad39) into main (c197fe9) will decrease coverage by 0.1%.
The diff coverage is 80.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4337     +/-   ##
=======================================
- Coverage   83.4%   83.4%   -0.1%     
=======================================
  Files        184     185      +1     
  Lines      14330   14329      -1     
=======================================
- Hits       11957   11955      -2     
  Misses      2147    2147             
- Partials     226     227      +1     
Impacted Files Coverage Δ
sdk/metric/instrument.go 91.8% <ø> (ø)
sdk/metric/instrumentkind_string.go 40.0% <40.0%> (ø)
sdk/metric/pipeline.go 91.1% <92.8%> (-0.2%) ⬇️
sdk/metric/meter.go 85.8% <100.0%> (ø)

... and 2 files with indirect coverage changes

@MrAlias MrAlias merged commit 84b2e54 into open-telemetry:main Jul 19, 2023
21 of 22 checks passed
@MrAlias MrAlias deleted the fix-cache-key branch July 19, 2023 16:59
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 this pull request may close these issues.

Return the same agg for identical instruments
3 participants