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

Add PeriodicExporterOptions for console exporter #2648

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/Console/TestMetrics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ internal static object Run(MetricsOptions options)
providerBuilder
.AddConsoleExporter(o =>
{
o.MetricExportIntervalMilliseconds = options.DefaultCollectionPeriodMilliseconds;
o.PeriodicExporterOptions.ExportIntervalMilliseconds = options.DefaultCollectionPeriodMilliseconds;
o.AggregationTemporality = options.IsDelta ? AggregationTemporality.Delta : AggregationTemporality.Cumulative;
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ public static MeterProviderBuilder AddConsoleExporter(this MeterProviderBuilder

var exporter = new ConsoleMetricExporter(options);

var reader = options.MetricExportIntervalMilliseconds == Timeout.Infinite
var reader = options.MetricReaderExportType == MetricReaderExportType.Simple
? new BaseExportingMetricReader(exporter)
: new PeriodicExportingMetricReader(exporter, options.MetricExportIntervalMilliseconds);
: new PeriodicExportingMetricReader(exporter, options.PeriodicExporterOptions.ExportIntervalMilliseconds);

reader.PreferredAggregationTemporality = options.AggregationTemporality;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
// limitations under the License.
// </copyright>

using System.Threading;
using OpenTelemetry.Metrics;

namespace OpenTelemetry.Exporter
Expand All @@ -26,10 +25,9 @@ public class ConsoleExporterOptions
/// </summary>
public ConsoleExporterOutputTargets Targets { get; set; } = ConsoleExporterOutputTargets.Console;

/// <summary>
/// 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;
Copy link
Member

Choose a reason for hiding this comment

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

@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....

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I think the challenges are:

  1. What should be the default exporting period - I think we don't know at this moment.
  2. What is the behavior during exit.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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)


public PeriodicExporterOptions PeriodicExporterOptions { get; set; } = new PeriodicExporterOptions();

/// <summary>
/// Gets or sets the AggregationTemporality used for Histogram
Expand Down
24 changes: 24 additions & 0 deletions src/OpenTelemetry/Metrics/MetricReaderExportType.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// <copyright file="MetricReaderExportType.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

namespace OpenTelemetry.Metrics
{
public enum MetricReaderExportType
reese-lee marked this conversation as resolved.
Show resolved Hide resolved
{
Simple,
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed Simple to Manual.

Copy link
Member

Choose a reason for hiding this comment

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

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 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. Any alternate name
  2. Or pick among the existing Basic, Simple, Manual

Copy link
Member

Choose a reason for hiding this comment

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

OnDemand?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about AdHoc?

Copy link
Member

Choose a reason for hiding this comment

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

Disordered 🤣

Periodic,
}
}
20 changes: 20 additions & 0 deletions src/OpenTelemetry/Metrics/PeriodicExporterOptions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// <copyright file="PeriodicExporterOptions.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

public class PeriodicExporterOptions
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. Rename to PeriodicExportingMetricReaderOptions
  2. Rename to PeriodicMetricReaderOptions
  3. 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

For now, we opted to rename PeriodExporterOptions to PeriodicExportingMetricReaderOptions.

{
public int ExportIntervalMilliseconds { get; set; } = 60000;
}