new classes ExportedMetric and Exporter

[TimothyMothra]

contributing to #2361

Changes

• new class ExportedMetric. This class is used to deep-copy Metric.

• new exporter. This will be RECOMMENDED for Metrics unit tests.

  • Option 1: new class InMemoryMetricExporter. This is a one-off Exporter that directly creates ExporterMetric.
  • Option 2: new class InMemoryTranslationExporter<Tinput, Toutput>. This is a generic Exporter, and the caller MUST provide a Func to translate input/output types.

• new class InMemoryMetricExporter. This will be RECOMMENDED for unit tests.

• Modify OpenTelemetry project to expose internals to the InMemoryExporter project. (aka "cheating")

• Modify InMemoryExporter removing classes now exposed from OpenTelemetry project.

Next Steps

• After this merges, I'll create a final PR to update the extension method in InMemoryExporterMetricsExtensions and specific unit tests.

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 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-copy Metrics.
The Metric class itself cannot be copied. (For more details see #2361 (comment)).
Instead an instance of Metric can be provided to the new ExportedMetric class which copies the minimum necessary members.
This builds on the changes from #3113 to make copies of MetricPoint and HistogramBuckets.

Alternative

Currently the InMemoryExporter<T> will export the same Type that it receives in the exported Batch<T>
Alternatively, we could introduce a new generic class that allows the caller to specify both the Tinput, Toutput and a provide translation Func<Tinput, Toutput>.

• Pro: Much more flexible, not tied to any specific use case.
• Con: As of today, we don't need additional exporters
• Pro: Future proofs us from a scenario of several one-off exporters.

alternative implementation example

public class InMemoryTransformationExporter<Tinput, Toutput> : BaseExporter<Tinput>
{
    public InMemoryTransformationExporter(ICollection<Toutput> exportedItems, Func<Tinput, Toutput> transformation)
    {
        this.exportedItems = exportedItems;
        this.translation = transformation;
    }

    public override ExportResult Export(in Batch<Tinput> batch)
    {
        foreach (var data in batch)
        {
            this.exportedItems.Add(this.transformation(data));
        }
    }
}

[reyang]

Why do we need a new exporter class?

[TimothyMothra]

We need an exporter that can perform the translation from Metric to ExportedMetric, and this new exporter does that.

The current InMemoryExporter is generic and the exported Type must match the Type in the Batch.

opentelemetry-dotnet/src/OpenTelemetry.Exporter.InMemory/InMemoryExporter.cs

Lines 21 to 31 in d3c0c6e

	public class InMemoryExporter<T> : BaseExporter<T>
	where T : class
	{
	private readonly ICollection<T> exportedItems;
	public InMemoryExporter(ICollection<T> exportedItems)
	{
	this.exportedItems = exportedItems;
	}
	public override ExportResult Export(in Batch<T> batch)

[reyang]

Sorry, what's the real problem?

[TimothyMothra]

The real problem is that users cannot depend on a normally exported Metric to be representative of the point in time it was exported.
Because the MetricApi reuses Metric instances (by design), an exported Metric can continue to be updated.

The Metric class itself cannot be deep-copied, therefore we're introducing a new class which can be representative of the Metric and a new Exporter to perform the translation.

This PR is what I believe to be the simplest solution to the specific problem.

[TimothyMothra]

Something I don't like about this implementation is that it's a one-off class for a specific use case.
We could attempt to future the Exporter library by defining an InMemoryTranslationExporter<Tinput, Toutput>.
It's still a new class, but this would be a single new class to hopefully prevent an unknowable number of future one-off classes.

[TimothyMothra]

Hi @reyang, circling back to this.

The problem I'm trying to address is that InMemoryExporter<T> receives and exports the same type, in this case Metric.
I want to introduce a new type ExportedMetric which is our copy of the Metric.

I don't see a way to accomplish this copy without introducing a new Exporter.

• Option 1: the simplest way to do this is a one-off exporter which performs this copy; InMemoryMetricExporter
• Option 2: to prevent this and future one-off exporters, I could solve this with an InMemoryTranslationExporter<Tinput, Toutput>.
  In this case, the caller must provide a Func<Tinput, Toutput>.

In either option, the caller would be the extension method AddInMemoryExporter which I will contribute in a follow up PR along with test changes.

[reyang]

Can InMemoryExporter be improved to cover the case, so you don't have to introduce a new class? (and if not, why)

[TimothyMothra]

Yes, I could introduce a means to provide an different behavior to the Export method.
I'm struggling with the best way to add this to the PublicApi and also explain customers would use it.

On this same thought, I'm investigating a different approach. I want to open a draft PR to get more feedback.

[codecov]

Codecov Report

Merging #3134 (2e48eb9) into main (c4abf0b) will decrease coverage by 0.26%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #3134      +/-   ##
==========================================
- Coverage   84.74%   84.47%   -0.27%     
==========================================
  Files         259      262       +3     
  Lines        9295     9327      +32     
==========================================
+ Hits         7877     7879       +2     
- Misses       1418     1448      +30     

Impacted Files	Coverage Δ
.../OpenTelemetry.Exporter.InMemory/ExportedMetric.cs	0.00% <0.00%> (ø)
...emetry.Exporter.InMemory/InMemoryMetricExporter.cs	0.00% <0.00%> (ø)
...y.Exporter.InMemory/InMemoryTranslationExporter.cs	0.00% <0.00%> (ø)
...tation/OpenTelemetryProtocolExporterEventSource.cs	75.00% <0.00%> (-10.00%)	⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs	100.00% <0.00%> (+2.85%)	⬆️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	82.35% <0.00%> (+5.88%)	⬆️
...ZPages/Implementation/ZPagesExporterEventSource.cs	62.50% <0.00%> (+6.25%)	⬆️

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[TimothyMothra]

abandoning in favor of #3198