feat(opentelemetry-sampler-aws-xray): Add Rules Caching and Rules Matching Logic #2824
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2824 +/- ##
==========================================
+ Coverage 89.76% 89.80% +0.03%
==========================================
Files 187 187
Lines 9149 9149
Branches 1885 1885
==========================================
+ Hits 8213 8216 +3
+ Misses 936 933 -3 🚀 New features to boost your workflow:
|
|
cc @lukeina2z for review who is familiar with Sampling. Currently everything is implemented except for Updating Sampling Targets and the Rate Limiting (Reservoir) Sampler. |
incubator/opentelemetry-sampler-aws-xray/src/fallback-sampler.ts
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public updateRules(newRuleAppliers: SamplingRuleApplier[]): void { | ||
| const oldRuleAppliersMap: { [key: string]: SamplingRuleApplier } = {}; |
There was a problem hiding this comment.
nit: You could use a Map<string, SamplingRuleApplier> instead of a plain object here for better perf.
There was a problem hiding this comment.
Thanks! I Incorporated your suggestion.
| attributes: Attributes | ||
| ): SamplingRuleApplier | undefined { | ||
| return this.ruleAppliers.find( | ||
| rule => |
There was a problem hiding this comment.
Could this, in some cases, find the default rule before a higher priority rule? Will this matter?
There was a problem hiding this comment.
This use of find() here relies on the this.ruleAppliers list being always sorted by priority (ascending integers), where the default rule is always hardcoded to be the last priority by AWS X-Ray. This sorting is assumed because it is sorted here whenever the Sampling rules are updated. This assumption is important, I've added a comment.
| // If scheme is not present, assume it's bad instrumentation and ignore. | ||
| if (schemeEndIndex > -1) { | ||
| // urlparse("scheme://netloc/path;parameters?query#fragment") | ||
| httpTarget = new URL(httpUrl).pathname; |
There was a problem hiding this comment.
Ensure that this will never receive a malformed httpUrl as it will throw if it does.
There was a problem hiding this comment.
Nice catch, I chose to rely on a try...catch to handle this.
| import { expect } from 'expect'; | ||
| import { FallbackSampler } from '../src/fallback-sampler'; | ||
|
|
||
| describe('FallBackSampler', () => { |
There was a problem hiding this comment.
Should we add additional tests here? For example:
- Sampling rate verification - test that 5% of traces are sampled over many iterations
- Deterministic behavior - test that the same trace id always gets the same sampling decision
- Parameter forwarding - test that all parameters are correctly passed to the underlying sampler
There was a problem hiding this comment.
Currently, the fallback sampler is just using the TraceIdRatioBasedSampler, which is covered under: https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-sdk-trace-base/test/common/sampler/TraceIdRatioBasedSampler.test.ts.
The next PR will add the Rate Limiter logic to the fallback sampler, where the Sampling Reservoir functionality will be tested.
|
It would be helpful to add some documentation explaining the fallback behavior when remote rules are unavailable. |
|
The Fallback Behaviour is currently documented in
|
pichlermarc
left a comment
pichlermarc
left a comment
There was a problem hiding this comment.
proxy-approving for @yiyuan-he (component-owner)
5 participants
Which problem is this PR solving?
Short description of the changes
This PR is a followup to #2750
RuleCacheSamplingRuleAppliers, ordered by rule priority then rule name. Each Rule Applier corresponds to the Sampling Rule from GetSamplingRules. Each call to GetSamplingRules will only update theRules that have changed properties, to preserve the state of unchanged rules. This means when Reservoir and Statistics are implemented (later) in theRules, they will persist for unchanged rules.{ResourceAttributes,SpanAttributes}) that has highest priority.SamplingRuleApplierto perform Fixed Rate Sampling, and to include a method to apply matching logic against a set of {ResourceAttributes,SpanAttributes} by using the wild card and attribute matching from UtilsFallbackSamplerX-Ray Remote Samplerto depend on the SamplingRuleApplier fromRuleCacheand the FallBack Sampler to performshouldSample