Fix xray.BeginSegment to honor sampling rule #244
Conversation
|
|
||
| testCases := []testCase{ | ||
| { | ||
| testName: "if the request is empty and the rule has a specific ServiceName, AppliesTo should return false", |
There was a problem hiding this comment.
Can we add one test case here, where all the values of rule are populated like URLPath, ServiceName, ServiceType, Host and httpMethod. Now, we pass an empty request, AppliesTo should return false ? Same for TestLocalizedRuleAppliesTo too.
| @@ -83,8 +83,8 @@ func BeginSegmentWithSampling(ctx context.Context, name string, r *http.Request, | |||
| } | |||
|
|
|||
| if r == nil || traceHeader == nil { | |||
| // Sampling strategy fallbacks to default sampling in the case of directly using BeginSegment API without any instrumentation | |||
| sd := seg.ParentSegment.GetConfiguration().SamplingStrategy.ShouldTrace(&sampling.Request{}) | |||
| // No header or request information provided so we can only evaluate sampling based on the serviceName | |||
There was a problem hiding this comment.
Also, can we include this as a Note in README file so that sampling behavior is very clear when customer use BeginSegment API directly. (since we are evaluating only based on service name here)
strategy/sampling/sampling_rule.go
Outdated
| (method == "" || pattern.WildcardMatchCaseInsensitive(p.HTTPMethod, method)) | ||
| return pattern.WildcardMatchCaseInsensitive(p.Host, host) && | ||
| pattern.WildcardMatchCaseInsensitive(p.URLPath, path) && | ||
| pattern.WildcardMatchCaseInsensitive(p.HTTPMethod, method) | ||
| } | ||
|
|
||
| // AppliesTo returns true if the sampling rule matches against given sampling request. False Otherwise. | ||
| // Assumes lock is already held, if required. | ||
| func (r *CentralizedRule) AppliesTo(request *Request) bool { |
There was a problem hiding this comment.
Changing this to exclude absent request attributes would lead to behavior inconsistent with other SDKs, for example our node SDK and Python SDK. I understand the ask to change this, but changing the behavior like this would be a fairly consequential, arguably breaking, change.
@kevioke I believe the change you made to the beginSegment API should be sufficient to resolve #242 since the serviceName will no longer be absent. Our team will have to discuss this change which may delay this PR quite a bit. If you'd like to remove this change and only include the beginSegment change that could expedite this getting released.
There was a problem hiding this comment.
@willarmiros thanks for the reply! I had a hunch this empty request logic was present for a reason, just didn't know quite why. In any case, I like your proposal of just including the changes to beginSegment to speed this PR along, and I believe you're correct that it should work for my use cases since now I can specify a rule with a ServiceName.
I'll update the PR later today or tomorrow.
Per aws#244 (comment) these changes require further consideration from the AWS team.
README.md
Outdated
| @@ -137,6 +137,7 @@ func init() { | |||
| ``` | |||
|
|
|||
| **Start a custom segment/subsegment** | |||
| Note that the `xray.BeginSegment` function currently uses only the service name parameter to determine the applicability of any sampling rules. | |||
There was a problem hiding this comment.
Hi can we rephrase this to something like customers using xray.BeginSegment API directly will only be able to evaluate sampling rules based on service name ? It's important to show that this behavior is not applicable for instrumented requests.
There was a problem hiding this comment.
That is much clearer :)
I'll update it accordingly
Fixes #242
Description of changes:
Requests that have empty fields should not apply to sampling rules that require specific patterns to match. This change removes the
ORcondition on having empty fields short circuit any sampling rule pattern match.In addition, this change also passes the
ServiceNameto the ShouldTrace in the BeginSegmentWithSampling function so that sampling rules can be applied more accurately with traces that only specify ServiceNames.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.