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

[PoC] alternate approach for InMemoryExporter, adding selective copy to Metric (for discussion) #3187

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
14 changes: 13 additions & 1 deletion src/OpenTelemetry.Exporter.InMemory/InMemoryExporter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

using System.Collections.Generic;

using OpenTelemetry.Metrics;

namespace OpenTelemetry.Exporter
{
public class InMemoryExporter<T> : BaseExporter<T>
Expand All @@ -37,7 +39,17 @@ public override ExportResult Export(in Batch<T> batch)

foreach (var data in batch)
{
this.exportedItems.Add(data);
if (data is Metric metric)
{
// By design, The MetricApi reuses Metrics (see: MetricReader.metricsCurrentBatch).
// Here we need to copy the metric to be exported to
// prevent the exported instance from being updated.
this.exportedItems.Add(metric.Copy() as T);
}
else
{
this.exportedItems.Add(data);
}
}

return ExportResult.Success;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
</ItemGroup>

<ItemGroup>
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\PeriodicExportingMetricReaderHelper.cs" Link="Includes\PeriodicExportingMetricReaderHelper.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\ServiceProviderExtensions.cs" Link="Includes\ServiceProviderExtensions.cs" />
<!--<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\PeriodicExportingMetricReaderHelper.cs" Link="Includes\PeriodicExportingMetricReaderHelper.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\ServiceProviderExtensions.cs" Link="Includes\ServiceProviderExtensions.cs" />-->
<Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\Guard.cs" Link="Includes\Guard.cs" />
</ItemGroup>

Expand Down
1 change: 1 addition & 0 deletions src/OpenTelemetry/AssemblyInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using System.Runtime.CompilerServices;

[assembly: InternalsVisibleTo("OpenTelemetry.Tests" + AssemblyInfo.PublicKey)]
[assembly: InternalsVisibleTo("OpenTelemetry.Exporter.InMemory" + AssemblyInfo.PublicKey)]
[assembly: InternalsVisibleTo("OpenTelemetry.Exporter.Prometheus" + AssemblyInfo.PublicKey)]
[assembly: InternalsVisibleTo("OpenTelemetry.Exporter.Prometheus.Tests" + AssemblyInfo.PublicKey)]
[assembly: InternalsVisibleTo("OpenTelemetry.Extensions.Hosting.Tests" + AssemblyInfo.PublicKey)]
Expand Down
10 changes: 10 additions & 0 deletions src/OpenTelemetry/Metrics/AggregatorStore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,14 @@ internal AggregatorStore(
}
}

private AggregatorStore(AggregatorStore other)
{
this.batchSize = other.batchSize;

this.currentMetricPointBatch = (int[])other.currentMetricPointBatch.Clone();
this.metricPoints = Array.ConvertAll(other.metricPoints, metricPoints => metricPoints.Copy());
}

private delegate void UpdateLongDelegate(long value, ReadOnlySpan<KeyValuePair<string, object>> tags);

private delegate void UpdateDoubleDelegate(double value, ReadOnlySpan<KeyValuePair<string, object>> tags);
Expand Down Expand Up @@ -155,6 +163,8 @@ internal void SnapshotCumulative(int indexSnapshot)
internal MetricPointsAccessor GetMetricPoints()
=> new(this.metricPoints, this.currentMetricPointBatch, this.batchSize);

internal AggregatorStore Copy() => new(this);

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void InitializeZeroTagPointIfNotInitialized()
{
Expand Down
1 change: 1 addition & 0 deletions src/OpenTelemetry/Metrics/HistogramBuckets.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ internal HistogramBuckets Copy()
HistogramBuckets copy = new HistogramBuckets(this.ExplicitBounds);

Array.Copy(this.SnapshotBucketCounts, copy.SnapshotBucketCounts, this.SnapshotBucketCounts.Length);
Array.Copy(this.RunningBucketCounts, copy.RunningBucketCounts, this.RunningBucketCounts.Length);
copy.SnapshotSum = this.SnapshotSum;

return copy;
Expand Down
12 changes: 12 additions & 0 deletions src/OpenTelemetry/Metrics/Metric.cs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,13 @@ internal Metric(
this.InstrumentDisposed = false;
}

private Metric(Metric other)
{
this.MetricType = other.MetricType;
this.InstrumentIdentity = other.InstrumentIdentity;
this.aggStore = other.aggStore.Copy();
Copy link
Member

Choose a reason for hiding this comment

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

One concern with this approach. AggregatorStore holds onto a pretty sizeable buffer (sizeof(MetricPoint) * 2000 essentially) so user might OOM if their inmemoryexporter is running long enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point!
It also looks like there's a circular reference with AggregatorStore appearing on both Metric and MetricPoint.

}

public MetricType MetricType { get; private set; }

public AggregationTemporality Temporality { get; private set; }
Expand Down Expand Up @@ -147,5 +154,10 @@ internal int Snapshot()
{
return this.aggStore.Snapshot();
}

internal Metric Copy()
{
return new Metric(this);
}
}
}