From 80fe1e44be8430fcee5cfe6ec7382be814f47b4c Mon Sep 17 00:00:00 2001 From: Mohamed Asaker Date: Mon, 7 Oct 2024 16:11:21 -0700 Subject: [PATCH 1/6] Wrapped root sampler with ParentBasedSampler --- .../.publicApi/PublicAPI.Unshipped.txt | 2 +- .../AWSXRayRemoteSamplerBuilder.cs | 14 ++++++++++++-- src/OpenTelemetry.Sampler.AWS/FallbackSampler.cs | 4 ++-- .../SamplingRuleApplier.cs | 8 ++++---- .../TestAWSXRayRemoteSampler.cs | 6 +++--- 5 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/OpenTelemetry.Sampler.AWS/.publicApi/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Sampler.AWS/.publicApi/PublicAPI.Unshipped.txt index 063bdbf96d..2bd436bbb1 100644 --- a/src/OpenTelemetry.Sampler.AWS/.publicApi/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry.Sampler.AWS/.publicApi/PublicAPI.Unshipped.txt @@ -2,7 +2,7 @@ OpenTelemetry.Sampler.AWS.AWSXRayRemoteSampler OpenTelemetry.Sampler.AWS.AWSXRayRemoteSampler.Dispose() -> void OpenTelemetry.Sampler.AWS.AWSXRayRemoteSamplerBuilder -OpenTelemetry.Sampler.AWS.AWSXRayRemoteSamplerBuilder.Build() -> OpenTelemetry.Sampler.AWS.AWSXRayRemoteSampler! +OpenTelemetry.Sampler.AWS.AWSXRayRemoteSamplerBuilder.Build() -> OpenTelemetry.Trace.ParentBasedSampler! OpenTelemetry.Sampler.AWS.AWSXRayRemoteSamplerBuilder.SetEndpoint(string! endpoint) -> OpenTelemetry.Sampler.AWS.AWSXRayRemoteSamplerBuilder! OpenTelemetry.Sampler.AWS.AWSXRayRemoteSamplerBuilder.SetPollingInterval(System.TimeSpan pollingInterval) -> OpenTelemetry.Sampler.AWS.AWSXRayRemoteSamplerBuilder! override OpenTelemetry.Sampler.AWS.AWSXRayRemoteSampler.ShouldSample(in OpenTelemetry.Trace.SamplingParameters samplingParameters) -> OpenTelemetry.Trace.SamplingResult diff --git a/src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs b/src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs index 7d64254b7b..1715f8f7ad 100644 --- a/src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs +++ b/src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 using OpenTelemetry.Resources; +using OpenTelemetry.Trace; namespace OpenTelemetry.Sampler.AWS; @@ -66,8 +67,17 @@ public AWSXRayRemoteSamplerBuilder SetEndpoint(string endpoint) /// /// Returns a with configuration of this builder. /// - /// an instance of . - public AWSXRayRemoteSampler Build() + /// an instance of . + public ParentBasedSampler Build() + { + using var rootSampler = this.BuildXraySampler(); + return new ParentBasedSampler(rootSampler); + } + + // This is intended for testing to check that the XRayRemoteSampler is built with the correct attributes + // Should not be exposed to public as the public build method should return the sampler wrapped inside + // ParentBasedSampler. + internal AWSXRayRemoteSampler BuildXraySampler() { return new AWSXRayRemoteSampler(this.resource, this.pollingInterval, this.endpoint, this.clock); } diff --git a/src/OpenTelemetry.Sampler.AWS/FallbackSampler.cs b/src/OpenTelemetry.Sampler.AWS/FallbackSampler.cs index bf92a8ec97..66967bc3e2 100644 --- a/src/OpenTelemetry.Sampler.AWS/FallbackSampler.cs +++ b/src/OpenTelemetry.Sampler.AWS/FallbackSampler.cs @@ -14,8 +14,8 @@ internal class FallbackSampler : Trace.Sampler public FallbackSampler(Clock clock) { this.clock = clock; - this.reservoirSampler = new ParentBasedSampler(new RateLimitingSampler(1, clock)); - this.fixedRateSampler = new ParentBasedSampler(new TraceIdRatioBasedSampler(0.05)); + this.reservoirSampler = new RateLimitingSampler(1, clock); + this.fixedRateSampler = new TraceIdRatioBasedSampler(0.05); } public override SamplingResult ShouldSample(in SamplingParameters samplingParameters) diff --git a/src/OpenTelemetry.Sampler.AWS/SamplingRuleApplier.cs b/src/OpenTelemetry.Sampler.AWS/SamplingRuleApplier.cs index b5910499c5..8775455788 100644 --- a/src/OpenTelemetry.Sampler.AWS/SamplingRuleApplier.cs +++ b/src/OpenTelemetry.Sampler.AWS/SamplingRuleApplier.cs @@ -20,7 +20,7 @@ public SamplingRuleApplier(string clientId, Clock clock, SamplingRule rule, Stat { // Until calling GetSamplingTargets, the default is to borrow 1/s if reservoir size is // positive. - this.ReservoirSampler = new ParentBasedSampler(new RateLimitingSampler(1, this.Clock)); + this.ReservoirSampler = new RateLimitingSampler(1, this.Clock); this.Borrowing = true; } else @@ -30,7 +30,7 @@ public SamplingRuleApplier(string clientId, Clock clock, SamplingRule rule, Stat this.Borrowing = false; } - this.FixedRateSampler = new ParentBasedSampler(new TraceIdRatioBasedSampler(rule.FixedRate)); + this.FixedRateSampler = new TraceIdRatioBasedSampler(rule.FixedRate); // We either have no reservoir sampling or borrow until we get a quota so have no end time. this.ReservoirEndTime = DateTimeOffset.MaxValue; @@ -198,7 +198,7 @@ public SamplingStatisticsDocument Snapshot(DateTimeOffset now) public SamplingRuleApplier WithTarget(SamplingTargetDocument target, DateTimeOffset now) { Trace.Sampler newFixedRateSampler = target.FixedRate != null - ? new ParentBasedSampler(new TraceIdRatioBasedSampler(target.FixedRate.Value)) + ? new TraceIdRatioBasedSampler(target.FixedRate.Value) : this.FixedRateSampler; Trace.Sampler newReservoirSampler = new AlwaysOffSampler(); @@ -207,7 +207,7 @@ public SamplingRuleApplier WithTarget(SamplingTargetDocument target, DateTimeOff { if (target.ReservoirQuota > 0) { - newReservoirSampler = new ParentBasedSampler(new RateLimitingSampler(target.ReservoirQuota.Value, this.Clock)); + newReservoirSampler = new RateLimitingSampler(target.ReservoirQuota.Value, this.Clock); } else { diff --git a/test/OpenTelemetry.Sampler.AWS.Tests/TestAWSXRayRemoteSampler.cs b/test/OpenTelemetry.Sampler.AWS.Tests/TestAWSXRayRemoteSampler.cs index 3c629e79c6..834e372ab0 100644 --- a/test/OpenTelemetry.Sampler.AWS.Tests/TestAWSXRayRemoteSampler.cs +++ b/test/OpenTelemetry.Sampler.AWS.Tests/TestAWSXRayRemoteSampler.cs @@ -22,7 +22,7 @@ public void TestSamplerWithConfiguration() AWSXRayRemoteSampler sampler = AWSXRayRemoteSampler.Builder(ResourceBuilder.CreateEmpty().Build()) .SetPollingInterval(pollingInterval) .SetEndpoint(endpoint) - .Build(); + .BuildXraySampler(); Assert.Equal(pollingInterval, sampler.PollingInterval); Assert.Equal(endpoint, sampler.Endpoint); @@ -33,7 +33,7 @@ public void TestSamplerWithConfiguration() [Fact] public void TestSamplerWithDefaults() { - AWSXRayRemoteSampler sampler = AWSXRayRemoteSampler.Builder(ResourceBuilder.CreateEmpty().Build()).Build(); + AWSXRayRemoteSampler sampler = AWSXRayRemoteSampler.Builder(ResourceBuilder.CreateEmpty().Build()).BuildXraySampler(); Assert.Equal(TimeSpan.FromMinutes(5), sampler.PollingInterval); Assert.Equal("http://localhost:2000", sampler.Endpoint); @@ -53,7 +53,7 @@ public void TestSamplerUpdateAndSample() .SetPollingInterval(TimeSpan.FromMilliseconds(10)) .SetEndpoint(mockServer.Url!) .SetClock(clock) - .Build(); + .BuildXraySampler(); // the sampler will use fallback sampler until rules are fetched. Assert.Equal(SamplingDecision.RecordAndSample, this.DoSample(sampler, "cat-service")); From 33ca3b4d63920102bed81642284e8bdebaada81b Mon Sep 17 00:00:00 2001 From: Mohamed Asaker Date: Tue, 8 Oct 2024 15:49:29 -0700 Subject: [PATCH 2/6] updated the return type to sampler instead of parentBasedSampler --- .../.publicApi/PublicAPI.Unshipped.txt | 2 +- src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry.Sampler.AWS/.publicApi/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Sampler.AWS/.publicApi/PublicAPI.Unshipped.txt index 2bd436bbb1..96770ae2fa 100644 --- a/src/OpenTelemetry.Sampler.AWS/.publicApi/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry.Sampler.AWS/.publicApi/PublicAPI.Unshipped.txt @@ -2,7 +2,7 @@ OpenTelemetry.Sampler.AWS.AWSXRayRemoteSampler OpenTelemetry.Sampler.AWS.AWSXRayRemoteSampler.Dispose() -> void OpenTelemetry.Sampler.AWS.AWSXRayRemoteSamplerBuilder -OpenTelemetry.Sampler.AWS.AWSXRayRemoteSamplerBuilder.Build() -> OpenTelemetry.Trace.ParentBasedSampler! +OpenTelemetry.Sampler.AWS.AWSXRayRemoteSamplerBuilder.Build() -> OpenTelemetry.Trace.Sampler! OpenTelemetry.Sampler.AWS.AWSXRayRemoteSamplerBuilder.SetEndpoint(string! endpoint) -> OpenTelemetry.Sampler.AWS.AWSXRayRemoteSamplerBuilder! OpenTelemetry.Sampler.AWS.AWSXRayRemoteSamplerBuilder.SetPollingInterval(System.TimeSpan pollingInterval) -> OpenTelemetry.Sampler.AWS.AWSXRayRemoteSamplerBuilder! override OpenTelemetry.Sampler.AWS.AWSXRayRemoteSampler.ShouldSample(in OpenTelemetry.Trace.SamplingParameters samplingParameters) -> OpenTelemetry.Trace.SamplingResult diff --git a/src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs b/src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs index 1715f8f7ad..30dd52c996 100644 --- a/src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs +++ b/src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs @@ -67,8 +67,8 @@ public AWSXRayRemoteSamplerBuilder SetEndpoint(string endpoint) /// /// Returns a with configuration of this builder. /// - /// an instance of . - public ParentBasedSampler Build() + /// an instance of . + public Trace.Sampler Build() { using var rootSampler = this.BuildXraySampler(); return new ParentBasedSampler(rootSampler); From a075ffec8311d9478ea7c37aad9cba72310d91da Mon Sep 17 00:00:00 2001 From: Mohamed Asaker Date: Fri, 15 Nov 2024 14:33:05 -0800 Subject: [PATCH 3/6] Updated changelog --- src/OpenTelemetry.Sampler.AWS/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md b/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md index 6c3514b50c..5ac958eff4 100644 --- a/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md +++ b/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md @@ -17,6 +17,8 @@ * Updated OpenTelemetry core component version(s) to `1.10.0`. ([#2317](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2317)) +* `AWSXRayRemoteSamplerBuilder.Build()` now returns `ParentBasedSampler` which is of type `Trace.Sampler` instead of `AWSXRayRemoteSampler` + ## 0.1.0-alpha.2 Released 2024-Sep-09 From ee41abc4d2f20c8f32a12281303c921fce222004 Mon Sep 17 00:00:00 2001 From: Mohamed Asaker Date: Mon, 18 Nov 2024 11:09:01 -0800 Subject: [PATCH 4/6] applied comments --- .../AWSXRayRemoteSamplerBuilder.cs | 10 +----- src/OpenTelemetry.Sampler.AWS/CHANGELOG.md | 3 +- .../TestAWSXRayRemoteSampler.cs | 34 ++++++++++++------- 3 files changed, 24 insertions(+), 23 deletions(-) diff --git a/src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs b/src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs index 30dd52c996..7f2ad35bee 100644 --- a/src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs +++ b/src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs @@ -70,18 +70,10 @@ public AWSXRayRemoteSamplerBuilder SetEndpoint(string endpoint) /// an instance of . public Trace.Sampler Build() { - using var rootSampler = this.BuildXraySampler(); + var rootSampler = new AWSXRayRemoteSampler(this.resource, this.pollingInterval, this.endpoint, this.clock); return new ParentBasedSampler(rootSampler); } - // This is intended for testing to check that the XRayRemoteSampler is built with the correct attributes - // Should not be exposed to public as the public build method should return the sampler wrapped inside - // ParentBasedSampler. - internal AWSXRayRemoteSampler BuildXraySampler() - { - return new AWSXRayRemoteSampler(this.resource, this.pollingInterval, this.endpoint, this.clock); - } - // This is intended for testing with a mock clock. // Should not be exposed to public. internal AWSXRayRemoteSamplerBuilder SetClock(Clock clock) diff --git a/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md b/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md index 5ac958eff4..923882871b 100644 --- a/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md +++ b/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md @@ -17,7 +17,8 @@ * Updated OpenTelemetry core component version(s) to `1.10.0`. ([#2317](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2317)) -* `AWSXRayRemoteSamplerBuilder.Build()` now returns `ParentBasedSampler` which is of type `Trace.Sampler` instead of `AWSXRayRemoteSampler` +* `AWSXRayRemoteSamplerBuilder.Build()` now returns `ParentBasedSampler` which is of +type `Trace.Sampler` instead of `AWSXRayRemoteSampler` ## 0.1.0-alpha.2 diff --git a/test/OpenTelemetry.Sampler.AWS.Tests/TestAWSXRayRemoteSampler.cs b/test/OpenTelemetry.Sampler.AWS.Tests/TestAWSXRayRemoteSampler.cs index 86539d6c00..5d0c0bbc04 100644 --- a/test/OpenTelemetry.Sampler.AWS.Tests/TestAWSXRayRemoteSampler.cs +++ b/test/OpenTelemetry.Sampler.AWS.Tests/TestAWSXRayRemoteSampler.cs @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 using System.Diagnostics; +using System.Reflection; using OpenTelemetry.Resources; using OpenTelemetry.Trace; using WireMock.RequestBuilders; @@ -18,27 +19,34 @@ public void TestSamplerWithConfiguration() { var pollingInterval = TimeSpan.FromSeconds(5); var endpoint = "http://localhost:3000"; - - var sampler = AWSXRayRemoteSampler.Builder(ResourceBuilder.CreateEmpty().Build()) + var parentBasedSampler = AWSXRayRemoteSampler.Builder(ResourceBuilder.CreateEmpty().Build()) .SetPollingInterval(pollingInterval) .SetEndpoint(endpoint) - .BuildXraySampler(); + .Build(); + + FieldInfo? rootSamplerFieldInfo = typeof(ParentBasedSampler).GetField("rootSampler", BindingFlags.NonPublic | BindingFlags.Instance); - Assert.Equal(pollingInterval, sampler.PollingInterval); - Assert.Equal(endpoint, sampler.Endpoint); - Assert.NotNull(sampler.RulePollerTimer); - Assert.NotNull(sampler.Client); + var xraySampler = (AWSXRayRemoteSampler?)rootSamplerFieldInfo?.GetValue(parentBasedSampler); + + Assert.Equal(pollingInterval, xraySampler?.PollingInterval); + Assert.Equal(endpoint, xraySampler?.Endpoint); + Assert.NotNull(xraySampler?.RulePollerTimer); + Assert.NotNull(xraySampler?.Client); } [Fact] public void TestSamplerWithDefaults() { - var sampler = AWSXRayRemoteSampler.Builder(ResourceBuilder.CreateEmpty().Build()).BuildXraySampler(); + var parentBasedSampler = AWSXRayRemoteSampler.Builder(ResourceBuilder.CreateEmpty().Build()).Build(); + + FieldInfo? rootSamplerFieldInfo = typeof(ParentBasedSampler).GetField("rootSampler", BindingFlags.NonPublic | BindingFlags.Instance); + + var xraySampler = (AWSXRayRemoteSampler?)rootSamplerFieldInfo?.GetValue(parentBasedSampler); - Assert.Equal(TimeSpan.FromMinutes(5), sampler.PollingInterval); - Assert.Equal("http://localhost:2000", sampler.Endpoint); - Assert.NotNull(sampler.RulePollerTimer); - Assert.NotNull(sampler.Client); + Assert.Equal(TimeSpan.FromMinutes(5), xraySampler?.PollingInterval); + Assert.Equal("http://localhost:2000", xraySampler?.Endpoint); + Assert.NotNull(xraySampler?.RulePollerTimer); + Assert.NotNull(xraySampler?.Client); } [Fact(Skip = "Flaky test. Related issue: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/issues/1219")] @@ -53,7 +61,7 @@ public void TestSamplerUpdateAndSample() .SetPollingInterval(TimeSpan.FromMilliseconds(10)) .SetEndpoint(mockServer.Url!) .SetClock(clock) - .BuildXraySampler(); + .Build(); // the sampler will use fallback sampler until rules are fetched. Assert.Equal(SamplingDecision.RecordAndSample, this.DoSample(sampler, "cat-service")); From ed35468b8bda09b08620953401da465291df8fba Mon Sep 17 00:00:00 2001 From: Mohamed Asaker Date: Mon, 18 Nov 2024 11:17:40 -0800 Subject: [PATCH 5/6] updated changelog --- src/OpenTelemetry.Sampler.AWS/CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md b/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md index 923882871b..6c82738e7f 100644 --- a/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md +++ b/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md @@ -17,8 +17,8 @@ * Updated OpenTelemetry core component version(s) to `1.10.0`. ([#2317](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2317)) -* `AWSXRayRemoteSamplerBuilder.Build()` now returns `ParentBasedSampler` which is of -type `Trace.Sampler` instead of `AWSXRayRemoteSampler` +* `AWSXRayRemoteSamplerBuilder.Build()` now returns `ParentBasedSampler` which + which is of type `Trace.Sampler` instead of `AWSXRayRemoteSampler` ## 0.1.0-alpha.2 From 56e5f14ac9c14beef6fa984f4e58b29644ad160e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Kie=C5=82kowicz?= Date: Mon, 18 Nov 2024 20:33:11 +0100 Subject: [PATCH 6/6] Update src/OpenTelemetry.Sampler.AWS/CHANGELOG.md --- src/OpenTelemetry.Sampler.AWS/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md b/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md index 6c82738e7f..6a78f967c5 100644 --- a/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md +++ b/src/OpenTelemetry.Sampler.AWS/CHANGELOG.md @@ -18,7 +18,8 @@ ([#2317](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2317)) * `AWSXRayRemoteSamplerBuilder.Build()` now returns `ParentBasedSampler` which - which is of type `Trace.Sampler` instead of `AWSXRayRemoteSampler` + which is of type `Sampler` instead of `AWSXRayRemoteSampler`. + ([#2188](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2188)) ## 0.1.0-alpha.2