From 239b868103b004f0c45e2cd6ba7401cd4af2481a Mon Sep 17 00:00:00 2001 From: Jonathan Lee Date: Wed, 22 Mar 2023 21:21:00 -0700 Subject: [PATCH 1/4] preserve previous rule reservoir if rule property has not changed --- CHANGELOG.md | 1 + samplers/aws/xray/internal/manifest.go | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c7e520aef5..4c6e4c014e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - AWS SDK span name to be of the format `Service.Operation` in `go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws`. (#3582, #3521) - Prevent sampler configuration reset from erroneously sampling first span in `go.opentelemetry.io/contrib/samplers/jaegerremote`. (#3603, #3604) +- AWS XRay Remote Sampling to preserve previous rule if updated rule property has not changed in `go.opentelemetry.io/contrib/samplers/aws/xray`. (#3619, # TBD) ## [1.16.0-rc.1/0.41.0-rc.1/0.9.0-rc.1] - 2023-03-02 diff --git a/samplers/aws/xray/internal/manifest.go b/samplers/aws/xray/internal/manifest.go index caef1c43c61..b192b1006a2 100644 --- a/samplers/aws/xray/internal/manifest.go +++ b/samplers/aws/xray/internal/manifest.go @@ -20,6 +20,7 @@ import ( "fmt" "math" "net/url" + "reflect" "sort" "strings" "sync" @@ -183,7 +184,21 @@ func (m *Manifest) updateRules(rules *getSamplingRulesOutput) { // Re-sort to fix matching priorities. tempManifest.sort() + var currentRuleMap = make(map[string]Rule) + m.mu.Lock() + for _, rule := range m.Rules { + currentRuleMap[rule.ruleProperties.RuleName] = rule + } + + for i, newRule := range tempManifest.Rules { + if curRule, ok := currentRuleMap[newRule.ruleProperties.RuleName]; ok { + if reflect.DeepEqual(newRule.ruleProperties, curRule.ruleProperties) { + tempManifest.Rules[i] = curRule + } + } + } + m.Rules = tempManifest.Rules m.refreshedAt = m.clock.now() m.mu.Unlock() From 6d8f3969f0268a4d1e6b4129e1b023f6479be69f Mon Sep 17 00:00:00 2001 From: Jonathan Lee Date: Wed, 22 Mar 2023 22:49:28 -0700 Subject: [PATCH 2/4] update CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c6e4c014e0..864b95677fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - AWS SDK span name to be of the format `Service.Operation` in `go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws`. (#3582, #3521) - Prevent sampler configuration reset from erroneously sampling first span in `go.opentelemetry.io/contrib/samplers/jaegerremote`. (#3603, #3604) -- AWS XRay Remote Sampling to preserve previous rule if updated rule property has not changed in `go.opentelemetry.io/contrib/samplers/aws/xray`. (#3619, # TBD) +- AWS XRay Remote Sampling to preserve previous rule if updated rule property has not changed in `go.opentelemetry.io/contrib/samplers/aws/xray`. (#3619, #3620) ## [1.16.0-rc.1/0.41.0-rc.1/0.9.0-rc.1] - 2023-03-02 From 6274efd3fdad9871db2ef1db85653008657f3083 Mon Sep 17 00:00:00 2001 From: Jonathan Lee Date: Thu, 23 Mar 2023 09:53:28 -0700 Subject: [PATCH 3/4] add unit tests --- samplers/aws/xray/internal/manifest.go | 1 + samplers/aws/xray/internal/manifest_test.go | 137 ++++++++++++++++++++ 2 files changed, 138 insertions(+) diff --git a/samplers/aws/xray/internal/manifest.go b/samplers/aws/xray/internal/manifest.go index b192b1006a2..21b5ce3d374 100644 --- a/samplers/aws/xray/internal/manifest.go +++ b/samplers/aws/xray/internal/manifest.go @@ -191,6 +191,7 @@ func (m *Manifest) updateRules(rules *getSamplingRulesOutput) { currentRuleMap[rule.ruleProperties.RuleName] = rule } + // Preserve entire Rule if newRule.ruleProperties == curRule.ruleProperties for i, newRule := range tempManifest.Rules { if curRule, ok := currentRuleMap[newRule.ruleProperties.RuleName]; ok { if reflect.DeepEqual(newRule.ruleProperties, curRule.ruleProperties) { diff --git a/samplers/aws/xray/internal/manifest_test.go b/samplers/aws/xray/internal/manifest_test.go index 8d65087ea2c..5a933f15cb6 100644 --- a/samplers/aws/xray/internal/manifest_test.go +++ b/samplers/aws/xray/internal/manifest_test.go @@ -1644,6 +1644,143 @@ func TestRaceUpdatingRulesAndTargetsWhileMatching(t *testing.T) { } } +// Validate Rules are preserved when a rule is updated with the same ruleProperties. +func TestPreserveRulesWithSameRuleProperties(t *testing.T) { + // getSamplingRules response to update existing manifest rule, with matching ruleProperties + ruleRecords := samplingRuleRecords{ + SamplingRule: &ruleProperties{ + RuleName: "r1", + Priority: 10000, + Host: "localhost", + HTTPMethod: "*", + URLPath: "/test/path", + ReservoirSize: 40, + FixedRate: 0.9, + Version: 1, + ServiceName: "helios", + ResourceARN: "*", + ServiceType: "*", + }, + } + + // existing rule already present in manifest + r1 := Rule{ + ruleProperties: ruleProperties{ + RuleName: "r1", + Priority: 10000, + Host: "localhost", + HTTPMethod: "*", + URLPath: "/test/path", + ReservoirSize: 40, + FixedRate: 0.9, + Version: 1, + ServiceName: "helios", + ResourceARN: "*", + ServiceType: "*", + }, + reservoir: &reservoir{ + capacity: 100, + quota: 100, + quotaBalance: 80, + refreshedAt: time.Unix(13000000, 0), + }, + samplingStatistics: &samplingStatistics{ + matchedRequests: 500, + sampledRequests: 10, + borrowedRequests: 0, + }, + } + clock := &mockClock{ + nowTime: 1500000000, + } + + rules := []Rule{r1} + + m := &Manifest{ + Rules: rules, + clock: clock, + } + + // Update rules + m.updateRules(&getSamplingRulesOutput{ + SamplingRuleRecords: []*samplingRuleRecords{&ruleRecords}, + }) + + require.Equal(t, r1.reservoir, m.Rules[0].reservoir) + require.Equal(t, r1.samplingStatistics, m.Rules[0].samplingStatistics) +} + +// Validate Rules are NOT preserved when a rule is updated with a different ruleProperties with the same RuleName. +func TestDoNotPreserveRulesWithDifferentRuleProperties(t *testing.T) { + // getSamplingRules response to update existing manifest rule, with different ruleProperties + ruleRecords := samplingRuleRecords{ + SamplingRule: &ruleProperties{ + RuleName: "r1", + Priority: 10000, + Host: "localhost", + HTTPMethod: "*", + URLPath: "/test/path", + ReservoirSize: 40, + FixedRate: 0.9, + Version: 1, + ServiceName: "helios", + ResourceARN: "*", + ServiceType: "*", + }, + } + + // existing rule already present in manifest + r1 := Rule{ + ruleProperties: ruleProperties{ + RuleName: "r1", + Priority: 10001, + Host: "localhost", + HTTPMethod: "*", + URLPath: "/test/path", + ReservoirSize: 40, + FixedRate: 0.9, + Version: 1, + ServiceName: "helios", + ResourceARN: "*", + ServiceType: "*", + }, + reservoir: &reservoir{ + capacity: 100, + quota: 100, + quotaBalance: 80, + refreshedAt: time.Unix(13000000, 0), + }, + samplingStatistics: &samplingStatistics{ + matchedRequests: 500, + sampledRequests: 10, + borrowedRequests: 0, + }, + } + clock := &mockClock{ + nowTime: 1500000000, + } + + rules := []Rule{r1} + + m := &Manifest{ + Rules: rules, + clock: clock, + } + + // Update rules + m.updateRules(&getSamplingRulesOutput{ + SamplingRuleRecords: []*samplingRuleRecords{&ruleRecords}, + }) + + require.Equal(t, m.Rules[0].reservoir.quota, 0.0) + require.Equal(t, m.Rules[0].reservoir.quotaBalance, 0.0) + require.Equal(t, *m.Rules[0].samplingStatistics, samplingStatistics{ + matchedRequests: 0, + sampledRequests: 0, + borrowedRequests: 0, + }) +} + // validate no data race is when capturing sampling statistics in manifest while sampling. func TestRaceUpdatingSamplingStatisticsWhenSampling(t *testing.T) { // existing rule already present in manifest From a582122eefdaf9a47ea84ddf8ae91a1d6c7b9fa3 Mon Sep 17 00:00:00 2001 From: Jonathan Lee Date: Tue, 2 May 2023 15:27:54 -0700 Subject: [PATCH 4/4] update CHANGELOG --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 97ffb9205af..7a8d8bcb418 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Add the new `go.opentelemetry.io/contrib/instrgen` package to provide auto-generated source code instrumentation. (#3068, #3108) - The `WithPublicEndpoint` and `WithPublicEndpointFn` options in `go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/otelmux`. +### Fixed + +- AWS XRay Remote Sampling to preserve previous rule if updated rule property has not changed in `go.opentelemetry.io/contrib/samplers/aws/xray`. (#3619, #3620) + ## [1.16.0/0.41.0/0.10.0] - 2023-04-28 ### Added @@ -41,7 +45,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - AWS SDK rename attributes `aws.operation`, `aws.service` to `rpc.method`,`rpc.service` in `go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws`. (#3582, #3617) - AWS SDK span name to be of the format `Service.Operation` in `go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws`. (#3582, #3521) - Prevent sampler configuration reset from erroneously sampling first span in `go.opentelemetry.io/contrib/samplers/jaegerremote`. (#3603, #3604) -- AWS XRay Remote Sampling to preserve previous rule if updated rule property has not changed in `go.opentelemetry.io/contrib/samplers/aws/xray`. (#3619, #3620) ## [1.16.0-rc.1/0.41.0-rc.1/0.10.0-rc.1] - 2023-03-02