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

add Copy methods to MetricPoint and HistogramBuckets to facilitate deep-copying #3113

Merged
merged 23 commits into from
Mar 31, 2022
Merged

add Copy methods to MetricPoint and HistogramBuckets to facilitate deep-copying #3113

merged 23 commits into from
Mar 31, 2022

Conversation

TimothyMothra
Copy link
Contributor

@TimothyMothra TimothyMothra commented Mar 30, 2022

This is Part1 to address #2361.

Changes

  • Add Copy method to both MetricPoint and HistogramBuckets.
  • remove readonly from MetricPoint.histogramBuckets

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

Discussion

This design is based on my original PoC (#2907) and examples shared by @CodeBlanch (https://github.com/open-telemetry/opentelemetry-dotnet/compare/main...CodeBlanch:inmemory-metrics-copy?expand=1).

The end goal is to enable the InMemoryExporter to deep-copying Metrics.
I looked doing copying only in the InMemoryExporter, but this would require exposing private members.
This PR is the smallest possible change to facilitate copying.

Because we're adding this to the OpenTelemetry library, I ran some Benchmarks.
I evaluated manual copying vs using MemberwiseClone.
I determined that manually copying the MetricPoint and using MemberwiseClone on HistogramBuckets created the fewest allocations.
MemberwiseClone does NOT deep copy nested references, and the manual copy is necessary here. Unit tests validate this behavior.

|                                Method |      Mean |    Error |   StdDev |  Gen 0 |  Gen 1 | Allocated |
|-------------------------------------- |----------:|---------:|---------:|-------:|-------:|----------:|
|         MP_StaticCopy_HB_InstanceCopy |  51.38 ns | 0.734 ns | 0.686 ns | 0.0459 | 0.0001 |     288 B |
|      MP_StaticCopy_HB_MemberwiseClone |  51.06 ns | 0.689 ns | 0.645 ns | 0.0102 |      - |      64 B |
|       MP_InstanceCopy_HB_InstanceCopy |  50.75 ns | 0.539 ns | 0.505 ns | 0.0459 | 0.0001 |     288 B |
|    MP_InstanceCopy_HB_MemberwiseClone |  50.10 ns | 0.522 ns | 0.463 ns | 0.0102 |      - |      64 B |
| MP_MemberwiseClone_HB_MemberwiseClone | 105.55 ns | 1.088 ns | 1.017 ns | 0.0433 |      - |     272 B |
Benchmark code
[MemoryDiagnoser]
public class MetricPointCopyBenchmarks
{
	private Meter meter;
	private MeterProvider provider;
	private Metric metric;
	private MetricPoint metricPoint;

	private Histogram<long> histogram;
	private double[] bounds;

	[GlobalSetup]
	public void Setup()
	{
		this.meter = new Meter(Utils.GetCurrentMethodName());
		this.histogram = this.meter.CreateHistogram<long>("histogram");

		// Evenly distribute the bound values over the range [0, MaxValue)
		this.bounds = new double[10];
		for (int i = 0; i < this.bounds.Length; i++)
		{
			this.bounds[i] = i * 1000 / this.bounds.Length;
		}

		var exportedItems = new List<Metric>();

		this.provider = Sdk.CreateMeterProviderBuilder()
			.AddMeter(this.meter.Name)
			.AddInMemoryExporter(exportedItems)
			.AddView(this.histogram.Name, new ExplicitBucketHistogramConfiguration() { Boundaries = this.bounds })
			.Build();

		this.histogram.Record(500);

		this.provider.ForceFlush();

		this.metric = exportedItems[0];
		var metricPointsEnumerator = this.metric.GetMetricPoints().GetEnumerator();
		metricPointsEnumerator.MoveNext();
		this.metricPoint = metricPointsEnumerator.Current;
	}

	[GlobalCleanup]
	public void Cleanup()
	{
		this.meter?.Dispose();
		this.provider?.Dispose();
	}

	[Benchmark]
	public MetricPoint MP_StaticCopy_HB_InstanceCopy()
	{
		return MetricPoint.StaticCopyWithInstanceCopy(this.metricPoint);
	}

	[Benchmark]
	public MetricPoint MP_StaticCopy_HB_MemberwiseClone()
	{
		return MetricPoint.StaticCopyWithMC(this.metricPoint);
	}

	[Benchmark]
	public MetricPoint MP_InstanceCopy_HB_InstanceCopy()
	{
		return this.metricPoint.InstanceCopyWithInstanceCopy();
	}

	[Benchmark]
	public MetricPoint MP_InstanceCopy_HB_MemberwiseClone()
	{
		return this.metricPoint.InstanceCopyWithMC();
	}

	[Benchmark]
	public MetricPoint MP_MemberwiseClone_HB_MemberwiseClone()
	{
		return this.metricPoint.CopyMC();
	}
}
struct MetricPoint
{
	internal static MetricPoint StaticCopyWithInstanceCopy(in MetricPoint metricPoint)
	{
		MetricPoint copy = metricPoint;
		copy.histogramBuckets = metricPoint.histogramBuckets?.InstanceCopy();
		return copy;
	}

	internal static MetricPoint StaticCopyWithMC(in MetricPoint metricPoint)
	{
		MetricPoint copy = metricPoint;
		copy.histogramBuckets = metricPoint.histogramBuckets?.CopyMC();
		return copy;
	}

	internal MetricPoint InstanceCopyWithInstanceCopy()
	{
		MetricPoint copy = this;
		copy.histogramBuckets = this.histogramBuckets?.InstanceCopy();
		return copy;
	}

	internal MetricPoint InstanceCopyWithMC()
	{
		MetricPoint copy = this;
		copy.histogramBuckets = this.histogramBuckets?.CopyMC();
		return copy;
	}

	internal MetricPoint CopyMC()
	{
		var copy = (MetricPoint)this.MemberwiseClone();
		copy.histogramBuckets = this.histogramBuckets?.CopyMC();
		return copy;
	}
}	
class HistogramBuckets
{
	internal HistogramBuckets InstanceCopy()
	{
		HistogramBuckets copy = new HistogramBuckets(this.ExplicitBounds);

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

		return copy;
	}

	internal HistogramBuckets CopyMC() => (HistogramBuckets)this.MemberwiseClone();
}

@TimothyMothra TimothyMothra requested a review from a team March 30, 2022 21:20
@codecov
Copy link

codecov bot commented Mar 30, 2022

Codecov Report

Merging #3113 (c2db474) into main (213fc03) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head c2db474 differs from pull request most recent head 46ef282. Consider uploading reports for the commit 46ef282 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3113      +/-   ##
==========================================
+ Coverage   84.68%   84.72%   +0.04%     
==========================================
  Files         259      259              
  Lines        9213     9232      +19     
==========================================
+ Hits         7802     7822      +20     
+ Misses       1411     1410       -1     
Impacted Files Coverage Δ
src/OpenTelemetry/Metrics/HistogramBuckets.cs 100.00% <100.00%> (ø)
src/OpenTelemetry/Metrics/MetricPoint.cs 85.51% <100.00%> (+0.95%) ⬆️
src/OpenTelemetry/Metrics/Metric.cs 94.20% <0.00%> (-1.45%) ⬇️
src/OpenTelemetry/Metrics/MetricReaderExt.cs 85.34% <0.00%> (-0.61%) ⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 73.68% <0.00%> (-0.46%) ⬇️
src/OpenTelemetry/Metrics/MetricPointsAccessor.cs 100.00% <0.00%> (ø)
src/OpenTelemetry/Metrics/AggregatorStore.cs 82.85% <0.00%> (+0.09%) ⬆️
src/OpenTelemetry/Metrics/MeterProviderSdk.cs 88.85% <0.00%> (+0.11%) ⬆️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 97.65% <0.00%> (+0.78%) ⬆️
... and 1 more

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

@TimothyMothra TimothyMothra changed the title add Copy methods to MetricPoint and HistogramBuckets to facility deep-copying add Copy methods to MetricPoint and HistogramBuckets to facilitate deep-copying Mar 31, 2022
@cijothomas cijothomas merged commit ac2a4f4 into open-telemetry:main Mar 31, 2022
@TimothyMothra TimothyMothra deleted the 2361_cloning_methods branch March 31, 2022 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants