[Sampler.AWS] Part-2: Add rules cache and rule matching logic #1124

srprash · 2023-03-30T17:48:42Z

This PR is a continuation of building the AWS X-Ray Remote Sampler.
See part 1 PR: #1091

Changes

Adding a Sampling Rules Cache.
- This cache holds the sampling rules which are periodically fetched from the X-Ray service.
- When a new set of sampling rules are fetched, the stateful properties like the Reservoir and the Statistics are preserved from the old set of rules. These stateful properties will be used in making the sampling decisions and keeping the sampling records respectively throughout the duration of the application. Note: The Reservoir and Statistics classes are very bare-bone right now, but will be developed in further PRs.
- The cache is also responsible for finding the sampling rule that matched a particular request. This sampling rule will be used to make the final sampling decision.
Adding the sampling rule matching logic.
- When an activity/span is created, the sampler will try and match a sampling rule based on the attributes like service name, http method, host, etc.
- A rule definition can specify matching of these attributes using wildcard characters.
The sampler now requires an OpenTelemetry Resource object.
- The sampler allows users to specify service name and service type (for example AWS::EC2::Instance) in their sampling rule to control sampling on a service level. These attributes are currently obtainable from the OTel SDK Resource configured for the application.
- There is an open ask to make the OTel SDK Resource available to samplers. In the meantime, the users will have to provide the Resource instance to the sampler themselves.

codecov · 2023-03-30T20:30:56Z

Codecov Report

Merging #1124 (76a7c1b) into main (4cc8429) will increase coverage by 0.27%.
The diff coverage is 84.82%.

❗ Current head 76a7c1b differs from pull request most recent head 9289865. Consider uploading reports for the commit 9289865 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1124      +/-   ##
==========================================
+ Coverage   72.53%   72.80%   +0.27%     
==========================================
  Files         236      243       +7     
  Lines        8570     8781     +211     
==========================================
+ Hits         6216     6393     +177     
- Misses       2354     2388      +34

Impacted Files	Coverage Δ
.../OpenTelemetry.Sampler.AWS/AWSXRaySamplerClient.cs	`88.09% <ø> (ø)`
src/OpenTelemetry.Sampler.AWS/SystemClock.cs	`46.15% <46.15%> (ø)`
...lemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs	`73.68% <50.00%> (-18.63%)`	⬇️
src/OpenTelemetry.Sampler.AWS/RulesCache.cs	`72.54% <72.54%> (ø)`
.../OpenTelemetry.Sampler.AWS/AWSXRayRemoteSampler.cs	`73.33% <88.23%> (+11.42%)`	⬆️
...c/OpenTelemetry.Sampler.AWS/SamplingRuleApplier.cs	`92.98% <92.98%> (ø)`
src/OpenTelemetry.Sampler.AWS/Matcher.cs	`97.91% <97.91%> (ø)`
src/OpenTelemetry.Sampler.AWS/Clock.cs	`100.00% <100.00%> (ø)`
src/OpenTelemetry.Sampler.AWS/FallbackSampler.cs	`100.00% <100.00%> (ø)`
src/OpenTelemetry.Sampler.AWS/SamplingRule.cs	`90.47% <100.00%> (+0.73%)`	⬆️
... and 1 more

src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSampler.cs

Kielek

It will be great to review code also by AWS experienced guys.

Maybe @Oberon00?

src/OpenTelemetry.Sampler.AWS/CHANGELOG.md

Kielek · 2023-04-05T07:10:14Z

src/OpenTelemetry.Sampler.AWS/Clock.cs

+namespace OpenTelemetry.Sampler.AWS;
+
+// A time keeper for the purpose of this sampler.
+internal sealed class Clock


What is a benefit for using this additional abstraction layer, instread of just using directly DateTime.UtcNow?

The sampler does quite a few things based on time, like the rules cache freshness and rate limited sampling (which would come in the next PR. The DateTime APIs can be used for such logic, but I think for testing the functionality, I need a mocked clock which can be passed in as a Clock type.

src/OpenTelemetry.Sampler.AWS/Matcher.cs

src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSampler.cs

src/OpenTelemetry.Sampler.AWS/README.md

src/OpenTelemetry.Sampler.AWS/Statistics.cs

test/OpenTelemetry.Sampler.AWS.Tests/TestMatcher.cs

Kielek · 2023-04-05T07:35:33Z

test/OpenTelemetry.Sampler.AWS.Tests/TestMatcher.cs

+    [Fact]
+    public void TestWildcardMatching()
+    {
+        Assert.True(Matcher.WildcardMatch(null, "*"));


I am not sure that null should match anything. I think it needs some documentation for this case.

a null input here would mean that the particular attribute is not set on the span, and so the * should match any value of an attribute whether it is set or not.

Oberon00 · 2023-04-05T07:52:21Z

Sorry, I don't have any experience with X-Ray, so this PR is out of my area of expertise.

src/OpenTelemetry.Sampler.AWS/FallbackSampler.cs

atshaw43 · 2023-04-07T19:30:12Z

test/OpenTelemetry.Sampler.AWS.Tests/TestMatcher.cs

+        Assert.True(Matcher.WildcardMatch("HelloWorld", "HelloWorld"));
+        Assert.True(Matcher.WildcardMatch("HelloWorld", "Hello*"));
+        Assert.True(Matcher.WildcardMatch("HelloWorld", "*World"));
+        Assert.True(Matcher.WildcardMatch("HelloWorld", "?ello*"));


Could we add two more tests with '*' and '?' in the middle of the sting on the second variable, some with other regex special characters such as '.', and a few more complex ones as well.

I want to make sure we test that ToRegexPattern sufficiently. If we have a bug here, then a customer might accidentally depend on it. It would be painful for them if we fix the bug later.

We don't need really complex testing here since the sampling rule options only allow for * and ? for matching.
https://docs.aws.amazon.com/xray/latest/devguide/xray-console-sampling.html#xray-console-sampling-options

atshaw43 · 2023-04-07T20:23:15Z

src/OpenTelemetry.Sampler.AWS/RulesCache.cs

+    }
+
+    public bool Expired()
+    {


Do we need to lock here? We are not changing state and UpdatedAt cannot be changed back to null as far as I can tell.

I think we do need a lock here. The background thread can be writing the UpdatedAt while the main thread could be reading the value when trying to sample a request. We don't want these two to happen concurrently.

src/OpenTelemetry.Sampler.AWS/SamplingRule.cs

src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSampler.cs

test/OpenTelemetry.Sampler.AWS.Tests/Utils.cs

github-actions · 2023-04-15T03:16:01Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

…lemetry-dotnet-contrib into xray_sampler_pr_2

atshaw43 · 2023-04-18T04:43:46Z

src/OpenTelemetry.Sampler.AWS/Matcher.cs

+        {
+            if (c == '*' || c == '?')
+            {
+                return Regex.IsMatch(text, ToRegexPattern(globPattern));


I do have concerns here about regex being slow. Especially since we are using a simplified version. I will leave it up to you on if we should keep it. Might be worth adding a performance test.

Yes. A performance benchmark would be a good idea. I can think of adding it once the sampler implementation is complete.

Kielek

I still have some doubts about public API. @utpilla, could you please review it?

Other parts LGTM.

utpilla · 2023-04-24T22:52:25Z

src/OpenTelemetry.Sampler.AWS/README.md

+    .AddSource(serviceName)
+    .SetResourceBuilder(resourceBuilder)
+    .AddConsoleExporter()
+    .SetSampler(AWSXRayRemoteSampler.Builder(resourceBuilder.Build()) // you must provide a resource


Just a suggestion:

This setup looks quite verbose. I think you could extract this into an extension method. Something like: AddAWSXRayRemoteSampler(this TracerProviderBuilder, Action<AWSXRaySamplerOptions> configure).

That way you would only be exposing the required options and you could reduce the publicAPI surface by marking these methods as internal: AWSXRayRemoteSampler.Builder, SetPollingInterval, SetEndpoint, Build

I had earlier of thought of doing the extension method similar to AddXRayTraceId() we have for the custom trace id generator, but I think that is counterintuitive for adding a Sampler. Since the AWSXRayRemoteSampler is just another implementation of Sampler, it should follow the standard procedure of adding a sampler via the SetSampler method.

Regarding the setting of the sampler being verbose, I agree. The customers of OTel Java and GoLang are already configuring the remote sampler in similar way, so I would like to keep the same behavior here as well.

src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSampler.cs

atshaw43 · 2023-04-25T16:52:15Z

src/OpenTelemetry.Sampler.AWS/RulesCache.cs

+        this.rwLock.EnterReadLock();
+        try
+        {
+            return this.UpdatedAt;


Is locking around returning a variable needed?

Also, I don't think GetUpdatedAt is used.

Yes. Since the UpdatedAt will be written by rule poller and read by target poller, we need this lock for safe reads.

The GetUpdatedAt will be used in my next PR by the target poller: https://github.com/srprash/opentelemetry-dotnet-contrib/blob/xray_sampler_pr_3/src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSampler.cs#L173

add rules cache and rule matching logic

c775460

srprash force-pushed the xray_sampler_pr_2 branch from 12ec9b6 to c775460 Compare March 30, 2023 18:27

srprash marked this pull request as ready for review March 30, 2023 20:00

srprash requested a review from a team March 30, 2023 20:00

Merge branch 'main' into xray_sampler_pr_2

2efb374

github-actions bot assigned srprash Mar 30, 2023

utpilla added the comp:sampler.aws Things related to OpenTelemetry.Samplers.AWS label Mar 30, 2023

Kielek reviewed Mar 31, 2023

View reviewed changes

src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSampler.cs Show resolved Hide resolved

Kielek reviewed Apr 5, 2023

View reviewed changes

atshaw43 suggested changes Apr 7, 2023

View reviewed changes

github-actions bot added the Stale label Apr 15, 2023

srprash added 3 commits April 16, 2023 16:05

rewrite using less mutable states and locking

e1d1675

Merge branch 'xray_sampler_pr_2' of https://github.com/srprash/opente…

0c990d6

…lemetry-dotnet-contrib into xray_sampler_pr_2

fix unit test

65a2e78

github-actions bot removed the Stale label Apr 17, 2023

addressing PR comments

3c18dee

atshaw43 reviewed Apr 18, 2023

View reviewed changes

atshaw43 approved these changes Apr 18, 2023

View reviewed changes

Kielek approved these changes Apr 24, 2023

View reviewed changes

Merge branch 'main' into xray_sampler_pr_2

6f37b4a

utpilla reviewed Apr 24, 2023

View reviewed changes

atshaw43 reviewed Apr 25, 2023

View reviewed changes

src/OpenTelemetry.Sampler.AWS/AWSXRayRemoteSampler.cs Show resolved Hide resolved

srprash and others added 2 commits April 25, 2023 08:45

Merge branch 'main' into xray_sampler_pr_2

76a7c1b

adding info log when using fallback sampler

9289865

atshaw43 reviewed Apr 25, 2023

View reviewed changes

atshaw43 approved these changes Apr 26, 2023

View reviewed changes

Kielek merged commit 1a58431 into open-telemetry:main Apr 26, 2023

srprash deleted the xray_sampler_pr_2 branch April 26, 2023 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Sampler.AWS] Part-2: Add rules cache and rule matching logic #1124

[Sampler.AWS] Part-2: Add rules cache and rule matching logic #1124

srprash commented Mar 30, 2023 •

edited

Loading

codecov bot commented Mar 30, 2023 •

edited

Loading

Kielek left a comment

Kielek Apr 5, 2023

srprash Apr 17, 2023

Kielek Apr 5, 2023

srprash Apr 17, 2023

Oberon00 commented Apr 5, 2023

atshaw43 Apr 7, 2023

srprash Apr 17, 2023

atshaw43 Apr 7, 2023

srprash Apr 17, 2023 •

edited

Loading

github-actions bot commented Apr 15, 2023

atshaw43 Apr 18, 2023 •

edited

Loading

srprash Apr 18, 2023

Kielek left a comment

utpilla Apr 24, 2023

srprash Apr 25, 2023

atshaw43 Apr 25, 2023

srprash Apr 25, 2023

[Sampler.AWS] Part-2: Add rules cache and rule matching logic #1124

[Sampler.AWS] Part-2: Add rules cache and rule matching logic #1124

Conversation

srprash commented Mar 30, 2023 • edited Loading

Changes

codecov bot commented Mar 30, 2023 • edited Loading

Codecov Report

Kielek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 commented Apr 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srprash Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Apr 15, 2023

atshaw43 Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kielek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srprash commented Mar 30, 2023 •

edited

Loading

codecov bot commented Mar 30, 2023 •

edited

Loading

srprash Apr 17, 2023 •

edited

Loading

atshaw43 Apr 18, 2023 •

edited

Loading