[DRAFT] fixing InMemoryExporter & Metrics bug. new class: ExportableMetricCopy. new ctor: InMemoryExporter(Func)

[TimothyMothra]

Fixes #2361

Changes

• new class ExportableMetricCopy. This is used for a strategic selective copy of the minimum members to facilitate unit testing.
• new constructor added to InMemoryExporter to accept Func<Batch<T>, ExportResult.
  This allows the export behavior to be fully replaced and provides greater flexibility for unit tests.
• new extension methods AddInMemoryExporter(ICollection<ExportableMetricCopy>) added to InMemoryExporterMetricsExtensions.
• all affected tests have been updated

Please provide a brief description of the changes here.

Metric instances are reused by design.
As a side effect, the InMemoryExporter will repeatedly export duplicates of Metric instances.
Because of this, Metrics can be modified after export. See also: #2361 (comment)

This change introduces a new class ExportableMetricCopy to be used in unit tests.
This class is a collection of minimum members needed for existing unit tests.
An end user has the option to use either this copy or the original Metric.

This change also introduces a new ctor public InMemoryExporter(Func<Batch<T>, ExportResult> exportFunc).
This is needed to export a type that differs from the Batch type, such as converting Metric to ExportableMetricCopy.
This is also used by other tests:

• PrometheusSerializerTests needs Metric to test PrometheusSerializer.WriteMetric()
• OtlpMetricsExporterTests needs Batch<Metric> to test ExportMetricsServiceRequest.AddMetrics()

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

Todo

• waiting for general acceptance before updating documentation.
  • Update InMemoryExporter changelog
  • Update InMemoryExporter readme

Alternate approaches investigated.

• Deep-clone of Metric has many obstacles. (Not Serializeable, missing parameterless constructors, significant MetricPoint buffer could cause OOM).
• Re-Using Metric class for a selective copy would give a false impression of a deep-clone and introduces a LARGE maintenance tax.

[codecov]

Codecov Report

Merging #3198 (38f0b6d) into main (72fcbf3) will decrease coverage by 0.28%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main    #3198      +/-   ##
==========================================
- Coverage   85.48%   85.19%   -0.29%     
==========================================
  Files         261      262       +1     
  Lines        9410     9452      +42     
==========================================
+ Hits         8044     8053       +9     
- Misses       1366     1399      +33     

Impacted Files	Coverage Δ
...rter.InMemory/InMemoryExporterMetricsExtensions.cs	51.16% <76.00%> (-15.51%)	⬇️
...elemetry.Exporter.InMemory/ExportableMetricCopy.cs	86.66% <86.66%> (ø)
...penTelemetry.Exporter.InMemory/InMemoryExporter.cs	93.33% <100.00%> (-6.67%)	⬇️
src/OpenTelemetry/Metrics/HistogramBuckets.cs	100.00% <100.00%> (ø)
...entation/ExportClient/OtlpGrpcTraceExportClient.cs	50.00% <0.00%> (-28.58%)	⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs	59.09% <0.00%> (-18.19%)	⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	73.52% <0.00%> (-8.83%)	⬇️
...metryProtocol/Implementation/ActivityExtensions.cs	88.36% <0.00%> (-3.45%)	⬇️
... and 2 more

[reyang]

Is there a way (e.g. list.AddRange) to reduce the boilerplate code (ideally keeping it a one liner)?

[TimothyMothra]

I had that same thought while i was writing this. i'll spend some time on this today

[TimothyMothra]

@reyang
AddRange doesn't work here because there's not a built in way to convert Batch to IEnumerable.
If we need to convert Batch to something, I would prefer to convert it to a List.

My initial idea was to add a helper method to the Batch class itself.
Instead, my most recent change adds a helper method to only this test class.
See: private static List<Metric> BatchToList(Batch<Metric> batch)

[reyang]

Looks better, still have room for simplification. Could you make it even simpler?

[TimothyMothra]

The simplest would be to add an additional Extension method that accepts Action<T>. I didn't think I could get away with adding both Func and Action to the PublicApi and between the two I chose Func. :)

[reyang]

One simple way would be having an extension method just in this test project - AddInMemoryExporter(some list type) which handles the boilerplate job.

[TimothyMothra]

I put a new extension method AddInMemoryExporter(List<Metric>) in Tests.Shared so it can be reused by multiple projects.
This helps reduce the number of changes in this PR, the individual Prometheus and OTLP unit tests don't need any changes.

[reyang]

I don't think we want to encourage more use of this copy approach given the fact that Metric can be changed after the export.

[reyang]

LGTM with some suggestions (clean up, reduce boilerplate, etc.).

[TimothyMothra]

This PR got very large.
I'm going to pull out some smaller changes into separate PRs.

[TimothyMothra]

@cijothomas @CodeBlanch, this is ready for review :)

[cijothomas]

I am not sure I follow the reason behind this throw...

[TimothyMothra]

It is by design to force the use of the Copy.
The thought behind this was to prevent users from creating the InMemoryExporter<Metric> because the Metric is known to be faulty. #3198 (comment)

I've documented some scenarios where the Metric can and cannot be trusted: #2361 (comment)

That being said, Prometheus and OTLP have some units tests where they need the actual Metric to provide to their own internals. (documented in the Summary above).

I'm eager to hear your perspective on this!
I'm trying to find the happy medium to support all these use cases while protecting users.
If we decide to allow users to create InMemoryExporter<Metric> we also need to inform/educate users on the risks.

[cijothomas]

We don't need to prevent users from using the one. A lot of tests are already using it, and working fine. (they understand the documented limitation of Metric that it may be updated after Export() call, but they are orchestrating things so as to not be affected)

This can be additional option, for InMemory users, who wants to test metrics, without being affected by the limitation that the memory could be updated.

[cijothomas]

To unblock progress - lets split PR into sub Prs.

1. Add the additional option in InMemoryExporter to use MetricCopy, leave existing functionality untouched. i.e don't change unit tests, except the one breaking due to the metric-being-updated-after-export issue.
2. We can use the SIG meeting to see if there are strong reasons favoring/against keeping the current exporter, and based on that keep it or remove it.

[cijothomas]

is it required to force every test to use the new MetricCopy unless they need to rely on it?
Or is it by design that InMemoryExporter will force one to use the MetricCopy option only? What I don't need/care about the fact that Metric could be update after export() call is done?

[TimothyMothra]

Yes, current approach was to force the use of the Copy.
See the comment above for more details #3198 (comment)

[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.