Fix Samplers to match spec (#1037)

* Modified sampler to return Decision

* Added missing functionalites affected with conflict.

* Added test

* Fix test

* Removed IsAllDataRequested check from processor

* Checking if sampler is null.

* Adding code missed from conflict

* Incorporating PR feedback

* Fix test

* Removed OpenTelemetry.Trace.Samplers namespace

* Update to changelog

* Moved test to tracersdk

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

Author: rajkumar-rangaraj

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;
             }
         }
     }

…metry/Trace/Samplers/AlwaysOffSampler.cs …/OpenTelemetry/Trace/AlwaysOffSampler.cssrc/OpenTelemetry/Trace/Samplers/AlwaysOffSampler.cs renamed to src/OpenTelemetry/Trace/AlwaysOffSampler.cs src/OpenTelemetry/Trace/Samplers/AlwaysOffSampler.cs renamed to 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);
         }
     }
 }

…emetry/Trace/Samplers/AlwaysOnSampler.cs …c/OpenTelemetry/Trace/AlwaysOnSampler.cssrc/OpenTelemetry/Trace/Samplers/AlwaysOnSampler.cs renamed to src/OpenTelemetry/Trace/AlwaysOnSampler.cs src/OpenTelemetry/Trace/Samplers/AlwaysOnSampler.cs renamed to 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

@@ -139,40 +139,43 @@ public BatchingActivityProcessor(ActivityExporter exporter, int maxQueueSize, Ti
         /// <inheritdoc/>
         public override void OnEnd(Activity activity)
         {
-            // because of race-condition between checking the size and enqueueing,
-            // we might end up with a bit more activities than maxQueueSize.
-            // Let's just tolerate it to avoid extra synchronization.
-            if (this.currentQueueSize >= this.maxQueueSize)
+            if (activity.Recorded)
             {
-                OpenTelemetrySdkEventSource.Log.SpanProcessorQueueIsExhausted();
-                return;
-            }
+                // because of race-condition between checking the size and enqueueing,
+                // we might end up with a bit more activities than maxQueueSize.
+                // Let's just tolerate it to avoid extra synchronization.
+                if (this.currentQueueSize >= this.maxQueueSize)
+                {
+                    OpenTelemetrySdkEventSource.Log.SpanProcessorQueueIsExhausted();
+                    return;
+                }
 
-            var size = Interlocked.Increment(ref this.currentQueueSize);
+                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;
+                    }
                 }
             }
         }

…ry/Trace/Samplers/ParentOrElseSampler.cs …enTelemetry/Trace/ParentOrElseSampler.cssrc/OpenTelemetry/Trace/Samplers/ParentOrElseSampler.cs renamed to src/OpenTelemetry/Trace/ParentOrElseSampler.cs src/OpenTelemetry/Trace/Samplers/ParentOrElseSampler.cs renamed to 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);
         }
     }
 }

…try/Trace/Samplers/ProbabilitySampler.cs …penTelemetry/Trace/ProbabilitySampler.cssrc/OpenTelemetry/Trace/Samplers/ProbabilitySampler.cs renamed to src/OpenTelemetry/Trace/ProbabilitySampler.cs src/OpenTelemetry/Trace/Samplers/ProbabilitySampler.cs renamed to 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,
+    }
+}