-
Notifications
You must be signed in to change notification settings - Fork 772
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
[DO NOT MERGE] Exporter should not be allowed to update MetricPoint in the AggregatorStore #2731
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -15,6 +15,10 @@ | |||
// </copyright> | ||||
|
||||
using System; | ||||
using System.Collections.Generic; | ||||
using System.Diagnostics.Metrics; | ||||
using OpenTelemetry.Exporter; | ||||
using OpenTelemetry.Tests; | ||||
using Xunit; | ||||
|
||||
namespace OpenTelemetry.Metrics.Tests | ||||
|
@@ -65,6 +69,55 @@ public void FlushMetricExporterTest(ExportModes mode) | |||
} | ||||
} | ||||
|
||||
[Fact] | ||||
public void ExporterShouldNotUpdateMetricPoint() | ||||
{ | ||||
var exportedItems = new List<Metric>(); | ||||
var updateMetricPointExporter = new UpdateMetricPointExporter(exportedItems); | ||||
using var meter = new Meter(Utils.GetCurrentMethodName()); | ||||
using var meterProvider = Sdk.CreateMeterProviderBuilder() | ||||
.AddMeter(meter.Name) | ||||
.AddReader(new BaseExportingMetricReader(updateMetricPointExporter) { Temporality = AggregationTemporality.Delta }) | ||||
.Build(); | ||||
|
||||
var counter = meter.CreateCounter<long>("counter"); | ||||
counter.Add(100, new KeyValuePair<string, object>("key", "value")); | ||||
|
||||
meterProvider.ForceFlush(); | ||||
|
||||
Assert.Single(exportedItems); | ||||
var metricPoint = this.GetFirstMetricPointFromMetric(exportedItems[0]); | ||||
Assert.Equal(100, metricPoint.GetSumLong()); | ||||
foreach (var tag in metricPoint.Tags) | ||||
{ | ||||
Assert.Equal("key", tag.Key); | ||||
Assert.Equal("value", tag.Value); | ||||
} | ||||
|
||||
exportedItems.Clear(); | ||||
|
||||
counter.Add(150, new KeyValuePair<string, object>("key", "value")); | ||||
|
||||
meterProvider.ForceFlush(); | ||||
|
||||
Assert.Single(exportedItems); | ||||
metricPoint = this.GetFirstMetricPointFromMetric(exportedItems[0]); | ||||
Assert.Equal(150, metricPoint.GetSumLong()); | ||||
foreach (var tag in metricPoint.Tags) | ||||
{ | ||||
Assert.Equal("key", tag.Key); | ||||
Assert.Equal("value", tag.Value); | ||||
} | ||||
} | ||||
|
||||
private ref MetricPoint GetFirstMetricPointFromMetric(Metric metric) | ||||
{ | ||||
var metricPoints = metric.GetMetricPoints(); | ||||
var metricPointsEnumerator = metricPoints.GetEnumerator(); | ||||
Assert.True(metricPointsEnumerator.MoveNext()); // One MetricPoint is emitted for the Metric | ||||
return ref metricPointsEnumerator.Current; | ||||
} | ||||
|
||||
[ExportModes(ExportModes.Push)] | ||||
private class PushOnlyMetricExporter : BaseExporter<Metric> | ||||
{ | ||||
|
@@ -99,5 +152,35 @@ public override ExportResult Export(in Batch<Metric> batch) | |||
return ExportResult.Success; | ||||
} | ||||
} | ||||
|
||||
private class UpdateMetricPointExporter : BaseExporter<Metric> | ||||
{ | ||||
private static int invocationCount = 1; | ||||
private readonly List<Metric> exportedItems; | ||||
|
||||
public UpdateMetricPointExporter(List<Metric> exportedItems) | ||||
{ | ||||
this.exportedItems = exportedItems; | ||||
} | ||||
|
||||
public override ExportResult Export(in Batch<Metric> batch) | ||||
{ | ||||
foreach (var metric in batch) | ||||
{ | ||||
if (invocationCount > 1) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not updating the |
||||
{ | ||||
foreach (ref var metricPoint in metric.GetMetricPoints()) | ||||
{ | ||||
metricPoint = default; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is allowed as of today. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bummer... good catch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call @utpilla! If you change this line...
...to... public readonly ref MetricPoint Current ...does that fix it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah |
||||
} | ||||
} | ||||
|
||||
this.exportedItems.Add(metric); | ||||
} | ||||
|
||||
invocationCount++; | ||||
return ExportResult.Success; | ||||
} | ||||
} | ||||
} | ||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point,
ForceFlush()
has called theExport
method of the exporter and the exporter has updated the originalMetricPoint
todefault
.