Fix Samplers to match spec

docs/trace/building-your-own-sampler/MySampler.cs

@@ -14,12 +14,12 @@
 // limitations under the License.
 // </copyright>
 
-using OpenTelemetry.Trace;
+using OpenTelemetry.Trace.Samplers;
 
 internal class MySampler : Sampler
 {
     public override SamplingResult ShouldSample(in SamplingParameters samplingParameters)
     {
-        return new SamplingResult(false);
+        return new SamplingResult(SamplingDecision.NotRecord);
     }
 }

src/OpenTelemetry/CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+* Modified Sampler implementation to match the
+  [spec](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#sampler)
 * Renaming `BroadcastActivityProcessor` to `FanOutActivityProcessor` (#1015)
 * Introduce `SuppressInstrumentationScope` API (#988).
 * `ActivityProcessor` implements `IDisposable`.

src/OpenTelemetry/Trace/ActivitySourceAdapter.cs

@@ -16,6 +16,7 @@
 
 using System.Diagnostics;
 using OpenTelemetry.Resources;
+using OpenTelemetry.Trace.Samplers;
 
 namespace OpenTelemetry.Trace
 {
@@ -101,8 +102,9 @@ private void RunGetRequestedData(Activity activity)
                 activity.Links);
 
             var samplingDecision = this.sampler.ShouldSample(samplingParameters);
-            activity.IsAllDataRequested = samplingDecision.IsSampled;
-            if (samplingDecision.IsSampled)
+            var isSampled = samplingDecision.Decision == SamplingDecision.Record || samplingDecision.Decision == SamplingDecision.RecordAndSampled;
+            activity.IsAllDataRequested = isSampled;
+            if (samplingDecision.Decision == SamplingDecision.RecordAndSampled)
             {
                 activity.ActivityTraceFlags |= ActivityTraceFlags.Recorded;
             }

src/OpenTelemetry/Trace/BatchingActivityProcessor.cs

@@ -148,31 +148,34 @@ public override void OnEnd(Activity activity)
                 return;
             }
 
-            var size = Interlocked.Increment(ref this.currentQueueSize);
+            if (activity.Recorded)
+            {
+                var size = Interlocked.Increment(ref this.currentQueueSize);
 
-            this.exportQueue.Enqueue(activity);
+                this.exportQueue.Enqueue(activity);
 
-            if (size >= this.maxExportBatchSize)
-            {
-                bool lockTaken = this.flushLock.Wait(0);
-                if (lockTaken)
+                if (size >= this.maxExportBatchSize)
                 {
-                    Task.Run(async () =>
+                    bool lockTaken = this.flushLock.Wait(0);
+                    if (lockTaken)
                     {
-                        try
-                        {
-                            await this.FlushAsyncInternal(drain: false, lockAlreadyHeld: true, CancellationToken.None).ConfigureAwait(false);
-                        }
-                        catch (Exception ex)
-                        {
-                            OpenTelemetrySdkEventSource.Log.SpanProcessorException(nameof(this.OnEnd), ex);
-                        }
-                        finally
+                        Task.Run(async () =>
                         {
-                            this.flushLock.Release();
-                        }
-                    });
-                    return;
+                            try
+                            {
+                                await this.FlushAsyncInternal(drain: false, lockAlreadyHeld: true, CancellationToken.None).ConfigureAwait(false);
+                            }
+                            catch (Exception ex)
+                            {
+                                OpenTelemetrySdkEventSource.Log.SpanProcessorException(nameof(this.OnEnd), ex);
+                            }
+                            finally
+                            {
+                                this.flushLock.Release();
+                            }
+                        });
+                        return;
+                    }
                 }
             }
         }

src/OpenTelemetry/Trace/Samplers/AlwaysOffSampler.cs

@@ -24,7 +24,7 @@ public sealed class AlwaysOffSampler : Sampler
         /// <inheritdoc />
         public override SamplingResult ShouldSample(in SamplingParameters samplingParameters)
         {
-            return new SamplingResult(false);
+            return new SamplingResult(SamplingDecision.NotRecord);
         }
     }
 }

src/OpenTelemetry/Trace/Samplers/AlwaysOnSampler.cs

@@ -25,7 +25,7 @@ public sealed class AlwaysOnSampler : Sampler
         /// <inheritdoc />
         public override SamplingResult ShouldSample(in SamplingParameters samplingParameters)
         {
-            return new SamplingResult(true);
+            return new SamplingResult(SamplingDecision.RecordAndSampled);
         }
     }
 }

src/OpenTelemetry/Trace/Samplers/ParentOrElseSampler.cs

@@ -51,7 +51,7 @@ public override SamplingResult ShouldSample(in SamplingParameters samplingParame
             // If the parent is sampled keep the sampling decision.
             if ((parentContext.TraceFlags & ActivityTraceFlags.Recorded) != 0)
             {
-                return new SamplingResult(true);
+                return new SamplingResult(SamplingDecision.RecordAndSampled);
             }
 
             if (samplingParameters.Links != null)
@@ -61,13 +61,13 @@ public override SamplingResult ShouldSample(in SamplingParameters samplingParame
                 {
                     if ((parentLink.Context.TraceFlags & ActivityTraceFlags.Recorded) != 0)
                     {
-                        return new SamplingResult(true);
+                        return new SamplingResult(SamplingDecision.RecordAndSampled);
                     }
                 }
             }
 
             // If parent was not sampled, do not sample.
-            return new SamplingResult(false);
+            return new SamplingResult(SamplingDecision.NotRecord);
         }
     }
 }

src/OpenTelemetry/Trace/Samplers/ProbabilitySampler.cs

@@ -74,7 +74,7 @@ public override SamplingResult ShouldSample(in SamplingParameters samplingParame
             // code is executed in-line for every Activity creation).
             Span<byte> traceIdBytes = stackalloc byte[16];
             samplingParameters.TraceId.CopyTo(traceIdBytes);
-            return Math.Abs(this.GetLowerLong(traceIdBytes)) < this.idUpperBound ? new SamplingResult(true) : new SamplingResult(false);
+            return Math.Abs(this.GetLowerLong(traceIdBytes)) < this.idUpperBound ? new SamplingResult(SamplingDecision.RecordAndSampled) : new SamplingResult(SamplingDecision.NotRecord);
         }
 
         private long GetLowerLong(ReadOnlySpan<byte> bytes)

src/OpenTelemetry/Trace/Sampler.cs → src/OpenTelemetry/Trace/Samplers/Sampler.cs

@@ -15,7 +15,7 @@
 // </copyright>
 using System.Diagnostics;
 
-namespace OpenTelemetry.Trace
+namespace OpenTelemetry.Trace.Samplers
 {
     /// <summary>
     /// Sampler to select data to be exported. This sampler executes before Activity object is created.

src/OpenTelemetry/Trace/Samplers/SamplingDecision.cs

@@ -0,0 +1,43 @@
+// <copyright file="SamplingDecision.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>
+
+namespace OpenTelemetry.Trace.Samplers
+{
+    /// <summary>
+    /// Enumeration to define sampling decision.
+    /// </summary>
+    public enum SamplingDecision
+    {
+        /// <summary>
+        /// The activity object needs to be created. It will have Name, Source, Id and Baggage.
+        /// Other properties will be ignored.
+        /// </summary>
+        NotRecord,
+
+        /// <summary>
+        /// The activity object should be populated with all the propagation info and also all other
+        /// properties such as Links, Tags, and Events. Activity.IsAllDataRequested will return true.
+        /// </summary>
+        Record,
+
+        /// <summary>
+        /// The activity object should be populated with all the propagation info and also all other
+        /// properties such as Links, Tags, and Events.
+        /// Both Activity.IsAllDataRequested and Activity.IsRecorded will return true.
+        /// </summary>
+        RecordAndSampled,
+    }
+}

src/OpenTelemetry/Trace/SamplingParameters.cs → src/OpenTelemetry/Trace/Samplers/SamplingParameters.cs

@@ -13,10 +13,11 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 // </copyright>
+
 using System.Collections.Generic;
 using System.Diagnostics;
 
-namespace OpenTelemetry.Trace
+namespace OpenTelemetry.Trace.Samplers
 {
     /// <summary>
     /// Sampling parameters passed to an <see cref="Sampler"/> for it to make a sampling decision.

src/OpenTelemetry/Trace/SamplingResult.cs → src/OpenTelemetry/Trace/Samplers/SamplingResult.cs

@@ -13,10 +13,11 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 // </copyright>
+
 using System.Collections.Generic;
 using System.Linq;
 
-namespace OpenTelemetry.Trace
+namespace OpenTelemetry.Trace.Samplers
 {
     /// <summary>
     /// Sampling decision.
@@ -26,22 +27,32 @@ public readonly struct SamplingResult
         /// <summary>
         /// Initializes a new instance of the <see cref="SamplingResult"/> struct.
         /// </summary>
-        /// <param name="isSampled">True if sampled, false otherwise.</param>
+        /// <param name="decision"> indicates whether an activity object is recorded and sampled.</param>
+        public SamplingResult(SamplingDecision decision)
+        {
+            this.Decision = decision;
+            this.Attributes = Enumerable.Empty<KeyValuePair<string, object>>();
+        }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="SamplingResult"/> struct.
+        /// </summary>
+        /// <param name="isSampled"> True if sampled, false otherwise.</param>
         public SamplingResult(bool isSampled)
         {
-            this.IsSampled = isSampled;
+            this.Decision = isSampled ? SamplingDecision.RecordAndSampled : SamplingDecision.NotRecord;
             this.Attributes = Enumerable.Empty<KeyValuePair<string, object>>();
         }
 
         /// <summary>
         /// Initializes a new instance of the <see cref="SamplingResult"/> struct.
         /// </summary>
-        /// <param name="isSampled">True if sampled, false otherwise.</param>
+        /// <param name="decision">indicates whether an activity object is recorded and sampled.</param>
         /// <param name="attributes">Attributes associated with the sampling decision. Attributes list passed to
         /// this method must be immutable. Mutations of the collection and/or attribute values may lead to unexpected behavior.</param>
-        public SamplingResult(bool isSampled, IEnumerable<KeyValuePair<string, object>> attributes)
+        public SamplingResult(SamplingDecision decision, IEnumerable<KeyValuePair<string, object>> attributes)
         {
-            this.IsSampled = isSampled;
+            this.Decision = decision;
 
             // Note: Decision object takes ownership of the collection.
             // Current implementation has no means to ensure the collection will not be modified by the caller.
@@ -50,10 +61,9 @@ public SamplingResult(bool isSampled, IEnumerable<KeyValuePair<string, object>>
         }
 
         /// <summary>
-        /// Gets a value indicating whether Span was sampled or not.
-        /// The value is not suppose to change over time and can be cached.
+        /// Gets a value indicating indicates whether an activity object is recorded and sampled.
         /// </summary>
-        public bool IsSampled { get; }
+        public SamplingDecision Decision { get; }
 
         /// <summary>
         /// Gets a map of attributes associated with the sampling decision.
@@ -83,14 +93,14 @@ public override bool Equals(object obj)
             }
 
             var that = (SamplingResult)obj;
-            return this.IsSampled == that.IsSampled && this.Attributes == that.Attributes;
+            return this.Decision == that.Decision && this.Attributes == that.Attributes;
         }
 
         /// <inheritdoc/>
         public override int GetHashCode()
         {
             var result = 1;
-            result = (31 * result) + this.IsSampled.GetHashCode();
+            result = (31 * result) + this.Decision.GetHashCode();
             result = (31 * result) + this.Attributes.GetHashCode();
             return result;
         }

src/OpenTelemetry/Trace/SimpleActivityProcessor.cs

@@ -43,10 +43,12 @@ public override void OnEnd(Activity activity)
         {
             try
             {
-                // do not await, just start export
-                // it can still throw in synchronous part
-
-                _ = this.exporter.ExportAsync(new[] { activity }, CancellationToken.None);
+                if (activity.Recorded)
+                {
+                    // do not await, just start export
+                    // it can still throw in synchronous part
+                    _ = this.exporter.ExportAsync(new[] { activity }, CancellationToken.None);
+                }
             }
             catch (Exception ex)
             {

src/OpenTelemetry/Trace/TracerProviderBuilder.cs

@@ -67,21 +67,21 @@ public TracerProviderBuilder SetResource(Resource resource)
         /// <param name="names">Activity source names.</param>
         /// <returns>Returns <see cref="TracerProviderBuilder"/> for chaining.</returns>
         public TracerProviderBuilder AddSource(params string[] names)
-        {
-            if (names == null)
-            {
-                throw new ArgumentNullException(nameof(names));
+        {
+            if (names == null)
+            {
+                throw new ArgumentNullException(nameof(names));
             }
 
             foreach (var name in names)
-            {
-                if (string.IsNullOrWhiteSpace(name))
-                {
-                    throw new ArgumentException($"{nameof(names)} contains null or whitespace string.");
-                }
-
-                // TODO: We need to fix the listening model.
-                // Today it ignores version.
+            {
+                if (string.IsNullOrWhiteSpace(name))
+                {
+                    throw new ArgumentException($"{nameof(names)} contains null or whitespace string.");
+                }
+
+                // TODO: We need to fix the listening model.
+                // Today it ignores version.
                 this.sources.Add(name);
             }
 
@@ -94,10 +94,10 @@ public TracerProviderBuilder AddSource(params string[] names)
         /// <param name="activityProcessor">Activity processor to add.</param>
         /// <returns>Returns <see cref="TracerProviderBuilder"/> for chaining.</returns>
         public TracerProviderBuilder AddProcessor(ActivityProcessor activityProcessor)
-        {
-            if (activityProcessor == null)
-            {
-                throw new ArgumentNullException(nameof(activityProcessor));
+        {
+            if (activityProcessor == null)
+            {
+                throw new ArgumentNullException(nameof(activityProcessor));
             }
 
             this.processors.Add(activityProcessor);
@@ -136,7 +136,7 @@ public TracerProviderBuilder AddInstrumentation<TInstrumentation>(
 
         public TracerProvider Build()
         {
-            return new TracerProviderSdk(this.sources, this.processors, this.InstrumentationFactories, this.sampler, this.resource);
+            return new TracerProviderSdk(this.sources, this.processors, this.InstrumentationFactories, this.sampler, this.resource);
         }
 
         internal readonly struct InstrumentationFactory