diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c8fe4d5a52..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 diff --git a/samplers/aws/xray/internal/manifest.go b/samplers/aws/xray/internal/manifest.go index caef1c43c61..21b5ce3d374 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,22 @@ 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 + } + + // 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) { + tempManifest.Rules[i] = curRule + } + } + } + m.Rules = tempManifest.Rules m.refreshedAt = m.clock.now() m.mu.Unlock() 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