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

MetricReaderOptions for AddInMemoryExporter extension methods #2931

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Feb 23, 2022

As I was working through this PR I was originally feeling "🤮 I don't like it" and then I started feeling "🤔 I suppose this could work". At first I didn't think there'd ever be the need for a periodic InMemory exporter, but I guess we do have some tests that use it.

I'll let you all be the judge. Do you find it nice to have an extension method for the InMemory exporter that allows configuring the standard options for the underlying metric reader?

@alanwest alanwest requested a review from a team February 23, 2022 01:06
@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #2931 (6927868) into main (843d6fd) will decrease coverage by 0.02%.
The diff coverage is 60.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2931      +/-   ##
==========================================
- Coverage   83.87%   83.84%   -0.03%     
==========================================
  Files         254      254              
  Lines        8918     8940      +22     
==========================================
+ Hits         7480     7496      +16     
- Misses       1438     1444       +6     
Impacted Files Coverage Δ
...rter.InMemory/InMemoryExporterMetricsExtensions.cs 64.00% <60.86%> (-36.00%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 96.87% <0.00%> (-0.79%) ⬇️
...heus/Implementation/PrometheusCollectionManager.cs 82.27% <0.00%> (+2.53%) ⬆️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+2.94%) ⬆️
...ZPages/Implementation/ZPagesExporterEventSource.cs 62.50% <0.00%> (+6.25%) ⬆️

…m:alanwest/opentelemetry-dotnet into alanwest/metric-options-inmemory-exporter
@reyang
Copy link
Member

reyang commented Feb 23, 2022

I'll let you all be the judge. Do you find it nice to have an extension method for the InMemory exporter that allows configuring the standard options for the underlying metric reader?

Yes +1

@cijothomas
Copy link
Member

I'll let you all be the judge. Do you find it nice to have an extension method for the InMemory exporter that allows configuring the standard options for the underlying metric reader?

Yes +1

The question is self-answered in this PR itself. The tests looks much nicer :)

@cijothomas cijothomas merged commit cbc5172 into open-telemetry:main Feb 23, 2022
@alanwest alanwest deleted the alanwest/metric-options-inmemory-exporter branch February 23, 2022 18: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.

3 participants