Skip to content
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

Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 83 additions & 0 deletions test/OpenTelemetry.Tests/Metrics/MetricExporterTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Copy link
Contributor Author

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 the Export method of the exporter and the exporter has updated the original MetricPoint to default.

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>
{
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not updating the MetricPoint the first time so that we can test that things were working correctly up until we updated the MetricPoint.

{
foreach (ref var metricPoint in metric.GetMetricPoints())
{
metricPoint = default;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is allowed as of today.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bummer... good catch.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah ref readonly should fix it. We would have to update the exporters though which should be fine.

}
}

this.exportedItems.Add(metric);
}

invocationCount++;
return ExportResult.Success;
}
}
}
}