XRay Remote Sampler - Preserve previous rule reservoir if rule proper…

…ty has not changed (#3620) * preserve previous rule reservoir if rule property has not changed * update CHANGELOG * add unit tests * update CHANGELOG
open-telemetry · May 2, 2023 · 46bde6d · 46bde6d
1 parent 8538a77
commit 46bde6d
Show file tree

Hide file tree

Showing 3 changed files with 157 additions and 0 deletions.
diff --git 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
@@ -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
@@ -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