You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
At the moment there's zero code reuse in DataPoints collection phase. Due to too much copy/paste there are few issues:
there are inefficiencies by doing unnecessary .clone()
I noticed at least one bug in PrecomputedSum::delta where no_attribute_tracker uses get_value instead of get_and_reset_value.
hard to understand the core logic of aggregator, versus implementation details of how DataPoints are collected.
makes very hard to experiment with performance improvements in measuring and/or collecting phase. E.g. if I change something within ValueMap, I need to go and update at minimum 5 aggregators (if we add ExpoHistogram) * 2 (delta/comulative) = 10 places.
makes very hard to review, because simple change will affect at least 10 places.
Solution
Hide implementation details of ValueMap, and provide simple collect_cumulative and collect_delta functions instead (similar idea as in #2114).
In the future we'll be able to experiment and improve ValueMap implementation and get benefits across all aggregators.
The text was updated successfully, but these errors were encountered:
Agreed. In other implementations like OTel .NET, the entire logic of keeping track of metric points, look up, etc are done in a common place (i.e not repeated for each instument type).
Problem
At the moment there's zero code reuse in DataPoints collection phase. Due to too much copy/paste there are few issues:
.clone()
PrecomputedSum::delta
whereno_attribute_tracker
usesget_value
instead ofget_and_reset_value
.measuring
and/orcollecting
phase. E.g. if I change something within ValueMap, I need to go and update at minimum 5 aggregators (if we addExpoHistogram
) * 2 (delta/comulative) = 10 places.Solution
Hide implementation details of
ValueMap
, and provide simplecollect_cumulative
andcollect_delta
functions instead (similar idea as in #2114).In the future we'll be able to experiment and improve
ValueMap
implementation and get benefits across all aggregators.The text was updated successfully, but these errors were encountered: