Add AWS X-Ray Adaptive Sampling Support #2147
Conversation
| SamplingResult result = | ||
| applier.shouldSample(parentContext, traceId, name, spanKind, attributes, parentLinks); | ||
|
|
||
| String ruleToPropagate = |
There was a problem hiding this comment.
nit: It is readable to update line 134-150:
- if there is rule from upstream, return upstream's rule;
- if adaptiveSamplingConfig, return current rule;
There was a problem hiding this comment.
Added a comment. Splitting the two cases would require some code duplication or turning the logic into a function which doesn't really make sense to me here.
| for (AwsXrayAdaptiveSamplingConfig.AnomalyConditions condition : anomalyConditions) { | ||
| // Check if the operation matches any in the list or if operations list is null (match all) | ||
| List<String> operations = condition.getOperations(); | ||
| if (!(operations == null || operations.isEmpty() || operations.contains(operation))) { |
There was a problem hiding this comment.
Is operation(s) in condition required?
| } | ||
| } | ||
| } | ||
| if (shouldBoostSampling && shouldCaptureAnomalySpan) { |
There was a problem hiding this comment.
For performance, maybe if shouldBoostSampling or shouldCaptureAnomalySpan is true we can skip the remaining BoostSampling or CaptureAnomaly check.
| boolean isLocalRootSpan = | ||
| parentContext == null || !parentContext.isValid() || parentContext.isRemote(); | ||
|
|
||
| if (shouldBoostSampling || shouldCaptureAnomalySpan || isLocalRootSpan) { |
There was a problem hiding this comment.
Readability, not sure whether the "if" here is useful since you have
// Anomaly Capture, Sampling Boost below
There was a problem hiding this comment.
Split // Anomaly Capture from // Sampling Boost section. Still need shouldBoostSampling || isLocalRootSpan together since they both trigger the same loop logic that finds the ruleToReportTo
Description:
Adding support for X-Ray's new Adaptive Sampling feature. The feature will allow users to detect anomalies in their application across distributed services and boost the sampling rate of their root service based on their X-Ray sampling rule configuration and optionally a user-provided SDK level configuration. It will also allow users to optionally provide an error capture configuration, where - if configured - the sampler will send unsampled anomaly spans to be exported directly:
statusCode > 499(i.e. 5XX) will be considered anomaliesThe changes:
setSpanExporterandsetAdaptiveSamplingConfigto set up the feature - if these are not provided any attempt to use theadaptSamplingAPI will throw anIllegalStateExceptionadaptSamplingthat accepts a span and its associated spanData:Testing:
Re-testing needs to be done based on latest changes in aws-otel-java-instrumentation once ready, hence the need for the draft revision of the PR
Documentation:
TBD - hence draft PR
Outstanding items:
These changes are the first phase and iteration of AWS X-Ray's adaptive sampling feature. As we get feedback, more changes may be introduced to improve or streamline the experience.