[PoC] Mitigation for InMemoryExporter + Metrics (for discussion)

[TimothyMothra]

This is an alternative to DeepCopying the Metric.
Discussing this in #2361.

Changes

• DeepCopy MetricPoint. This includes minor changes to MetricPoint and HistogramBuckets.
• Metric.GetMetricPoints changed to always return a deepcopy.
  This change passes all unit tests locally.
  Need to discuss if this change is acceptable.
• InMemoryExporter to clear the exportedItems collection when exporting Metric.
  This mitigates the issue with growing the collection with copies of the same Metric.
  Need to discuss if this change is acceptable.

For significant contributions please make sure you have completed the following items:

• Appropriate CHANGELOG.md updated for non-trivial changes
• Design discussion issue InMemoryExporter for Metric is buggy #2361
• Changes in public API reviewed

[utpilla]

We cannot update this method as this would be used by all other exporters for best performance.

We need to add a new method GetClonedMetricPoints which should call this.aggStore.GetClonedMetricPoints. The user of InMemoryExporter has to be aware of this method and call GetClonedMetricPoints instead of GetMetricPoints:

foreach (var metric in batch)
{
    foreach (ref readonly var metricPoint in metric.GetCloneMetricPoints())
    {
        ...
        ...
        ...
    }
}

[utpilla]

We only need to deep copy HistogramBuckets.SnapshotBucketCounts.

[codecov]

Codecov Report

Merging #2907 (e9dee07) into main (84821ff) will decrease coverage by 0.33%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main    #2907      +/-   ##
==========================================
- Coverage   84.22%   83.89%   -0.34%     
==========================================
  Files         251      253       +2     
  Lines        8883     8922      +39     
==========================================
+ Hits         7482     7485       +3     
- Misses       1401     1437      +36     

Impacted Files	Coverage Δ
src/OpenTelemetry/Metrics/HistogramBuckets.cs	95.65% <0.00%> (-4.35%)	⬇️
...penTelemetry.Exporter.InMemory/InMemoryExporter.cs	100.00% <100.00%> (ø)
src/OpenTelemetry/Metrics/AggregatorStore.cs	83.24% <100.00%> (+0.38%)	⬆️
src/OpenTelemetry/Metrics/Metric.cs	94.52% <100.00%> (+0.07%)	⬆️
src/OpenTelemetry/Metrics/MetricPoint.cs	85.92% <100.00%> (+0.31%)	⬆️
...Telemetry/Metrics/PeriodicExportingMetricReader.cs	0.00% <0.00%> (-78.73%)	⬇️
...nTelemetryProtocol/OtlpMetricExporterExtensions.cs	64.28% <0.00%> (-35.72%)	⬇️
...OpenTelemetry/Metrics/BaseExportingMetricReader.cs	83.33% <0.00%> (-1.67%)	⬇️
...umentation.AspNet/Implementation/HttpInListener.cs	88.31% <0.00%> (-0.30%)	⬇️
...metryProtocol/OtlpTraceExporterHelperExtensions.cs	94.73% <0.00%> (-0.27%)	⬇️
... and 11 more

[utpilla]

Let's first decide on this. Do we want the collection of exported items to hold the MetricPoint state corresponding to every export made so far?

[CodeBlanch]

Personally, I feel like... no, it shouldn't auto-clear anything. Whoever constructed the InMemoryExporter owns the collection that was passed in. So they can clear it if needed based on whatever test is being modeled? But yes the exporter should move copies into that collection so the data doesn't continue to change.

[TimothyMothra]

Hi @CodeBlanch,
If the InMemoryExporter could deepcopy the Metric into the export collection, I believe that would fully address this issue.
However, I don't see an obvious way to accomplish this.
I listed here several methods to perform a deepcopy and the blocking issues with each.
If you have any other ideas, I'm eager to get your input!