Merge instrument cache to inserter #3724
Conversation
MrAlias
force-pushed
the
inst-cache
branch
3 times, most recently
from
47cfeb4
to
71a7bfb
Compare
MrAlias
added
bug
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3724 +/- ##
=======================================
- Coverage 81.6% 81.6% -0.1%
=======================================
Files 166 166
Lines 12445 12431 -14
=======================================
- Hits 10165 10148 -17
- Misses 2065 2067 +2
- Partials 215 216 +1
|
MrAlias
force-pushed
the
inst-cache
branch
2 times, most recently
from
8c822ae
to
dee4c2a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
MrAlias
changed the title
The current pipeline resolution path will only add the resolved aggregators to the pipeline when it creates one (cache miss). It will not add it if there is a cache hit. This means (since we cache instruments at the meter level, not the pipeline level) the first reader in a multiple-reader setup is the only one that will collect data for that aggregator. All other readers will have a cache hit and nothing is added to the pipeline. This is causing open-telemetry#3720. This resolves open-telemetry#3720 by moving the instrument caching into the inserter. This means aggregators are cached at the reader level, not the meter.
MrAlias
changed the title
MrAlias
requested review from
jmacd,
Aneurysm9,
evantorrie,
XSAM,
dashpole,
MadVikingGod,
pellared,
hanyuancheung and
dmathieu
as code owners
There was a problem hiding this comment.
IIUC, we currently have something like:
Meter > Reader (pipeline) > Instrument > Aggregation
The downside seems to be that two readers that want identical data will duplicate all of the aggregations. I wonder if, in the future it would a good idea to switch to:
Meter > Instrument > Aggregation > Reader (pipeline)
So that identical aggregations would be shared by readers. But I'm not entirely sure that will work, and suspect it would require a large refactor to implement.
True the data is going to be duplicated in memory. We did this because each reader needs to have an unaffected aggregation, when one reader collects it cannot affect the data another reader would collect. Also, when we made this choice views were tied to the reader not the meter provider. There does seem to be an optimization for when multiple readers use cumulative temporarily and the same aggregation selectors, but state would still need to be held if delta temporality is used by a reader. Given how long it took for #3720 to be reported, I don't expect there to be many situations using multiple readers. Which makes me think that optimization isn't too high of a priority. |
|
So maybe can we merge this soon? Looking forward to the next release which brings a fix of the memory issue :) |
The current
pipelineresolution path will only add the resolved aggregator to the pipeline when it creates one (cache miss). It will not add it if there is a cache hit. This means (since we cache instrument at the meter level, not the pipeline level) the first reader in a multiple-reader setup is the only one that will collect data for that aggregator. All other readers will have a cache hit and nothing is added to the pipeline. This is causing #3720.This resolves #3720 by moving the instrument caching into the inserter. This means aggregators are cached at the reader level, not the meter.