Minor improvements to the Metrics SDK (#2515)

Co-authored-by: Cijo Thomas <cithomas@microsoft.com>

docs/metrics/extending-the-sdk/MyReader.cs

@@ -28,7 +28,7 @@ internal class MyReader : MetricReader
         this.name = name;
     }
 
-    protected override bool ProcessMetrics(Batch<Metric> metrics, int timeoutMilliseconds)
+    protected override bool ProcessMetrics(in Batch<Metric> metrics, int timeoutMilliseconds)
     {
         var sb = new StringBuilder();
         foreach (var record in metrics)

src/OpenTelemetry/Metrics/BaseExportingMetricReader.cs

@@ -79,7 +79,7 @@ namespace OpenTelemetry.Metrics
         }
 
         /// <inheritdoc/>
-        protected override bool ProcessMetrics(Batch<Metric> metrics, int timeoutMilliseconds)
+        protected override bool ProcessMetrics(in Batch<Metric> metrics, int timeoutMilliseconds)
         {
             // TODO: Do we need to consider timeout here?
             return this.exporter.Export(metrics) == ExportResult.Success;

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);
 
         /// <inheritdoc/>
-        protected override bool ProcessMetrics(Batch<Metric> metrics, int timeoutMilliseconds)
+        protected override bool ProcessMetrics(in Batch<Metric> metrics, int timeoutMilliseconds)
         {
             // CompositeMetricReader delegates the work to its underlying readers,
             // so CompositeMetricReader.ProcessMetrics should never be called.
-            throw new NotImplementedException();
+            throw new NotSupportedException();
         }
 
         /// <inheritdoc/>

src/OpenTelemetry/Metrics/MetricReader.cs

@@ -57,20 +57,24 @@ namespace OpenTelemetry.Metrics
 
         /// <summary>
         /// Attempts to collect the metrics, blocks the current thread until
-        /// metrics collection completed or timed out.
+        /// metrics collection completed, shutdown signaled or timed out.
         /// </summary>
         /// <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 <c>true</c> when metrics collection succeeded; otherwise,
+        /// <c>false</c>.
         /// </returns>
         /// <exception cref="ArgumentOutOfRangeException">
         /// Thrown when the <c>timeoutMilliseconds</c> is smaller than -1.
         /// </exception>
         /// <remarks>
-        /// 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 <c>OnCollect</c> callback for improved performance, as long as
+        /// the semantic can be preserved.
         /// </remarks>
         public bool Collect(int timeoutMilliseconds = Timeout.Infinite)
         {
@@ -182,7 +186,7 @@ namespace OpenTelemetry.Metrics
         /// Returns <c>true</c> when metrics processing succeeded; otherwise,
         /// <c>false</c>.
         /// </returns>
-        protected abstract bool ProcessMetrics(Batch<Metric> metrics, int timeoutMilliseconds);
+        protected abstract bool ProcessMetrics(in Batch<Metric> metrics, int timeoutMilliseconds);
 
         /// <summary>
         /// Called by <c>Collect</c>. This function should block the current
@@ -198,9 +202,8 @@ namespace OpenTelemetry.Metrics
         /// <c>false</c>.
         /// </returns>
         /// <remarks>
-        /// This function is called synchronously on the thread which called
-        /// <c>Collect</c>. This function should be thread-safe, and should
-        /// not throw exceptions.
+        /// This function is called synchronously on the threads which called
+        /// <c>Collect</c>. This function should not throw exceptions.
         /// </remarks>
         protected virtual bool OnCollect(int timeoutMilliseconds)
         {

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

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<Metric> metrics, int timeoutMilliseconds)
+            protected override bool ProcessMetrics(in Batch<Metric> metrics, int timeoutMilliseconds)
             {
                 return true;
             }