From b1fc7616b1cdb05e7b814c0f826bc2f3cf05df14 Mon Sep 17 00:00:00 2001 From: Reiley Yang Date: Tue, 26 Oct 2021 07:42:12 -0700 Subject: [PATCH] Minor improvements to the Metrics SDK (#2515) Co-authored-by: Cijo Thomas --- docs/metrics/extending-the-sdk/MyReader.cs | 2 +- .../Metrics/BaseExportingMetricReader.cs | 2 +- .../Metrics/CompositeMetricReader.cs | 6 +++--- src/OpenTelemetry/Metrics/MetricReader.cs | 17 ++++++++++------- .../Metrics/PeriodicExportingMetricReader.cs | 12 +++--------- .../HostingMeterExtensionTests.cs | 2 +- 6 files changed, 19 insertions(+), 22 deletions(-) diff --git a/docs/metrics/extending-the-sdk/MyReader.cs b/docs/metrics/extending-the-sdk/MyReader.cs index 25993314a..eef6b2b03 100644 --- a/docs/metrics/extending-the-sdk/MyReader.cs +++ b/docs/metrics/extending-the-sdk/MyReader.cs @@ -28,7 +28,7 @@ internal class MyReader : MetricReader this.name = name; } - protected override bool ProcessMetrics(Batch metrics, int timeoutMilliseconds) + protected override bool ProcessMetrics(in Batch metrics, int timeoutMilliseconds) { var sb = new StringBuilder(); foreach (var record in metrics) diff --git a/src/OpenTelemetry/Metrics/BaseExportingMetricReader.cs b/src/OpenTelemetry/Metrics/BaseExportingMetricReader.cs index ffd32d921..146ea4d81 100644 --- a/src/OpenTelemetry/Metrics/BaseExportingMetricReader.cs +++ b/src/OpenTelemetry/Metrics/BaseExportingMetricReader.cs @@ -79,7 +79,7 @@ namespace OpenTelemetry.Metrics } /// - protected override bool ProcessMetrics(Batch metrics, int timeoutMilliseconds) + protected override bool ProcessMetrics(in Batch metrics, int timeoutMilliseconds) { // TODO: Do we need to consider timeout here? return this.exporter.Export(metrics) == ExportResult.Success; diff --git a/src/OpenTelemetry/Metrics/CompositeMetricReader.cs b/src/OpenTelemetry/Metrics/CompositeMetricReader.cs index 4b82716cd..816e60f2a 100644 --- a/src/OpenTelemetry/Metrics/CompositeMetricReader.cs +++ b/src/OpenTelemetry/Metrics/CompositeMetricReader.cs @@ -22,7 +22,7 @@ using OpenTelemetry.Internal; namespace OpenTelemetry.Metrics { - internal class CompositeMetricReader : MetricReader + internal sealed class CompositeMetricReader : MetricReader { private readonly DoublyLinkedListNode head; private DoublyLinkedListNode tail; @@ -64,11 +64,11 @@ namespace OpenTelemetry.Metrics public Enumerator GetEnumerator() => new Enumerator(this.head); /// - protected override bool ProcessMetrics(Batch metrics, int timeoutMilliseconds) + protected override bool ProcessMetrics(in Batch metrics, int timeoutMilliseconds) { // CompositeMetricReader delegates the work to its underlying readers, // so CompositeMetricReader.ProcessMetrics should never be called. - throw new NotImplementedException(); + throw new NotSupportedException(); } /// diff --git a/src/OpenTelemetry/Metrics/MetricReader.cs b/src/OpenTelemetry/Metrics/MetricReader.cs index 2155cef67..fecc7e786 100644 --- a/src/OpenTelemetry/Metrics/MetricReader.cs +++ b/src/OpenTelemetry/Metrics/MetricReader.cs @@ -57,20 +57,24 @@ namespace OpenTelemetry.Metrics /// /// Attempts to collect the metrics, blocks the current thread until - /// metrics collection completed or timed out. + /// metrics collection completed, shutdown signaled or timed out. /// /// /// The number (non-negative) of milliseconds to wait, or /// Timeout.Infinite to wait indefinitely. /// /// - /// Returns true when metrics collection succeeded; otherwise, false. + /// Returns true when metrics collection succeeded; otherwise, + /// false. /// /// /// Thrown when the timeoutMilliseconds is smaller than -1. /// /// - /// This function guarantees thread-safety. + /// This function guarantees thread-safety. If multiple calls occurred + /// simultaneously, they might get folded and result in less calls to + /// the OnCollect callback for improved performance, as long as + /// the semantic can be preserved. /// public bool Collect(int timeoutMilliseconds = Timeout.Infinite) { @@ -182,7 +186,7 @@ namespace OpenTelemetry.Metrics /// Returns true when metrics processing succeeded; otherwise, /// false. /// - protected abstract bool ProcessMetrics(Batch metrics, int timeoutMilliseconds); + protected abstract bool ProcessMetrics(in Batch metrics, int timeoutMilliseconds); /// /// Called by Collect. This function should block the current @@ -198,9 +202,8 @@ namespace OpenTelemetry.Metrics /// false. /// /// - /// This function is called synchronously on the thread which called - /// Collect. This function should be thread-safe, and should - /// not throw exceptions. + /// This function is called synchronously on the threads which called + /// Collect. This function should not throw exceptions. /// protected virtual bool OnCollect(int timeoutMilliseconds) { diff --git a/src/OpenTelemetry/Metrics/PeriodicExportingMetricReader.cs b/src/OpenTelemetry/Metrics/PeriodicExportingMetricReader.cs index ff4fa02d9..67bc35d28 100644 --- a/src/OpenTelemetry/Metrics/PeriodicExportingMetricReader.cs +++ b/src/OpenTelemetry/Metrics/PeriodicExportingMetricReader.cs @@ -17,6 +17,7 @@ using System; using System.Diagnostics; using System.Threading; +using OpenTelemetry.Internal; namespace OpenTelemetry.Metrics { @@ -38,15 +39,8 @@ namespace OpenTelemetry.Metrics int exportTimeoutMilliseconds = DefaultExportTimeoutMilliseconds) : base(exporter) { - if (exportIntervalMilliseconds <= 0) - { - throw new ArgumentOutOfRangeException(nameof(exportIntervalMilliseconds), exportIntervalMilliseconds, "exportIntervalMilliseconds should be greater than zero."); - } - - if (exportTimeoutMilliseconds < 0) - { - throw new ArgumentOutOfRangeException(nameof(exportTimeoutMilliseconds), exportTimeoutMilliseconds, "exportTimeoutMilliseconds should be non-negative."); - } + Guard.Range(exportIntervalMilliseconds, nameof(exportIntervalMilliseconds), min: 1); + Guard.Range(exportTimeoutMilliseconds, nameof(exportTimeoutMilliseconds), min: 0); if ((this.SupportedExportModes & ExportModes.Push) != ExportModes.Push) { diff --git a/test/OpenTelemetry.Extensions.Hosting.Tests/HostingMeterExtensionTests.cs b/test/OpenTelemetry.Extensions.Hosting.Tests/HostingMeterExtensionTests.cs index 1c02b6eb1..1043e7f0d 100644 --- a/test/OpenTelemetry.Extensions.Hosting.Tests/HostingMeterExtensionTests.cs +++ b/test/OpenTelemetry.Extensions.Hosting.Tests/HostingMeterExtensionTests.cs @@ -229,7 +229,7 @@ namespace OpenTelemetry.Extensions.Hosting.Tests internal class TestReader : MetricReader { - protected override bool ProcessMetrics(Batch metrics, int timeoutMilliseconds) + protected override bool ProcessMetrics(in Batch metrics, int timeoutMilliseconds) { return true; }