Improve Metrics pull export mode by reyang · Pull Request #2389 · open-telemetry/opentelemetry-dotnet

reyang · 2021-09-21T03:22:46Z

With this change, meterProvider.ForceFlush would return false if the underlying exporter ONLY supports pull mode.

In pull exporters (e.g. PrometheusExporter), we can still use the following mechanism to pull metrics.

A basic pull-only exporter would look like this:

[ExportModes(ExportModes.Pull)]
private class PullOnlyMetricExporter : BaseExporter<Metric>, IPullMetricExporter
{
    private Func<int, bool> funcCollect;

    public Func<int, bool> Collect
    {
        get => this.funcCollect;
        set { this.funcCollect = value; }
    }

    public override ExportResult Export(in Batch<Metric> batch)
    {
        return ExportResult.Success;
    }
}

And the exporter can trigger Collect by using exporter.Collect(timeoutMilliseconds). Any direct invocation on reader.Collect and provider.ForceFlush would result in false.

// in the exporter code where we handle scraper
exporter.Collect(timeoutMilliseconds);

codecov · 2021-09-21T03:29:45Z

Codecov Report

Merging #2389 (d64dec7) into main (032f22d) will increase coverage by 0.20%.
The diff coverage is 96.42%.

@@            Coverage Diff             @@
##             main    #2389      +/-   ##
==========================================
+ Coverage   79.99%   80.20%   +0.20%     
==========================================
  Files         231      232       +1     
  Lines        7464     7492      +28     
==========================================
+ Hits         5971     6009      +38     
+ Misses       1493     1483      -10

Impacted Files	Coverage Δ
...OpenTelemetry/Metrics/BaseExportingMetricReader.cs	`85.10% <94.11%> (+11.77%)`	⬆️
src/OpenTelemetry/Metrics/PullMetricScope.cs	`100.00% <100.00%> (ø)`
...Zipkin/Implementation/ZipkinExporterEventSource.cs	`63.63% <0.00%> (-9.10%)`	⬇️
src/OpenTelemetry/Metrics/MeterProviderSdk.cs	`91.08% <0.00%> (+0.99%)`	⬆️
...c/OpenTelemetry/Metrics/MeterProviderExtensions.cs	`22.72% <0.00%> (+22.72%)`	⬆️
src/OpenTelemetry/Metrics/ExportModesAttribute.cs	`100.00% <0.00%> (+100.00%)`	⬆️

src/OpenTelemetry/Metrics/MetricExporterExtensions.cs

CodeBlanch · 2021-09-21T05:28:34Z

src/OpenTelemetry/Metrics/PullMetricScope.cs

+
+namespace OpenTelemetry
+{
+    internal sealed class PullMetricScope : IDisposable


I like this as a way for exporter to tell Collect that it wants to allow pull. Consistent with how we do the instrumentation suppression.

test/OpenTelemetry.Tests/Metrics/MetricExporterTests.cs

CodeBlanch · 2021-09-21T05:35:18Z

src/OpenTelemetry/Metrics/MetricExporterExtensions.cs

+        /// <remarks>
+        /// This function guarantees thread-safety.
+        /// </remarks>
+        public static bool Collect(this BaseExporter<Metric> exporter, BaseExportingMetricReader reader, int timeoutMilliseconds = Timeout.Infinite)


I was chatting to @reyang about this a bit on Slack. Nothing against the extension, just not sure how Exporter will get the second param (MetricReader). Also seems a bit odd the extension is on Exporter, but really only Reader is used.

A couple of ideas for exposing MetricReader to exporter...

Don't. Just feed it to Prometheus where we need it. If some other exporter comes along that needs to call collect, we tackle it then

public PrometheusExporter(PrometheusExporterOptions options, MetricReader parentMetricReader)

I guess @alanwest tried to do some base classes but ran into issues with InMemoryExporter<T>. What if we went with an interface? Closest thing C# has to composition.

public interface ICollectingExporter { MetricReader ParentMetricReader { get; set; } } public class PrometheusExporter : ICollectingExporter { public MetricReader ParentMetricReader { get; set; } }

During MeterProvider build-up we then just do an is check and set the ParentMetricReader on any exporter that implements the interface (basically if exporter asks for it).

Another idea we could explore:

leverage Event https://docs.microsoft.com/en-us/dotnet/standard/events/

Please take my comment from a distance. Just learning the API. But maybe some fresh eyes will help somehow.

This extension method design is strange. Does it mean that the BaseExporter<Metric> has to be coupled with BaseExportingMetricReader? If so then maybe it will be handy to create BaseMetricExporter?

Regarding @CodeBlanch I definitely prefer approach 1 (composition) as it leads to less coupling. Also, I consider interface getters and setters as a code smell.

Not sure how we would like to use event but personally I prefer IObservable<T> as it is easier to compose, works better with extension methods etc.

Option 1 is on the bottom of my list in terms of preference, I even suspect if we could do that (reader.ctor needs exporter, how could exporter.ctor take reader?). I guess we might be able to tweak it and make it work, but I have strong feeling that the exporter.ctor(options, reader) is a bad design:

why would reader be a separate parameter, rather than part of the options.

why would we have the options, reader ordering instead of reader, options = optional.

what if we later we figured that we need to add another parameter?

I've taken option 2 from @CodeBlanch.

A basic pull-only exporter would look like this:

[ExportModes(ExportModes.Pull)] private class PullOnlyMetricExporter : BaseExporter<Metric>, IPullMetricExporter { private Func<int, bool> funcCollect; public Func<int, bool> Collect { get => this.funcCollect; set { this.funcCollect = value; } } public override ExportResult Export(in Batch<Metric> batch) { return ExportResult.Success; } }

And the exporter can trigger Collect by using exporter.Collect(timeoutMilliseconds). Any direct invocation on reader.Collect and provider.ForceFlush would result in false.

// in the exporter code where we handle scraper exporter.Collect(timeoutMilliseconds);

@CodeBlanch @pellared PTAL.

After looking again, I agree that option 1 would be worse.

The current code is OK. I will create a PR, if I come up with anything potentially better.

pellared

My review probably does not bring much value. Only subjective comments.

src/OpenTelemetry/Metrics/BaseExportingMetricReader.cs

pellared · 2021-09-21T09:25:25Z

src/OpenTelemetry/Metrics/MetricExporterExtensions.cs

+        /// <remarks>
+        /// This function guarantees thread-safety.
+        /// </remarks>
+        public static bool Collect(this BaseExporter<Metric> exporter, BaseExportingMetricReader reader, int timeoutMilliseconds = Timeout.Infinite)


Please take my comment from a distance. Just learning the API. But maybe some fresh eyes will help somehow.

This extension method design is strange. Does it mean that the BaseExporter<Metric> has to be coupled with BaseExportingMetricReader? If so then maybe it will be handy to create BaseMetricExporter?

Regarding @CodeBlanch I definitely prefer approach 1 (composition) as it leads to less coupling. Also, I consider interface getters and setters as a code smell.

Not sure how we would like to use event but personally I prefer IObservable<T> as it is easier to compose, works better with extension methods etc.

reyang · 2021-09-22T04:11:23Z

src/OpenTelemetry.Exporter.Prometheus/MeterProviderBuilderExtensions.cs

+            var reader = new BaseExportingMetricReader(exporter);

            var metricsHttpServer = new PrometheusExporterMetricsHttpServer(exporter);
            metricsHttpServer.Start();


@alanwest I noticed this while changing the code.

I think we might run into some race condition - if a scraper happens to hit the HTTP server before we could add the reader, what would happen (I guess we will hit exception, which turns into HTTP 500)? I haven't looked into the HTTP server logic.

I think it might be OKAY. A better version could be - we only start the HTTP server once the exporter/reader are fully ready and both are hooked up to the provider.

if the reader is not ready, we could modify the http server to still return 200, but with empty metric.
try
{
this.exporter.Collect(Timeout.Infinite)
}
catch(Exporter/Reader not readyexception)
{
exporter.BatchMetrics = default.
}

Or we can wait for some signal before starting HTTP Server.
Will track this as a separate follow up once this PR is merged.

cijothomas

looks very neat!
please update the PR description to reflect new exporter.Collect() style.

reyang · 2021-09-22T05:56:55Z

looks very neat!
please update the PR description to reflect new exporter.Collect() style.

updated

reyang requested a review from a team September 21, 2021 03:22

reyang mentioned this pull request Sep 21, 2021

Metrics SDK bug caused app crash #2390

Closed

reyang commented Sep 21, 2021

View reviewed changes

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

reyang closed this Sep 21, 2021

reyang reopened this Sep 21, 2021

cijothomas approved these changes Sep 21, 2021

View reviewed changes

reyang added 5 commits September 20, 2021 22:10

Improve metrics pulling logic

c005741

add test case

95204e8

format code

b4a3e1a

fix format format

6c7fc48

fix nit

ba04ddf

reyang force-pushed the reyang/pull-mode branch from 39955ee to ba04ddf Compare September 21, 2021 05:10

update PrometheusExporter to leverage the new logic

f5912a4

CodeBlanch reviewed Sep 21, 2021

View reviewed changes

improve test

1e5b868

reyang commented Sep 21, 2021

View reviewed changes

test/OpenTelemetry.Tests/Metrics/MetricExporterTests.cs Outdated Show resolved Hide resolved

CodeBlanch reviewed Sep 21, 2021

View reviewed changes

clean up

86a162b

pellared reviewed Sep 21, 2021

View reviewed changes

reyang added 2 commits September 21, 2021 20:57

refactor

380ea1c

Merge branch 'main' into reyang/pull-mode

d64dec7

reyang commented Sep 22, 2021

View reviewed changes

cijothomas approved these changes Sep 22, 2021

View reviewed changes

cijothomas merged commit 15981df into open-telemetry:main Sep 22, 2021

reyang deleted the reyang/pull-mode branch September 22, 2021 20:09

reyang mentioned this pull request Sep 23, 2021

Implement the metrics dispose/shutdown logic #2404

Merged

Conversation

reyang commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pellared Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pellared left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reyang Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cijothomas left a comment

Choose a reason for hiding this comment

Uh oh!

reyang commented Sep 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

reyang commented Sep 21, 2021 •

edited

Loading

codecov bot commented Sep 21, 2021 •

edited

Loading

pellared Sep 22, 2021 •

edited

Loading

reyang Sep 22, 2021 •

edited

Loading