Fix Samplers to match spec

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

@@ -20,6 +20,6 @@ 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
+  ([#1037](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1037))
 * Changed `ActivityProcessor` to implement `IDisposable`
   ([#975](https://github.com/open-telemetry/opentelemetry-dotnet/pull/975))
 * Samplers now get the actual TraceId of the Activity to be created.

src/OpenTelemetry/README.md

@@ -64,7 +64,6 @@ with sampling probability of 25%.
 ```csharp
 using OpenTelemetry;
 using OpenTelemetry.Trace;
-using OpenTelemetry.Trace.Samplers;
 
 using var otel = Sdk.CreateTracerProvider(b => b
     .AddActivitySource("MyCompany.MyProduct.MyLibrary")

src/OpenTelemetry/Trace/ActivitySourceAdapter.cs

@@ -100,11 +100,20 @@ private void RunGetRequestedData(Activity activity)
                 activity.TagObjects,
                 activity.Links);
 
-            var samplingDecision = this.sampler.ShouldSample(samplingParameters);
-            activity.IsAllDataRequested = samplingDecision.IsSampled;
-            if (samplingDecision.IsSampled)
+            var samplingResult = this.sampler.ShouldSample(samplingParameters);
+
+            switch (samplingResult.Decision)
             {
-                activity.ActivityTraceFlags |= ActivityTraceFlags.Recorded;
+                case SamplingDecision.NotRecord:
+                    activity.IsAllDataRequested = false;
+                    break;
+                case SamplingDecision.Record:
+                    activity.IsAllDataRequested = true;
+                    break;
+                case SamplingDecision.RecordAndSampled:
+                    activity.IsAllDataRequested = true;
+                    activity.ActivityTraceFlags |= ActivityTraceFlags.Recorded;
+                    break;
             }
         }
     }

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

@@ -14,7 +14,7 @@
 // limitations under the License.
 // </copyright>
 
-namespace OpenTelemetry.Trace.Samplers
+namespace OpenTelemetry.Trace
 {
     /// <summary>
     /// Sampler implementation which never samples any activity.
@@ -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 → src/OpenTelemetry/Trace/AlwaysOnSampler.cs

@@ -14,7 +14,7 @@
 // limitations under the License.
 // </copyright>
 
-namespace OpenTelemetry.Trace.Samplers
+namespace OpenTelemetry.Trace
 {
     /// <summary>
     /// Sampler implementation which samples every activity.
@@ -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/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/ParentOrElseSampler.cs → src/OpenTelemetry/Trace/ParentOrElseSampler.cs

@@ -16,7 +16,7 @@
 using System;
 using System.Diagnostics;
 
-namespace OpenTelemetry.Trace.Samplers
+namespace OpenTelemetry.Trace
 {
     /// <summary>
     /// Sampler implementation which will take a sample if parent Activity or any linked Activity is sampled.
@@ -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 → src/OpenTelemetry/Trace/ProbabilitySampler.cs

@@ -16,7 +16,7 @@
 using System;
 using System.Globalization;
 
-namespace OpenTelemetry.Trace.Samplers
+namespace OpenTelemetry.Trace
 {
     /// <summary>
     /// Samples traces according to the specified probability.
@@ -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 new SamplingResult(Math.Abs(this.GetLowerLong(traceIdBytes)) < this.idUpperBound);
         }
 
         private long GetLowerLong(ReadOnlySpan<byte> bytes)

src/OpenTelemetry/Trace/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
+{
+    /// <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

@@ -13,6 +13,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 // </copyright>
+
 using System.Collections.Generic;
 using System.Diagnostics;
 

src/OpenTelemetry/Trace/SamplingResult.cs

@@ -13,6 +13,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 // </copyright>
+
 using System.Collections.Generic;
 using System.Linq;
 
@@ -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)
             {