InMemoryExporter for Metric is buggy

[cijothomas]

Unlike Traces, the Batch provided to the exporter is re-used by SDK to keep metrics in memory. This means that, after the Export(Batch), returns, the memory must not be read by the exporters. If they do so, they might be reading incorrect values.

InMemoryExporter is buggy in this way - the fix would be for the InMemoryExporter to "deep copy" the Metric, MetricPoints, before Export() call returns, so that it has own copy, unaffected by further aggregations.

[TimothyMothra]

@cijothomas, I'm interested in working on this.
I stumbled into a repro of this issue.

[cijothomas]

@utpilla might know more but i think we tried to fix this.. but hit some blockers.

[TimothyMothra]

Sharing my notes here so we can have a discussion next week.

For Context

The following test demonstrates the issue.
In this example, the InMemoryExporter.Export will add Metrics to the list exportedItems.
Each time meterProvider.ForceFlush() is called, the InMemoryExporter will add all available Metrics to the exportedItems list.

Note that exportedItems[0] and exportedItems[1] are the same instance, but the values for the first MetricPoint have been updated (by design).

opentelemetry-dotnet/test/OpenTelemetry.Tests/Metrics/InMemoryExporterTests.cs

Lines 44 to 52 in 8dc2d4a

	counter.Add(10, new KeyValuePair<string, object>("tag1", "value1"));
	meterProvider.ForceFlush();
	var metric = exportedItems[0]; // Only one Metric object is added to the collection at this point
	var metricPointsEnumerator = metric.GetMetricPoints().GetEnumerator();
	Assert.True(metricPointsEnumerator.MoveNext()); // One MetricPoint is emitted for the Metric
	ref readonly var metricPointForFirstExport = ref metricPointsEnumerator.Current;
	Assert.Equal(10, metricPointForFirstExport.GetSumLong());

opentelemetry-dotnet/src/OpenTelemetry.Exporter.InMemory/InMemoryExporter.cs

Lines 31 to 44 in 8dc2d4a

	public override ExportResult Export(in Batch<T> batch)
	{
	if (this.exportedItems == null)
	{
	return ExportResult.Failure;
	}
	foreach (var data in batch)
	{
	this.exportedItems.Add(data);
	}
	return ExportResult.Success;
	}

Currently during flushing, MetricReaderExt creates a new Batch<Metric> using the same Metric[] which is then provided to the exporter.

opentelemetry-dotnet/src/OpenTelemetry/Metrics/MetricReaderExt.cs

Line 232 in 8dc2d4a

return (metricCountCurrentBatch > 0) ? new Batch<Metric>(this.metricsCurrentBatch, metricCountCurrentBatch) : default;

Two problems to be discussed

• When the exporter adds Metrics to the list, this gives the perception that it's adding new unique items.
• MetricPoints can have their values updated after being accessed from the exported collection.

Proposed solutions

Option 1:

Add a new method Metric.GetDeepClonedMetricPoints().
The end user would be expected to call GetDeepClonedMetricPoints to get a unique instance of the MetricPoints.

opentelemetry-dotnet/src/OpenTelemetry/Metrics/AggregatorStore.cs

Lines 154 to 157 in 8dc2d4a

	internal MetricPointsAccessor GetMetricPoints()
	{
	return new MetricPointsAccessor(this.metricPoints, this.currentMetricPointBatch, this.batchSize, this.startTimeExclusive, this.endTimeInclusive);
	}

Option 2:

Change InMemoryExporter.Export to clear the exportedItems collection before returning (Metric only behavior).

• Concurrency implications are not fully understood.

Option 3:

Change MetricReaderExt.GetMetricsBatch() to deep clone Metric[] before returning.

The object hierarchy is as follows: Metric > AggregatorStore > MetricPoint[].

• Performance implications are not fully understood.

[reyang]

Here are the principles I would suggest:

1. Perf - it shouldn't affect the performance of the main story (e.g. anyone who is not using InMemoryExporter will not see perf drop).
2. Simplicity - folks who are using InMemoryExporter should not do extra things. No extra public APIs introduced.

[TimothyMothra]

Questions:

• Are we committed to deep-copying Metric?
• Can we change the behavior of InMemoryExporter for Metrics only?

Regarding deep-copy Metric

The original suggestion was to deep-copy the Metric, but there are challenges to this approach:

1. Cannot use MemoryStream/Binaryormatter because classes are not marked [Serializeable].
2. Cannot use XmlSerializer or JsonSerializer because classes do not have parameterless constructors.
3. A Reflection-based deep-copy might be possible but will have issues w/ parameterless constructor. This could be worked around, but this would be expensive to maintain because any change in any class could affect this.
4. An approach using dotnet's MemberwiseClone pattern might work.

   MemberwiseClone Metric
   MemberwiseClone Metric.AggregatorStore
   Deep-Copy Metric.AggregatorStore.MetricPoint[]
   

   This would require some new public methods.
   Currently, both the AggregatorStore and MetricPoint array are readonly fields.
   Either the readonly would need to be removed or would need to use reflection to override the fields.

Can we change the behavior of InMemoryExporter?

Currently the InMemoryExporter is initialized with a collection of exported items and the exporter adds to that collection.
Because of the way Metrics are reused, the export will add copies of the same Metrics to this collection.
We can mitigate this issue by clearing the collection everytime we begin a Metric export, but this is a change in behavior that needs to be discussed.

Whomever then consumes the exported items collection will have access to only the latest Metrics.
If a user needs to persist MetricPoints between exports, we can facilitate DeepCoping the MetricPoints only.

I opened a Draft PR to illustrate how this would work #2907

[TimothyMothra]

Update

Discussed this issue in the 2/22/2022 meeting.

• Will clear collection in exporter.
• Will update doc with description of change in behavior. Users will be expected to perform any operations with exported items before exporting again. - Will work on this statement in the PR.
• Will update existing unit tests that are manually clearing.

At this time, we will not implement deep-copy. Nothing prevents us from providing this in the future if needed.

Meeting Notes

For now: we can modify the InMemoryExporter to clear the collection. And make the contract of Metric Exporter in such a way that - once control is returned, exporter no longer can assume the state of the Batch. In current implementation of SDK, its reused. In the future, SDK can expose a deepclone()/equivalent. Update the exporter docs as well to reflect this contract.

[reyang]

• Will clear collection in exporter.
• Will update doc with description of change in behavior. Users will be expected to perform any operations with exported items before exporting again. - Will work on this statement in the PR.
• Will update existing unit tests that are manually clearing.

Would this proposal work? I guess no:

counter.Add(...);
provider.ForceFlush();
counter.Add(...); // would this affect what has already been exported (since the point is reused)?

[TimothyMothra]

// would this affect what has already been exported (since the point is reused)?

No, working with the Counter doesn't affect the Metric nor MetricPoint until calling ForceFlush.

The weirdness happens after the second flush and the side effect depends on what a user does with the exported items.

Metric

In this example, the exported collection has a reference to the same Metric and will always have updated values after the second flush.

counter.Add(...);
meterProvider.ForceFlush();
var metric1 = exportedItems[0];

counter.Add(...);
meterProvider.ForceFlush();
var metric2 = exportedItems[0];

Assert.Same(metric1, metric2); // User grabbed a reference to the Metric before the second flush. This will have updated values.

MetricPoint

In this example, metricPoint1 and metricPoint2 are copies of structs and their snapshotValues will be unique.

counter.Add(...);
meterProvider.ForceFlush();
var metricPointsEnumerator1 = exportedItems[0].GetMetricPoints().GetEnumerator(); 
metricPointsEnumerator1.MoveNext(); 
var metricPoint1 = metricPointsEnumerator1.Current; 

counter.Add(...);
meterProvider.ForceFlush();
var metricPointsEnumerator2 = exportedItems[0].GetMetricPoints().GetEnumerator(); 
metricPointsEnumerator2.MoveNext(); 
var metricPoint2 = metricPointsEnumerator2.Current; 

MetricPoint & HistogramBuckets

Contrary to the previous example, when working with Histograms the MetricPoint can no longer be trusted to have unique values.
The MetricPoint struct has a reference to HistogramBuckets and will always reflect the updated value.

var histogram = meter.CreateHistogram<int>("histogram");

histogram.Record(...);
meterProvider.ForceFlush();

var metricPointsEnumerator1 = exportedItems[0].GetMetricPoints().GetEnumerator(); 
metricPointsEnumerator1.MoveNext(); 
var metricPoint1 = metricPointsEnumerator1.Current; 

metricPoint1.GetHistogramCount(); // This will reflect the first Record()
metricPoint1.GetHistogramSum(); // this will reflect the first Record()

histogram.Record(...);
meterProvider.ForceFlush();

metricPoint1.GetHistogramCount(); // This will reflect the updated value
metricPoint1.GetHistogramSum(); // this will reflect the updated value

[TimothyMothra]

Update

Discussed this issue in the 3/22/2022 meeting.

• Will abandon any code changes in favor of doc updates
• Will update
  • extending-the-sdk Readme
  • InMemoryExporter Readme
  • ExtensionMethod xml

[TimothyMothra]

Per conversation in #3071 (comment)

• I'm going to use the doc PR (Update extending-the-sdk Readme to call out that the exporter can not make any assumptions about the Metric batch #3071) to update the extending-the-sdk guide.
• will focus on an approach to clone the exported metrics. We want this change in 1.2

@cijothomas, can you assign this item to the 1.2 milestone for tracking?

[cijothomas]

Sure. The approach shown here (#3071 (comment)), while "cheating" (😄 ), serves the purpose and may be followed.

[cijothomas]

To avoid any more delays to 1.2.0 stable release, removing this item from 1.2.0 milestone. This means InMemoryExporter will not be doing 1.2.0 stable release, along with SDK. (similar to Prometheus Exporter).
Will address this for 1.3.0 stable (dates/features for that will be updated in milestones page soon)

[stevejgordon]

@martinjt This one looks to have been resolved in #3266 so I think it can be closed.