OTLP exporter options separate out MetricReader options #2717

alanwest · 2021-12-02T03:03:54Z

Related to #2552.

This separates out options meant for configuring the MetricReader when using the AddOltpExporter extension method.

I think this PR is a better alternative to #2716. However unlike #2716 it does not attempt to add support the for the OTEL_EXPORTER_OTLP_METRICS_* environment variables. This could be a follow up PR.

If folks like this approach, I can port it to the Console and In-memory metric exporters.

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricReaderOptions.cs

codecov · 2021-12-02T03:13:52Z

Codecov Report

Merging #2717 (c7fdd42) into main (4f40d74) will decrease coverage by 0.53%.
The diff coverage is 62.06%.

@@            Coverage Diff             @@
##             main    #2717      +/-   ##
==========================================
- Coverage   84.41%   83.88%   -0.54%     
==========================================
  Files         252      253       +1     
  Lines        8893     8910      +17     
==========================================
- Hits         7507     7474      -33     
- Misses       1386     1436      +50

Impacted Files	Coverage Δ
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs	`100.00% <ø> (ø)`
...nTelemetryProtocol/OtlpMetricExporterExtensions.cs	`64.28% <57.14%> (-35.72%)`	⬇️
src/OpenTelemetry/Metrics/MetricReaderOptions.cs	`75.00% <75.00%> (ø)`
...Telemetry/Metrics/PeriodicExportingMetricReader.cs	`0.00% <0.00%> (-78.73%)`	⬇️
...OpenTelemetry/Metrics/BaseExportingMetricReader.cs	`83.33% <0.00%> (-1.67%)`	⬇️

joaopgrassi

I think this approach is great!

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs

alanwest · 2021-12-02T20:12:49Z

@CodeBlanch requested I convert this to draft for now. He's working on another approach and we'd like to give people a chance to compare.

…ions

examples/Console/TestMetrics.cs

mic-max

I'm liking this option

…ions

…lative temporality

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs

github-actions · 2022-02-02T03:16:59Z

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.

github-actions · 2022-02-09T03:17:11Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

…ions

…thub.com:alanwest/opentelemetry-dotnet into alanwest/otlp-separate-out-metric-reader-options

…ions

src/OpenTelemetry/Metrics/MetricReaderOptions.cs

cijothomas · 2022-02-17T23:00:32Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs

+        /// <returns>The instance of <see cref="MeterProviderBuilder"/> to chain the calls.</returns>
+        public static MeterProviderBuilder AddOtlpExporter(
+            this MeterProviderBuilder builder,
+            Action<OtlpExporterOptions, MetricReaderOptions> configureExporterAndMetricReader)


AddOtlpExporter(Action<OtlpExporterOptions> configureExporter, Action <MetricReaderOptions> configureMetricReader)

^ would this be more easy to use?

Maybe? It depends on who you ask 😄. I do not personally have a strong preference either way.

But... I think I may have landed on this solution in part because we're anticipating following this pattern eventually with traces as well. If I recall, the one issue I encountered was that we have

public static TracerProviderBuilder AddOtlpExporter( this TracerProviderBuilder builder, Action<OtlpExporterOptions> configure = null)

Because of the default parameter for configure we can't introduce the following because the methods would be ambiguous if someone were just calling this with no parameters AddOtlpExporter() <- this now does not compile.

public static TracerProviderBuilder AddOtlpExporter( this TracerProviderBuilder builder, Action<OtlpExporterOptions> configure = null, Action<ProcessorOptions> configureProcessor = null)

... though this was a while ago, so I may not have my head on straight about this 😄 - I'll try it out and refresh my memory.

:) not a blocker.

cijothomas · 2022-02-18T18:39:56Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs

-            var metricExporter = new OtlpMetricExporter(options);
+            var metricExporter = new OtlpMetricExporter(exporterOptions);
+
+            if (metricReaderOptions.MetricReaderType == (MetricReaderType)(-1))


lets take this as a follow up to decide

What should be the MetricReaderType default

Would that be same for all exporters (Console , OTLP). Or do we want separate defaults.

Not blocking this PR.

…ions

alanwest added 2 commits December 1, 2021 18:54

Separate out OtlpMetricReaderOptions from OtlpExporterOptions

f8a73d4

Collapse config delegates into one

04a3169

alanwest requested a review from a team December 2, 2021 03:03

alanwest commented Dec 2, 2021

View reviewed changes

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricReaderOptions.cs Outdated Show resolved Hide resolved

joaopgrassi approved these changes Dec 2, 2021

View reviewed changes

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs Outdated Show resolved Hide resolved

cijothomas reviewed Dec 2, 2021

View reviewed changes

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs Outdated Show resolved Hide resolved

cijothomas approved these changes Dec 2, 2021

View reviewed changes

alanwest marked this pull request as draft December 2, 2021 20:11

alanwest added 2 commits December 2, 2021 15:01

Merge branch 'main' into alanwest/otlp-separate-out-metric-reader-opt…

d54f726

…ions

Rename AggregationTemporality to Temporatlity

af01967

CodeBlanch mentioned this pull request Dec 3, 2021

OtlpExporter + JaegerExporter: Options builder #2721

Closed

alanwest added 2 commits December 2, 2021 16:27

Update public API

ccfa146

Change parameter names

6e4bb98

alanwest mentioned this pull request Dec 7, 2021

Move OTLP metric-specific options to its own class #2716

Closed

mic-max reviewed Dec 13, 2021

View reviewed changes

examples/Console/TestMetrics.cs Show resolved Hide resolved

mic-max approved these changes Dec 13, 2021

View reviewed changes

alanwest mentioned this pull request Jan 5, 2022

Batch export vs periodic exporting with OLTP exporter #2767

Closed

alanwest added 5 commits January 25, 2022 13:33

Merge branch 'main' into alanwest/otlp-separate-out-metric-reader-opt…

19e32a0

…ions

Move MetricReaderOptions to SDK

47334da

Default OTLP metric exporter to use a periodic metric reader and cumu…

6e86641

…lative temporality

Update example to configure exporter options

bb27af7

Update public API

0e71ea5

alanwest commented Jan 25, 2022

View reviewed changes

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs Outdated Show resolved Hide resolved

alanwest added 2 commits January 25, 2022 15:18

Everyone loves a good stackoverflow

e262ace

Update public API

5bf0a00

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Feb 2, 2022

github-actions bot closed this Feb 9, 2022

alanwest reopened this Feb 9, 2022

Merge branch 'main' into alanwest/otlp-separate-out-metric-reader-opt…

29959e0

…ions

alanwest marked this pull request as ready for review February 9, 2022 05:22

alanwest removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Feb 16, 2022

alanwest requested a review from cijothomas February 16, 2022 00:35

alanwest added this to the 1.2.0 milestone Feb 16, 2022

alanwest and others added 3 commits February 16, 2022 16:02

Merge branch 'main' into alanwest/otlp-separate-out-metric-reader-opt…

c2fcacb

…ions

Merge branch 'alanwest/otlp-separate-out-metric-reader-options' of gi…

1302f73

…thub.com:alanwest/opentelemetry-dotnet into alanwest/otlp-separate-out-metric-reader-options

Merge branch 'main' into alanwest/otlp-separate-out-metric-reader-opt…

6edffb4

…ions

cijothomas reviewed Feb 17, 2022

View reviewed changes

src/OpenTelemetry/Metrics/MetricReaderOptions.cs Outdated Show resolved Hide resolved

cijothomas reviewed Feb 17, 2022

View reviewed changes

alanwest added 5 commits February 17, 2022 15:27

Fix bad merge

1638313

Change namespace

6b80dfb

Fix the public API

d9dc04d

I ❤️ trying to get the public API correct without Visual Studio

19d62d7

Remove check for unspecified temporality

1d149ed

cijothomas reviewed Feb 18, 2022

View reviewed changes

cijothomas approved these changes Feb 18, 2022

View reviewed changes

cijothomas added 2 commits February 18, 2022 10:40

Merge branch 'main' into alanwest/otlp-separate-out-metric-reader-opt…

63bcd4e

…ions

Merge branch 'main' into alanwest/otlp-separate-out-metric-reader-opt…

c7fdd42

…ions

alanwest mentioned this pull request Feb 18, 2022

What should the default MetricReaderType be? #2921

Closed

alanwest merged commit f234829 into open-telemetry:main Feb 18, 2022

alanwest deleted the alanwest/otlp-separate-out-metric-reader-options branch February 18, 2022 20:02

alanwest mentioned this pull request Feb 22, 2022

Remove metric related options from ConsoleExporterOptions #2929

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OTLP exporter options separate out MetricReader options #2717

OTLP exporter options separate out MetricReader options #2717

alanwest commented Dec 2, 2021 •

edited

Loading

codecov bot commented Dec 2, 2021 •

edited

Loading

joaopgrassi left a comment

alanwest commented Dec 2, 2021 •

edited

Loading

mic-max left a comment

github-actions bot commented Feb 2, 2022

github-actions bot commented Feb 9, 2022

cijothomas Feb 17, 2022

alanwest Feb 18, 2022 •

edited

Loading

cijothomas Feb 18, 2022

cijothomas Feb 18, 2022

alanwest Feb 18, 2022

OTLP exporter options separate out MetricReader options #2717

OTLP exporter options separate out MetricReader options #2717

Conversation

alanwest commented Dec 2, 2021 • edited Loading

codecov bot commented Dec 2, 2021 • edited Loading

Codecov Report

joaopgrassi left a comment

Choose a reason for hiding this comment

alanwest commented Dec 2, 2021 • edited Loading

mic-max left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 2, 2022

github-actions bot commented Feb 9, 2022

cijothomas Feb 17, 2022

Choose a reason for hiding this comment

alanwest Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

cijothomas Feb 18, 2022

Choose a reason for hiding this comment

cijothomas Feb 18, 2022

Choose a reason for hiding this comment

alanwest Feb 18, 2022

Choose a reason for hiding this comment

alanwest commented Dec 2, 2021 •

edited

Loading

codecov bot commented Dec 2, 2021 •

edited

Loading

alanwest commented Dec 2, 2021 •

edited

Loading

alanwest Feb 18, 2022 •

edited

Loading