[improvement] Don't impose DO_NOT_SAMPLE on downstream services #170
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this PR
If a service decides to start sampling a trace, it's important that all downstream service do the same to ensure we get a nice complete set of data in trace-log-viewer.
HOWEVER, if the TraceSampler decides not to sample a trace, it currently imposes this DO_NOT_SAMPLE decision on all downstream services too by setting the X-B3-Sampled header to
0
This means that theoretically if a single service early in a call graph has a low sampling rate, then it may unnecessarily reduce tracing information that all downstream services, even if they had plenty of capacity to record traces.
After this PR
If a service decides not to observe a span, it no longer forces all the downstream services to also drop the span, and instead allows downstream samplers to decide.
Longer-term, I'd also quite like to make our trace sampling to be a target logging rate (e.g. up to 1000 lines/second) rather than just a constant percentage of incoming requests. This should mean that infrequently accessed services could just sample every network request, thereby hopefully making life easier for developers.
Possible downsides
Related to #32