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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47c54af
alternate approach, adding selective copy to Metric
TimothyMothra Apr 15, 2022
a5a6c47
minor fix
TimothyMothra Apr 15, 2022
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
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions src/OpenTelemetry.Exporter.InMemory/OpenTelemetry.Exporter.InMemory.csproj
View file
Open in desktop
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
View file
Open in desktop
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
11 changes: 11 additions & 0 deletions src/OpenTelemetry/Metrics/AggregatorStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,15 @@ internal AggregatorStore(
}
}

private AggregatorStore(AggregatorStore other)
{
this.name = other.name;
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 +164,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
View file
Open in desktop
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
13 changes: 13 additions & 0 deletions src/OpenTelemetry/Metrics/Metric.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,14 @@ 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.

this.Temporality = other.Temporality;
}

public MetricType MetricType { get; private set; }

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

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