Add PeriodicExporterOptions for console exporter #2648

reese-lee · 2021-11-20T01:31:10Z

Fixes #2575.

Proposes MetricReaderExportType as a means to allow users to configure periodic vs non-periodic metric readers.
Adds PeriodicExporterOptions similar to the BatchExporterOptions for tracing.
Applies these things to the ConsoleExporter. If we like this approach, then we can apply this to the OTLP exporter as well.

TODO:

Document all public API
Update CHANGELOG

linux-foundation-easycla · 2021-11-20T01:31:12Z

The committers are authorized under a signed CLA.

✅ Reese Lee (6d4fde9)

src/OpenTelemetry/Metrics/MetricReaderExportType.cs

reyang · 2021-11-20T03:57:06Z

src/OpenTelemetry/Metrics/PeriodicExporterOptions.cs

+// limitations under the License.
+// </copyright>
+
+public class PeriodicExporterOptions


This might be misleading, it is actual options for reader, not exporter?

Agreed the name is not quite right here. It was in part inspired by this section of the spec titled Periodic exporting MetricReader.

Few ideas:

Rename to PeriodicExportingMetricReaderOptions

Rename to PeriodicMetricReaderOptions

Do away with the options class and just keep the interval as a top-level property of the console options.

I think I like one of the rename options because the class kind of mimics what we have in tracing plus we should probably expand these options in the future with ExportTimeoutMilliseconds.

Do folks like any of these options?

For now, we opted to rename PeriodExporterOptions to PeriodicExportingMetricReaderOptions.

cijothomas · 2021-11-20T15:56:54Z

src/OpenTelemetry/Metrics/MetricReaderExportType.cs

+{
+    public enum MetricReaderExportType
+    {
+        Simple,


not sure if I can suggest a better name, but simple doesn't convey the behavior...(For tracing, we could use Simple, as spec defined it).
Manual? Basic?

Yea, I'm not a fan of Simple here either. I think Manual conveys behavior best.

I was tempted to consider the terms Push vs. Pull, but those are terms that primarily describe metric exporters not metric readers... if this is true, then I say let's go with Manual.

Renamed Simple to Manual.

I don't know if Manual is a good name, there are cases (e.g. during dispose) it flushes automatically. My gut feeling is Basic > Simple > Manual. Not a blocking comment though, as I'm always struggling and bad at naming 🤷‍♂️

I don't know if Manual is a good name, there are cases (e.g. during dispose) it flushes automatically

Well..the dispose is a manual action, as user has to explicitly call it (.Dispose, or use using statement/block to let c# do it)

I'll merge to unblock progress, and will do a quick vote in todays SIG meeting:

Any alternate name

Or pick among the existing Basic, Simple, Manual

How about AdHoc?

Disordered 🤣

cijothomas · 2021-11-20T17:45:53Z

src/OpenTelemetry.Exporter.Console/ConsoleExporterOptions.cs

-        /// Gets or sets the metric export interval in milliseconds. The default value is <c>Timeout.Infinite</c>.
-        /// </summary>
-        public int MetricExportIntervalMilliseconds { get; set; } = Timeout.Infinite;
+        public MetricReaderExportType MetricReaderExportType { get; set; } = MetricReaderExportType.Simple;


@reyang @alanwest Can you share your thoughts on making the default as Periodic?
One thing I have observed is, users are used to seeing traces/logs from ConsoleExporter, by just doing AddConsoleExporter. However, for Metrics, it won't work. There is nothing in the spec which says ConsoleExporter should be paired with a periodic exporting reader, but I think it makes sense to pair it with PeriodicExportingReader by default, to have similar experience as traces/logs.

This example: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/examples/AspNetCore/Startup.cs#L137, won't work as is, and most folks will be puzzled as to why Metrics are not showing up....

I do agree that it makes sense for the console exporter to default to periodic. I think the general expectation is that info piped to stdout is emitted automatically vs invoking a collect or flush.

Also, my understanding is the console exporter is primarily useful in testing/troubleshooting scenarios. I think this is especially true for metrics, so seeing a quick validation things are alive and working with the single line AddConsoleExporter() is a plus.

That said, I think when we take on the work to support additional output formats, I think the usefulness of the console exporter may expand beyond testing scenarios. In those cases I think folks will be fine configuring the exporter how they need it.

I think the challenges are:

What should be the default exporting period - I think we don't know at this moment.

What is the behavior during exit.

Could we borrow that we do for OTLP Metric exporter?
Same default export interval as OTLP.

The behavior during exit - not sure I follow the specifics here. We do the usual shutdown/dispose right?

Could we borrow that we do for OTLP Metric exporter? Same default export interval as OTLP.

You could. The question is why do you think Console and OTLP should have the same default interval?

I think there should be a concrete usage scenario and we need to think about the design. For example, if the expectation is to use it for interactive output, the refresh rate should be <= 0.3s (based on the common study from interactive UI, and that's the reason why in the Stress test I picked 0.2s delay for each refresh).

There are many issues with the current Console exporter. For example if there are both trace and metrics exporter, and folks would want to see Exemplar which connects traces and metrics, the output will be mixed up because the lack of synchronization on the outputs.

My gut feeling is that by making it outputs periodically without understanding the usage scenario and without a good design, is probably going to make the current situation worse.

I defer to @reyang's judgment on not yet understanding the usage scenario. I've definitely not thought deep about either interactivity nor metric's relationship to tracing.

That said, if we continue to default to non-periodic, perhaps we update the ASP.NET Core example to use periodic to help guide folks?

That said, if we continue to default to non-periodic, perhaps we update the ASP.NET Core example to use periodic to help guide folks?

Either we use periodic console exporter (and gather feedback about the default interval), or use Prometheus exporter. (the current console metrics exporter has a bug that it only outputs to stdout rather than leveraging the abstraction here)

…e-lee/opentelemetry-dotnet into reese-lee/periodic-exporter-options

reyang

LGTM.

codecov · 2021-11-23T00:38:58Z

Codecov Report

Merging #2648 (7502e65) into main (bc0e8af) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2648      +/-   ##
==========================================
- Coverage   82.25%   82.24%   -0.01%     
==========================================
  Files         247      248       +1     
  Lines        8691     8692       +1     
==========================================
  Hits         7149     7149              
- Misses       1542     1543       +1

Impacted Files	Coverage Δ
...ry/Metrics/PeriodicExportingMetricReaderOptions.cs	`0.00% <0.00%> (ø)`

examples/Console/TestMetrics.cs

src/OpenTelemetry/Metrics/MetricReaderType.cs

Add PeriodicExporterOptions for console exporter

6d4fde9

reese-lee requested a review from a team November 20, 2021 01:31

reese-lee and others added 2 commits November 19, 2021 17:54

Fix console example

582ab9e

Merge branch 'main' into reese-lee/periodic-exporter-options

b54d2f3

reyang reviewed Nov 20, 2021

View reviewed changes

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

reyang reviewed Nov 20, 2021

View reviewed changes

cijothomas reviewed Nov 20, 2021

View reviewed changes

reese-lee added 4 commits November 22, 2021 13:50

Rename MetricReaderExportType to MetricReaderType

bba1a15

Rename PeriodicExporterOptions to PeriodicExportingMetricReaderOptions

6219570

Merge branch 'reese-lee/periodic-exporter-options' of github.com:rees…

b455ad0

…e-lee/opentelemetry-dotnet into reese-lee/periodic-exporter-options

Rename Simple to Manual

bad1a9f

reyang approved these changes Nov 22, 2021

View reviewed changes

reese-lee added 7 commits November 22, 2021 15:29

Add XML documentation.

5e4034d

Remove unused using

d9f2ee7

Add missing namespace declaration

7b13ec4

Define new public API

cb2d200

Fix example app

d6b0379

Use periodic metric reader

b2dda9f

Merge branch 'main' into reese-lee/periodic-exporter-options

e1c8b15

reese-lee added 2 commits November 22, 2021 16:39

Update CHANGELOG.md

315ce34

Fix public API files

3e172af

cijothomas reviewed Nov 23, 2021

View reviewed changes

examples/Console/TestMetrics.cs Show resolved Hide resolved

Set MetricReaderType to periodic

88e805f

cijothomas reviewed Nov 23, 2021

View reviewed changes

src/OpenTelemetry/Metrics/MetricReaderType.cs Show resolved Hide resolved

cijothomas approved these changes Nov 23, 2021

View reviewed changes

Merge branch 'main' into reese-lee/periodic-exporter-options

7502e65

cijothomas merged commit cb4beda into open-telemetry:main Nov 23, 2021

alanwest mentioned this pull request Nov 24, 2021

Add MetricReaderType to OtlpExporterOptions #2674

Merged

alanwest mentioned this pull request Feb 18, 2022

What should the default MetricReaderType be? #2921

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PeriodicExporterOptions for console exporter #2648

Add PeriodicExporterOptions for console exporter #2648

reese-lee commented Nov 20, 2021 •

edited

Loading

linux-foundation-easycla bot commented Nov 20, 2021 •

edited

Loading

reyang Nov 20, 2021

alanwest Nov 22, 2021

reese-lee Nov 22, 2021

cijothomas Nov 20, 2021

alanwest Nov 22, 2021

reese-lee Nov 22, 2021

reyang Nov 22, 2021

cijothomas Nov 23, 2021

CodeBlanch Nov 24, 2021

utpilla Nov 24, 2021

reyang Nov 24, 2021

cijothomas Nov 20, 2021

alanwest Nov 20, 2021

reyang Nov 20, 2021

cijothomas Nov 21, 2021

reyang Nov 22, 2021

alanwest Nov 22, 2021

reyang Nov 22, 2021

reyang left a comment

codecov bot commented Nov 23, 2021 •

edited

Loading

Add PeriodicExporterOptions for console exporter #2648

Add PeriodicExporterOptions for console exporter #2648

Conversation

reese-lee commented Nov 20, 2021 • edited Loading

linux-foundation-easycla bot commented Nov 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 23, 2021 • edited Loading

Codecov Report

reese-lee commented Nov 20, 2021 •

edited

Loading

linux-foundation-easycla bot commented Nov 20, 2021 •

edited

Loading

codecov bot commented Nov 23, 2021 •

edited

Loading