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

Change InMemoryExporter to clear collection before exporting Metrics #2937

Closed
wants to merge 46 commits into from
Closed

Conversation

TimothyMothra
Copy link
Contributor

@TimothyMothra TimothyMothra commented Feb 23, 2022

Fixes #2361

Changes

  • New class InMemoryMetricExporter which inherits InmemoryExporter.
    This new class will clear the exported collection before exporting Metrics.
    This mitigates the issue with growing the collection with copies of the same Metric.
  • Minor changes InMemoryExporter to allow code reuse.
  • update doc to explain the contract of MetricExporter
  • update existing unit tests, no longer need to manually clear.
  • add new tests, exploring the behavior of working with Metrics.

TODO

  • Need help with statement in the Readme

Please provide a brief description of the changes here.

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

@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #2937 (83af046) into main (bdcf942) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 83af046 differs from pull request most recent head cfb8773. Consider uploading reports for the commit cfb8773 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2937   +/-   ##
=======================================
  Coverage   84.71%   84.72%           
=======================================
  Files         259      260    +1     
  Lines        9121     9124    +3     
=======================================
+ Hits         7727     7730    +3     
  Misses       1394     1394           
Impacted Files Coverage Δ
...penTelemetry.Exporter.InMemory/InMemoryExporter.cs 100.00% <100.00%> (ø)
...rter.InMemory/InMemoryExporterMetricsExtensions.cs 66.66% <100.00%> (ø)
...emetry.Exporter.InMemory/InMemoryMetricExporter.cs 100.00% <100.00%> (ø)

@TimothyMothra TimothyMothra marked this pull request as ready for review February 24, 2022 03:40
@TimothyMothra TimothyMothra requested a review from a team February 24, 2022 03:40
@TimothyMothra TimothyMothra requested a review from utpilla March 17, 2022 21:40
// This means that exported Metrics will always reflect the latest values.
// Here, we clear the exported collection to prevent populating
// this with duplicate instances of the same Metric.
this.ExportedItems.Clear();
Copy link
Member

Choose a reason for hiding this comment

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

if we (InMemoryExporter) does the clearing:

  1. We are clearing something collection given to us by the user.
  2. This prevents the user from doing comparison between 2 exports.

We could simply let the user do this, if they chose to. And document this in the readme.

@@ -16,28 +16,27 @@

using System.Collections.Generic;

using OpenTelemetry.Internal;

namespace OpenTelemetry.Exporter
{
public class InMemoryExporter<T> : BaseExporter<T>
Copy link
Member

Choose a reason for hiding this comment

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

we can add comment here.

@cijothomas cijothomas added this to the 1.2.0 milestone Mar 22, 2022
@reyang
Copy link
Member

reyang commented Mar 22, 2022

May I get a summary of what's the promise from this PR?

e.g. the metrics stored in the in-memory-exporter will be available until <when?>

The reason I'm asking is:

var meterProvider = Sdk.CreateMeterProviderBuilder()
    .AddMeter(MyMeter.Name)
    .AddInMemoryExporter(collection)
    .Build();

meterProvider.Dispose();

// are the data points in `collection` still available?

@TimothyMothra
Copy link
Contributor Author

Per SIG Meeting, will abandon this PR in favor of doc updates

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.

InMemoryExporter for Metric is buggy
7 participants