Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 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
1 change: 0 additions & 1 deletion docs/metrics/extending-the-sdk/MyExporter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ public override ExportResult Export(in Batch<Metric> batch)
}

sb.Append($"{record}");
sb.Append(')');
}

Console.WriteLine($"{this.name}.Export([{sb.ToString()}])");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,13 @@ public static MeterProviderBuilder AddPrometheusExporter(this MeterProviderBuild
configure?.Invoke(options);

var exporter = new PrometheusExporter(options);
var reader = new BaseExportingMetricReader(exporter);

var metricReader = new BaseExportingMetricReader(exporter);
exporter.CollectMetric = metricReader.Collect;
exporter.CollectMetric = timeout => exporter.Collect(reader, timeout);

var metricsHttpServer = new PrometheusExporterMetricsHttpServer(exporter);
metricsHttpServer.Start();
Copy link
Member Author

@reyang reyang Sep 22, 2021

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

return builder.AddMetricReader(metricReader);
return builder.AddMetricReader(reader);
}
}
}
17 changes: 17 additions & 0 deletions src/OpenTelemetry/Metrics/BaseExportingMetricReader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,23 @@ protected override bool ProcessMetrics(Batch<Metric> metrics, int timeoutMillise
return this.exporter.Export(metrics) == ExportResult.Success;
}

/// <inheritdoc />
protected override bool OnCollect(int timeoutMilliseconds)
{
if (this.supportedExportModes.HasFlag(ExportModes.Push))
{
return base.OnCollect(timeoutMilliseconds);
}

if (this.supportedExportModes.HasFlag(ExportModes.Pull) && PullMetricScope.IsPullAllowed)
{
return base.OnCollect(timeoutMilliseconds);
}

// TODO: add some error log
return false;
}

/// <inheritdoc />
protected override bool OnShutdown(int timeoutMilliseconds)
{
Expand Down
69 changes: 69 additions & 0 deletions src/OpenTelemetry/Metrics/MetricExporterExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// <copyright file="MetricExporterExtensions.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>

using System;
using System.Threading;

namespace OpenTelemetry.Metrics
{
/// <summary>
/// Contains extension methods for the <see cref="BaseExporter{Metric}"/> class.
/// </summary>
public static class MetricExporterExtensions
{
/// <summary>
/// Attempts to collect the metrics, blocks the current thread until
/// metrics collection completed or timed out.
/// </summary>
/// <param name="exporter">BaseExporter{Metric} instance on which Collect will be called.</param>
/// <param name="reader">BaseExportingMetricReader instance on which Collect will be called.</param>
/// <param name="timeoutMilliseconds">
/// The number (non-negative) of milliseconds to wait, or
/// <c>Timeout.Infinite</c> to wait indefinitely.
/// </param>
/// <returns>
/// Returns <c>true</c> when metrics collection succeeded; otherwise, <c>false</c>.
/// </returns>
/// <exception cref="System.ArgumentOutOfRangeException">
/// Thrown when the <c>timeoutMilliseconds</c> is smaller than -1.
/// </exception>
/// <remarks>
/// This function guarantees thread-safety.
/// </remarks>
public static bool Collect(this BaseExporter<Metric> exporter, BaseExportingMetricReader reader, int timeoutMilliseconds = Timeout.Infinite)
Copy link
Member

Choose a reason for hiding this comment

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

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

  1. 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)
  1. 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).

Copy link
Member Author

Choose a reason for hiding this comment

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

Another idea we could explore:

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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:

  1. why would reader be a separate parameter, rather than part of the options.
  2. why would we have the options, reader ordering instead of reader, options = optional.
  3. what if we later we figured that we need to add another parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

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);

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@pellared pellared Sep 22, 2021

Choose a reason for hiding this comment

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

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.

{
if (exporter == null)
{
throw new ArgumentNullException(nameof(exporter));
}

if (reader == null)
{
throw new ArgumentNullException(nameof(reader));
}

if (timeoutMilliseconds < 0 && timeoutMilliseconds != Timeout.Infinite)
{
throw new ArgumentOutOfRangeException(nameof(timeoutMilliseconds), timeoutMilliseconds, "timeoutMilliseconds should be non-negative or Timeout.Infinite.");
}

using (PullMetricScope.Begin())
{
return reader.Collect(timeoutMilliseconds);
}
}
}
}
53 changes: 53 additions & 0 deletions src/OpenTelemetry/Metrics/PullMetricScope.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// <copyright file="PullMetricScope.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>

using System;
using System.Runtime.CompilerServices;
using OpenTelemetry.Context;

namespace OpenTelemetry
{
internal sealed class PullMetricScope : IDisposable
Copy link
Member

Choose a reason for hiding this comment

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

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.

{
private static readonly RuntimeContextSlot<bool> Slot = RuntimeContext.RegisterSlot<bool>("otel.pull_metric");

private readonly bool previousValue;
private bool disposed;

internal PullMetricScope(bool value = true)
{
this.previousValue = Slot.Get();
Slot.Set(value);
}

internal static bool IsPullAllowed => Slot.Get();

public static IDisposable Begin(bool value = true)
{
return new PullMetricScope(value);
}

/// <inheritdoc/>
public void Dispose()
{
if (!this.disposed)
{
Slot.Set(this.previousValue);
this.disposed = true;
}
}
}
}
94 changes: 94 additions & 0 deletions test/OpenTelemetry.Tests/Metrics/MetricExporterTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
// <copyright file="MetricExporterTests.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>

using System;
using System.Collections.Generic;
using System.Diagnostics.Metrics;
using Xunit;

namespace OpenTelemetry.Metrics.Tests
{
public class MetricExporterTests
{
[Theory]
[InlineData(ExportModes.Push)]
[InlineData(ExportModes.Pull)]
[InlineData(ExportModes.Pull | ExportModes.Push)]
public void FlushMetricExporterTest(ExportModes mode)
{
BaseExporter<Metric> exporter = null;

switch (mode)
{
case ExportModes.Push:
exporter = new PushOnlyMetricExporter();
break;
case ExportModes.Pull:
exporter = new PullOnlyMetricExporter();
break;
case ExportModes.Pull | ExportModes.Push:
exporter = new PushPullMetricExporter();
break;
}

using var meterProvider = Sdk.CreateMeterProviderBuilder()
.AddMetricReader(new BaseExportingMetricReader(exporter))
.Build();

var result = meterProvider.ForceFlush();

switch (mode)
{
case ExportModes.Push:
Assert.True(result);
break;
case ExportModes.Pull:
Assert.False(result);
break;
case ExportModes.Pull | ExportModes.Push:
Assert.True(result);
break;
}
}

[ExportModes(ExportModes.Push)]
private class PushOnlyMetricExporter : BaseExporter<Metric>
{
public override ExportResult Export(in Batch<Metric> batch)
{
return ExportResult.Success;
}
}

[ExportModes(ExportModes.Pull)]
private class PullOnlyMetricExporter : BaseExporter<Metric>
{
public override ExportResult Export(in Batch<Metric> batch)
{
return ExportResult.Success;
}
}

[ExportModes(ExportModes.Pull | ExportModes.Push)]
private class PushPullMetricExporter : BaseExporter<Metric>
{
public override ExportResult Export(in Batch<Metric> batch)
{
return ExportResult.Success;
}
}
}
}