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

fixing InMemoryExporter & Metrics bug. new class: MetricSnapshot. new ctor: InMemoryExporter(Func) #3266

Merged
merged 23 commits into from
May 25, 2022
Merged

Conversation

TimothyMothra
Copy link
Contributor

@TimothyMothra TimothyMothra commented May 10, 2022

Bug: #2361
Supersedes: #3198.

Changes

  • new class MetricSnapshot. This is used for a strategic selective copy of the minimum members to facilitate unit testing.
    • other names considered: ExportedMetric, ExportableMetricCopy, InMemoryMetricCopy, ReadOnlyMetric.
  • 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<MetricSnapshot>) added to InMemoryExporterMetricsExtensions.

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 MetricSnapshot 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, in this case converting Metric to MetricSnapshot.

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

Additional Comment

This PR keeps the existing functionality, allowing users to export Metrics via the InMemoryExporter.
We have some tests that have a dependency on the Metric.

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

@TimothyMothra TimothyMothra requested a review from a team May 10, 2022 19:32
@TimothyMothra
Copy link
Contributor Author

TimothyMothra commented May 10, 2022

@cijothomas, this PR is meant to address your ask here: #3198 (comment)

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.

@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #3266 (3427907) into main (8aa1778) will increase coverage by 0.04%.
The diff coverage is 76.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3266      +/-   ##
==========================================
+ Coverage   85.22%   85.27%   +0.04%     
==========================================
  Files         264      265       +1     
  Lines        9505     9541      +36     
==========================================
+ Hits         8101     8136      +35     
- Misses       1404     1405       +1     
Impacted Files Coverage Δ
.../OpenTelemetry.Exporter.InMemory/MetricSnapshot.cs 60.00% <60.00%> (ø)
...rter.InMemory/InMemoryExporterMetricsExtensions.cs 75.00% <83.33%> (+8.33%) ⬆️
...penTelemetry.Exporter.InMemory/InMemoryExporter.cs 92.30% <100.00%> (-7.70%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 76.47% <0.00%> (+2.94%) ⬆️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 50.00% <0.00%> (+14.28%) ⬆️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 59.09% <0.00%> (+22.72%) ⬆️

@TimothyMothra TimothyMothra changed the title fixing InMemoryExporter & Metrics bug. new class: MetricCopy. new ctor: InMemoryExporter(Func) fixing InMemoryExporter & Metrics bug. new class: ReadOnlyMetric. new ctor: InMemoryExporter(Func) May 11, 2022
@TimothyMothra TimothyMothra changed the title fixing InMemoryExporter & Metrics bug. new class: ReadOnlyMetric. new ctor: InMemoryExporter(Func) fixing InMemoryExporter & Metrics bug. new class: MetricSnapshot. new ctor: InMemoryExporter(Func) May 13, 2022
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Left 2 comments - please address them as well.

@TimothyMothra
Copy link
Contributor Author

@cijothomas, i just updated the changelog;

* Adds new `AddInMemoryExporter` extension method to export `Metric` as new
type `MetricSnapshot`.
([#2361](https://github.com/open-telemetry/opentelemetry-dotnet/issues/2361))

@cijothomas cijothomas merged commit b9839f6 into open-telemetry:main May 25, 2022
@TimothyMothra TimothyMothra deleted the 2361_MetricCopy branch December 8, 2022 22:30
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