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.

Sign up for GitHub

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

Revive using Batch for metric export #2327

Merged
merged 8 commits into from
Sep 10, 2021
Merged

Revive using Batch for metric export #2327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
672a6bf
Use Batch for metric export - previously implemented in #2295
alanwest Sep 9, 2021
f67c9c9
Implement ConsoleExporter.GetAggregationTemporality
alanwest Sep 9, 2021
748ee0b
Add special handling for Batch<Metric>
alanwest Sep 9, 2021
f08cc50
Fix build
alanwest Sep 9, 2021
21ff3f7
Merge branch 'metrics' into alanwest/revive-batch-for-metrics
cijothomas Sep 9, 2021
104bd32
Merge branch 'metrics' into alanwest/revive-batch-for-metrics
alanwest Sep 10, 2021
a7403da
Fix build
alanwest Sep 10, 2021
ef58b68
Update src/OpenTelemetry/BaseExporter.cs
alanwest Sep 10, 2021
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
7 changes: 7 additions & 0 deletions src/OpenTelemetry.Exporter.Console/ConsoleExporter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
// limitations under the License.
// </copyright>

using OpenTelemetry.Metrics;

namespace OpenTelemetry.Exporter
{
public abstract class ConsoleExporter<T> : BaseExporter<T>
Expand All @@ -26,6 +28,11 @@ protected ConsoleExporter(ConsoleExporterOptions options)
this.options = options ?? new ConsoleExporterOptions();
}

public override AggregationTemporality GetAggregationTemporality()
{
return this.options.AggregationTemporality;
}

protected void WriteLine(string message)
{
if (this.options.Targets.HasFlag(ConsoleExporterOutputTargets.Console))
Expand Down
16 changes: 4 additions & 12 deletions src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
// </copyright>

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Text;
Expand All @@ -24,22 +23,16 @@

namespace OpenTelemetry.Exporter
{
public class ConsoleMetricExporter : MetricExporter
public class ConsoleMetricExporter : ConsoleExporter<Metric>
{
private Resource resource;
private ConsoleExporterOptions options;

public ConsoleMetricExporter(ConsoleExporterOptions options)
: base(options)
{
this.options = options;
}

public override AggregationTemporality GetAggregationTemporality()
{
return this.options.AggregationTemporality;
}

public override ExportResult Export(IEnumerable<Metric> metrics)
public override ExportResult Export(in Batch<Metric> batch)
{
if (this.resource == null)
{
Expand All @@ -56,7 +49,7 @@ public override ExportResult Export(IEnumerable<Metric> metrics)
}
}

foreach (var metric in metrics)
foreach (var metric in batch)
{
var msg = new StringBuilder($"\nExport ");
msg.Append(metric.Name);
Expand Down Expand Up @@ -167,7 +160,6 @@ public override ExportResult Export(IEnumerable<Metric> metrics)
}

msg = new StringBuilder();
msg.Append("(");
msg.Append(metricPoint.StartTime.ToString("yyyy-MM-ddTHH:mm:ss.fffffffZ", CultureInfo.InvariantCulture));
msg.Append(", ");
msg.Append(metricPoint.EndTime.ToString("yyyy-MM-ddTHH:mm:ss.fffffffZ", CultureInfo.InvariantCulture));
Expand Down
7 changes: 5 additions & 2 deletions src/OpenTelemetry.Exporter.InMemory/InMemoryExporterMetricHelperExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,11 @@ public static MeterProviderBuilder AddInMemoryExporter(this MeterProviderBuilder

var options = new InMemoryExporterOptions();
configure?.Invoke(options);
var exporter = new InMemoryMetricExporter(exportedItems, options);
return builder.AddMetricReader(new PeriodicExportingMetricReader(exporter, options.MetricExportIntervalMilliseconds));

// var exporter = new InMemoryMetricExporter(exportedItems, options);
// return builder.AddMetricReader(new PeriodicExportingMetricReader(exporter, options.MetricExportIntervalMilliseconds));
Comment on lines +48 to +49
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'll fix the InMemoryExporter right after this PR lands


return builder;
}
}
}
8 changes: 5 additions & 3 deletions src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterHelperExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,11 @@ public static MeterProviderBuilder AddOtlpExporter(this MeterProviderBuilder bui
var options = new OtlpExporterOptions();
configure?.Invoke(options);

var metricExporter = new OtlpMetricsExporter(options);
var metricReader = new PeriodicExportingMetricReader(metricExporter, options.MetricExportIntervalMilliseconds);
return builder.AddMetricReader(metricReader);
// var metricExporter = new OtlpMetricsExporter(options);
// var metricReader = new PeriodicExportingMetricReader(metricExporter, options.MetricExportIntervalMilliseconds);
// return builder.AddMetricReader(metricReader);
Comment on lines +43 to +45
Copy link
Member Author

Choose a reason for hiding this comment

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

Same thing with the OtlpExporter


return builder;
}
}
}
15 changes: 9 additions & 6 deletions src/OpenTelemetry.Exporter.Prometheus/MeterProviderBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,17 @@ public static MeterProviderBuilder AddPrometheusExporter(this MeterProviderBuild

var options = new PrometheusExporterOptions();
configure?.Invoke(options);
var exporter = new PrometheusExporter(options);

var metricReader = new BaseExportingMetricReader(exporter);
exporter.CollectMetric = metricReader.Collect;
// var exporter = new PrometheusExporter(options);

var metricsHttpServer = new PrometheusExporterMetricsHttpServer(exporter);
metricsHttpServer.Start();
return builder.AddMetricReader(metricReader);
// var metricReader = new BaseExportingMetricReader(exporter);
// exporter.CollectMetric = metricReader.Collect;

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

Choose a reason for hiding this comment

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

Oh yea and Prometheus too 😄

// metricsHttpServer.Start();
// return builder.AddMetricReader(metricReader);

return builder;
}
}
}
9 changes: 9 additions & 0 deletions src/OpenTelemetry/BaseExporter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using System;
using System.Threading;
using OpenTelemetry.Internal;
using OpenTelemetry.Metrics;

namespace OpenTelemetry
{
Expand Down Expand Up @@ -105,6 +106,14 @@ public void Dispose()
GC.SuppressFinalize(this);
}

public virtual AggregationTemporality GetAggregationTemporality()
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 do BaseMetricExporter : BaseExporter<Metric> and define GetAggregationTemporality() inside BaseMetricExporter only?

Copy link
Member

Choose a reason for hiding this comment

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

Probably just call it MetricExporter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, so I tried this out originally, the stickiness I ran into is that we'd like to be able to do ConsoleMetricExporter : ConsoleExporter<Metric> same kind of thing for OtlpMetricExporter : OltpExporter<Metric>. The base console and OTLP exporters contain common logic we're hoping to take advantage of.

Though, we might be fighting the type hierarchy too hard. It may make more sense to move this common logic to separate classes and have a handle to it. This would enable us to do something like OtlpMetricExporter : MetricExporter : BaseExporter<Metric>.

I plan to experiment with this in a follow up.

{
// TODO: One suggestion is to have SupportedTemporality
// and PreferredTemporality.
// see https://github.com/open-telemetry/opentelemetry-dotnet/pull/2306#discussion_r701532743
return AggregationTemporality.Cumulative;
}

/// <summary>
/// Called by <c>Shutdown</c>. This function should block the current
/// thread until shutdown completed or timed out.
Expand Down
49 changes: 49 additions & 0 deletions src/OpenTelemetry/Batch.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
using System.Collections.Generic;
using System.Diagnostics;
using OpenTelemetry.Internal;
using OpenTelemetry.Metrics;

namespace OpenTelemetry
{
Expand All @@ -31,12 +32,14 @@ namespace OpenTelemetry
{
private readonly T item;
private readonly CircularBuffer<T> circularBuffer;
private readonly T[] metrics;
private readonly long targetCount;

internal Batch(T item)
{
this.item = item ?? throw new ArgumentNullException(nameof(item));
this.circularBuffer = null;
this.metrics = null;
Copy link
Member

Choose a reason for hiding this comment

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

Just to call out loud here, do we need to initialize these to null, or they are null by default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since Batch is a struct, all fields must be explicitly initialized.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks!

Copy link
Member

@reyang reyang Sep 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@CodeBlanch I guess a private Batch ctor which sets everything to null plus aggressive inline, then reuse it across all other internal ctors?

Copy link
Member

Choose a reason for hiding this comment

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

we cannot define a default ctor like Batch().

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like a default copy constructor?

Copy link
Member

Choose a reason for hiding this comment

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

#3484

this.targetCount = 1;
}

Expand All @@ -45,10 +48,21 @@ internal Batch(CircularBuffer<T> circularBuffer, int maxSize)
Debug.Assert(maxSize > 0, $"{nameof(maxSize)} should be a positive number.");

this.item = null;
this.metrics = null;
this.circularBuffer = circularBuffer ?? throw new ArgumentNullException(nameof(circularBuffer));
this.targetCount = circularBuffer.RemovedCount + Math.Min(maxSize, circularBuffer.Count);
}

internal Batch(T[] metrics, int maxSize)
{
Debug.Assert(maxSize > 0, $"{nameof(maxSize)} should be a positive number.");

this.item = null;
this.circularBuffer = null;
this.metrics = metrics ?? throw new ArgumentNullException(nameof(metrics));
this.targetCount = maxSize;
}

/// <inheritdoc/>
public void Dispose()
{
Expand All @@ -70,6 +84,8 @@ public Enumerator GetEnumerator()
{
return this.circularBuffer != null
? new Enumerator(this.circularBuffer, this.targetCount)
: this.metrics != null
? new Enumerator(this.metrics, this.targetCount)
: new Enumerator(this.item);
}

Expand All @@ -79,20 +95,35 @@ public Enumerator GetEnumerator()
public struct Enumerator : IEnumerator<T>
{
private readonly CircularBuffer<T> circularBuffer;
private readonly T[] metrics;
private long targetCount;
private int metricIndex;

internal Enumerator(T item)
{
this.Current = item;
this.circularBuffer = null;
this.metrics = null;
this.targetCount = -1;
this.metricIndex = 0;
}

internal Enumerator(CircularBuffer<T> circularBuffer, long targetCount)
{
this.Current = null;
this.metrics = null;
this.circularBuffer = circularBuffer;
this.targetCount = targetCount;
this.metricIndex = 0;
}

internal Enumerator(T[] metrics, long targetCount)
{
this.Current = null;
this.circularBuffer = null;
this.metrics = metrics;
this.targetCount = targetCount;
this.metricIndex = 0;
}

/// <inheritdoc/>
Expand All @@ -109,6 +140,24 @@ public void Dispose()
/// <inheritdoc/>
public bool MoveNext()
{
if (typeof(T) == typeof(Metric))
Copy link
Member Author

Choose a reason for hiding this comment

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

@cijothomas's thought was that this would get optimized out when T != Metric

Copy link
Member

Choose a reason for hiding this comment

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

alternate option to try.
Similar to this:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/Trace/TracerProviderSdk.cs#L145

Copy link
Member Author

@alanwest alanwest Sep 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I haven't yet had the chance to try the alternate approach you've suggested, but before I make those changes here's some perf numbers with and without this if (typeof(T) == typeof(Metric)) block.

The benchmark simulates what the SimpleActivityExportProcessor does in order to assess the impact of the if (typeof(T) == typeof(Metric)) check.

Benchmark 
    [MemoryDiagnoser]
    public class BatchBenchmarks
    {
        [Benchmark]
        public void ActivityBatch()
        {
            var activity = new Activity("activity");
            var batch = new Batch<Activity>(activity);

            foreach (var item in batch)
            {
                item.Start();
                item.Stop();
            }
        }
    }

With if (typeof(T) == typeof(Metric))

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
ActivityBatch 493.1 ns 5.68 ns 5.04 ns 0.0486 - - 408 B

Without if (typeof(T) == typeof(Metric))

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
ActivityBatch 490.6 ns 6.43 ns 5.70 ns 0.0486 - - 408 B

I've gotten some pretty varied results. Sometimes the Without run is slower than the With run.

Copy link
Member

Choose a reason for hiding this comment

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

I tried similar benchmarks and results are not consistent. Sometimes the new Batch is fast!
I think the ActivityCreation itself is ~450-500. And the additional overhead we introduced is within margin of error.

namespace Benchmarks.Metrics
{
    [MemoryDiagnoser]
    public class MetricCollectBenchmarks
    {
        private double totalMs;

        [GlobalSetup]
        public void Setup()
        {
        }

        [GlobalCleanup]
        public void Cleanup()
        {
        }

        [Benchmark]
        public void BatchOriginal()
        {
            var activity = new Activity("activity");
            activity.Start();
            activity.Stop();
            var batch = new BatchOriginal<Activity>(activity);
            foreach (var act in batch)
            {
                var ms = act.Duration.TotalMilliseconds;
                this.totalMs += ms;
            }
        }

        [Benchmark]
        public void BatchNew()
        {
            var activity = new Activity("activity");
            activity.Start();
            activity.Stop();
            var batch = new Batch<Activity>(activity);
            foreach (var act in batch)
            {
                var ms = act.Duration.TotalMilliseconds;
                this.totalMs += ms;
            }
        }
    }
}

Run1:

Method Mean Error StdDev Median Gen 0 Allocated
BatchOriginal 463.9 ns 15.78 ns 44.50 ns 461.6 ns 0.0916 384 B
BatchNew 442.1 ns 18.27 ns 52.12 ns 423.8 ns 0.0916 384 B

Run2:

Method Mean Error StdDev Gen 0 Allocated
BatchOriginal 476.8 ns 12.50 ns 35.47 ns 0.0973 408 B
BatchNew 469.8 ns 9.40 ns 22.34 ns 0.0973 408 B

Copy link
Member

Choose a reason for hiding this comment

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

@reyang The benchmarks doesn't show any consistently measurable regression (sometimes new one is faster).

Given that adding metrics to Batch is not affecting other signals in measurable way, we should be okay to add metrics support to Batch and achieve cleaner exporter code.

Please let us know your comments.

Copy link
Member

Choose a reason for hiding this comment

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

+1.

{
var metrics = this.metrics;

if (metrics != null)
{
if (this.metricIndex < this.targetCount)
{
this.Current = metrics[this.metricIndex];
this.metricIndex++;
return true;
}
}

this.Current = null;
return false;
}

var circularBuffer = this.circularBuffer;

if (circularBuffer == null)
Expand Down
7 changes: 3 additions & 4 deletions src/OpenTelemetry/Metrics/BaseExportingMetricReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,20 @@
// </copyright>

using System;
using System.Collections.Generic;

namespace OpenTelemetry.Metrics
{
public class BaseExportingMetricReader : MetricReader
{
private readonly MetricExporter exporter;
private readonly BaseExporter<Metric> exporter;
private bool disposed;

public BaseExportingMetricReader(MetricExporter exporter)
public BaseExportingMetricReader(BaseExporter<Metric> exporter)
{
this.exporter = exporter;
}

public override void OnCollect(IEnumerable<Metric> metrics)
public override void OnCollect(Batch<Metric> metrics)
{
this.exporter.Export(metrics);
}
Expand Down
17 changes: 2 additions & 15 deletions src/OpenTelemetry/Metrics/MeterProviderSdk.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ internal void MeasurementRecordedLong(Instrument instrument, long value, ReadOnl
metric.UpdateLong(value, tagsRos);
}

internal IEnumerable<Metric> Collect()
internal Batch<Metric> Collect()
{
lock (this.collectLock)
{
Expand All @@ -155,20 +155,7 @@ internal IEnumerable<Metric> Collect()
this.metrics[i].SnapShot();
}

return Iterate(this.metrics, indexSnapShot + 1);

// We cannot simply return the internal structure (array)
// as the user is not expected to navigate it.
// properly.
static IEnumerable<Metric> Iterate(Metric[] metrics, long targetCount)
{
for (int i = 0; i < targetCount; i++)
{
// Check if the Metric has valid
// entries and skip, if not.
yield return metrics[i];
}
}
return new Batch<Metric>(this.metrics, indexSnapShot + 1);
}
catch (Exception)
{
Expand Down
3 changes: 1 addition & 2 deletions src/OpenTelemetry/Metrics/MetricReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
// </copyright>

using System;
using System.Collections.Generic;

namespace OpenTelemetry.Metrics
{
Expand All @@ -30,7 +29,7 @@ public virtual void Collect()
this.OnCollect(metricsCollected);
}

public virtual void OnCollect(IEnumerable<Metric> metrics)
public virtual void OnCollect(Batch<Metric> metrics)
{
}

Expand Down
7 changes: 3 additions & 4 deletions src/OpenTelemetry/Metrics/PeriodicExportingMetricReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,19 @@
// </copyright>

using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;

namespace OpenTelemetry.Metrics
{
public class PeriodicExportingMetricReader : MetricReader
{
private readonly MetricExporter exporter;
private readonly BaseExporter<Metric> exporter;
private readonly Task exportTask;
private readonly CancellationTokenSource token;
private bool disposed;

public PeriodicExportingMetricReader(MetricExporter exporter, int exportIntervalMs)
public PeriodicExportingMetricReader(BaseExporter<Metric> exporter, int exportIntervalMs)
{
this.exporter = exporter;
this.token = new CancellationTokenSource();
Expand All @@ -46,7 +45,7 @@ public PeriodicExportingMetricReader(MetricExporter exporter, int exportInterval
this.exportTask.Start();
}

public override void OnCollect(IEnumerable<Metric> metrics)
public override void OnCollect(Batch<Metric> metrics)
{
this.exporter.Export(metrics);
}
Expand Down
2 changes: 1 addition & 1 deletion src/OpenTelemetry/ProviderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public static Resource GetResource(this BaseProvider baseProvider)
return Resource.Empty;
}

public static Func<IEnumerable<Metric>> GetMetricCollect(this BaseProvider baseProvider)
public static Func<Batch<Metric>> GetMetricCollect(this BaseProvider baseProvider)
{
if (baseProvider is MeterProviderSdk meterProviderSdk)
{
Expand Down
2 changes: 1 addition & 1 deletion test/Benchmarks/Benchmarks.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

<ItemGroup>
<Compile Include="$(RepoRoot)\test\OpenTelemetry.Tests\Shared\TestHttpServer.cs" Link="Includes\TestHttpServer.cs" />
<Compile Include="$(RepoRoot)\test\OpenTelemetry.Tests\Shared\TestMetricExporter.cs" Link="TestMetricExporter.cs" />
<Compile Include="$(RepoRoot)\test\OpenTelemetry.Tests\Shared\TestExporter.cs" Link="TestExporter.cs" />
</ItemGroup>

<ItemGroup>
Expand Down
4 changes: 2 additions & 2 deletions test/Benchmarks/Metrics/MetricCollectBenchmarks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ public class MetricCollectBenchmarks
[GlobalSetup]
public void Setup()
{
var metricExporter = new TestMetricExporter(ProcessExport);
void ProcessExport(IEnumerable<Metric> batch)
var metricExporter = new TestExporter<Metric>(ProcessExport);
void ProcessExport(Batch<Metric> batch)
{
double sum = 0;
foreach (var metric in batch)
Expand Down
Loading