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

Adds a cache to the metric insrument pipeline. #3181

Closed

Conversation

MadVikingGod
Copy link
Contributor

@MadVikingGod MadVikingGod commented Sep 16, 2022

This adds a caching layer to instrument creation, so repeated requests for the same instrument (scope, name, kind) return the same underlying aggregations.

Fix #3229

@codecov
Copy link

codecov bot commented Sep 16, 2022

Codecov Report

Merging #3181 (a316885) into main (b6d4335) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3181   +/-   ##
=====================================
  Coverage   77.4%   77.5%           
=====================================
  Files        158     158           
  Lines      11084   11139   +55     
=====================================
+ Hits        8583    8636   +53     
- Misses      2306    2308    +2     
  Partials     195     195           
Impacted Files Coverage Δ
sdk/metric/instrument_provider.go 80.0% <100.0%> (ø)
sdk/metric/pipeline.go 97.9% <100.0%> (+0.6%) ⬆️
sdk/trace/batch_span_processor.go 80.2% <0.0%> (-0.9%) ⬇️

sdk/metric/pipeline.go Outdated Show resolved Hide resolved
@MrAlias MrAlias added this to the Metric v0.32.2 milestone Sep 23, 2022
@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Sep 23, 2022
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach is viable in that it is should provide correct behavior, but I think it suffers from two issues:

  1. this expands on the confusion of multiple "create" methods as talked about in Refactor Pipeline #3233
  2. it overloads the pipelineRegistry so now it is now a pipeline registry and instrument cache

The first issue I feel is sympomatic of the current pipeline design and it would be addressed in #3233. The second though, I feel needs to be addressed by moving the cache to the Meter object. That is where instrument uniqueness is defined from and could be constructed to emanate from that.

I built out a PoC based on #3233 with how we could resolve both of these issues: https://github.com/MrAlias/opentelemetry-go/pull/660/files

@@ -8,6 +8,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

## [Unreleased]

### Changed

- Add caching to instrument creation (#3181)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Add caching to instrument creation (#3181)
- Add caching to instrument creation.
Repeated requests for the same instrument will return the same instance. (#3181)

@MrAlias
Copy link
Contributor

MrAlias commented Oct 4, 2022

Closing as this has been superseded by #3251

Please reopen if this is in error.

@MrAlias MrAlias closed this Oct 4, 2022
@MadVikingGod MadVikingGod deleted the mvg/instrument_cache branch February 21, 2023 19:56
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
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate instrument registration generates an error instead of a valid instrument
3 participants