feat: support client and overall tracing sampling#9402
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9402 +/- ##
==========================================
+ Coverage 75.44% 75.45% +0.01%
==========================================
Files 252 252
Lines 41642 41659 +17
==========================================
+ Hits 31417 31434 +17
- Misses 8086 8089 +3
+ Partials 2139 2136 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
c4b0557 to
5629244
Compare
zirain
left a comment
zirain
left a comment
There was a problem hiding this comment.
Looks good, would be better if we could have e2e.
32e7073 to
dcc1994
Compare
|
I added E2E tests, but I still can't beat several CI tests. Maybe you can help me with this? |
|
it looks more like a flake test. /retest |
|
|
||
| observed := false | ||
| deadline := time.Now().Add(15 * time.Second) | ||
| for time.Now().Before(deadline) { |
There was a problem hiding this comment.
why not use same wait-until pattern?
|
all tests success |
| // selected for tracing when requested by the client. | ||
| // | ||
| // +optional | ||
| ClientSamplingFraction *gwapiv1.Fraction `json:"clientSamplingFraction,omitempty"` |
There was a problem hiding this comment.
- can we change the defaults of disabling clientSamplingFraction to 0, so users need to opt in to this feature ? we'll need to note this in the breaking changes section
- can we avoid overallSamplingFraction ?
wdyt @envoyproxy/gateway-maintainers
There was a problem hiding this comment.
Asked below about the default value 0
| return &hcm.HttpConnectionManager_Tracing{ | ||
| ClientSampling: &xdstype.Percent{ | ||
| Value: 100.0, | ||
| Value: ptr.Deref(tracing.ClientSamplingRate, 100), |
There was a problem hiding this comment.
As pointed out by @arkodg , please document this default to 100 if not settings, same behavior as before.
There was a problem hiding this comment.
Do we change the default client sampling value to 0 as suggested above?
There was a problem hiding this comment.
+1 to make a breaking change to default to 0
f6d80d7 to
32a43f5
Compare
Signed-off-by: Zakhar Zakharov <zakhar.zakharov.zz16@gmail.com>
Signed-off-by: Zakhar Zakharov <zakhar.zakharov.zz16@gmail.com>
Signed-off-by: z.s.zakharov <z.s.zakharov@tinkoff.ru>
Signed-off-by: Zakhar Zakharov <zakhar.zakharov.zz16@gmail.com>
Signed-off-by: z.s.zakharov <z.s.zakharov@tinkoff.ru>
268a3d2 to
c9adef9
Compare
| @@ -0,0 +1 @@ | |||
| Added support for configuring Envoy tracing client and overall sampling fractions on EnvoyProxy and BackendTrafficPolicy tracing settings. | |||
There was a problem hiding this comment.
we should also documented the breaking change of clientSamplingRate.
There was a problem hiding this comment.
create file in breaking_changes
Signed-off-by: z.s.zakharov <z.s.zakharov@tinkoff.ru>
|
/retest |
|
/retest |
| // selected for tracing after all other sampling checks have been applied. | ||
| // | ||
| // +optional | ||
| OverallSamplingFraction *gwapiv1.Fraction `json:"overallSamplingFraction,omitempty"` |
There was a problem hiding this comment.
can we avoid OverallSamplingFraction since ClientSamplingFraction now defaults to 0 ?
There was a problem hiding this comment.
I’d keep OverallSamplingFraction. ClientSamplingFraction=0 only disables client-triggered tracing via x-client-trace-id, while OverallSamplingFraction is still useful as the final cap on total trace volume across all sampling paths.
There was a problem hiding this comment.
ok lets keep it, if we think this is a legit use case,
samplingRate: 1
clientSamplingRate: 50
overallSamplingRate: 10
my preference would be to set explicit individual rates, vs a global cap, wdyt @envoyproxy/gateway-maintainers
| op, upstreamOp := buildTracingOperation(tracing.SpanName) | ||
|
|
||
| return &routev3.Tracing{ | ||
| ClientSampling: fractionalpercent.FromFraction(tracing.ClientSamplingFraction), |
There was a problem hiding this comment.
this needs to be set to 0 if val is nil
Signed-off-by: Zakhar Zakharov <zakhar.zakharov.zz16@gmail.com>
|
/retest |
|
/retest |
3 participants
What this PR does / why we need it:
This PR adds support for configuring Envoy tracing
client_samplingandoverall_samplingin addition to the existingrandom_samplingsupport.Envoy Gateway already exposes
samplingRate/samplingFraction, which maps to Envoy HCMrandom_sampling. This change adds new optional client and overall sampling settings so userscan control traces requested by clients and the final overall trace volume sent to collectors.
The PR updates:
EnvoyProxytracing API withclientSamplingRate,overallSamplingRate,clientSamplingFraction, andoverallSamplingFractionBackendTrafficPolicyroute tracing withclientSamplingFractionandoverallSamplingFractionWhich issue(s) this PR fixes:
Fixes #9372
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. The API change can be in a separate PR, or in the same PR, but the API must be agreed before implementation. N/A if this PR does not contain API changes.make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.)release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes.make gen-checkand committed the result if API/helm charts/modules changed.release-notes/current/breaking_changes/.