jwt_authn: set sampled=nullopt on JWKS fetch span to honor parent sampling decision#45452
jwt_authn: set sampled=nullopt on JWKS fetch span to honor parent sampling decision#45452zheng460 wants to merge 2 commits into
Conversation
…pling decision The JWKS fetcher built AsyncClient::RequestOptions without setting the sampled flag, so the "JWT Remote PubKey Fetch" span was always sampled regardless of the parent span's sampling decision. Set sampled to absl::nullopt so the fetch span inherits the parent's decision. Fixes envoyproxy#44431 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Shirong Zheng <szheng@indeed.com>
|
Hi @zheng460, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
Assert that RequestOptions::sampled_ is left unset (absl::nullopt) in TestSpanPassedDown, guarding the JWKS fetch span against regressing back to always-sampled behavior. Fixes envoyproxy#44431 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Shirong Zheng <szheng@indeed.com>
|
PTAL @TAOXUY |
|
/retest |
botengyao
left a comment
There was a problem hiding this comment.
thanks for the fix, lgtm module a test.
/wait
| @@ -64,6 +64,9 @@ class JwksFetcherImpl : public JwksFetcher, | |||
| .setTimeout(std::chrono::milliseconds( | |||
| DurationUtil::durationToMilliseconds(remote_jwks_.http_uri().timeout()))) | |||
| .setParentSpan(parent_span) | |||
| // Leave sampled unset so the JWKS fetch span honors the parent span's | |||
| // sampling decision instead of always being sampled. | |||
| .setSampled(absl::nullopt) | |||
There was a problem hiding this comment.
could you please add a test for the Inheritance?
There was a problem hiding this comment.
Updated TestSpanPassedDown now asserts the fetcher leaves the sampling decision unset:
EXPECT_FALSE(options.sampled_.has_value());
That assertion is the full extent of what's testable at this layer: the fetcher's only role in inheritance is to pass sampled_ = absl::nullopt to the async client — it never spawns a span or sets sampling itself, and the async client is mocked in this test. The actual "unset ⇒ parent's decision is honored" semantics are owned and already covered one layer down by AsyncClientImplTracingTest.BasicNamedChildSpanKeepParentSampling (asserts the async client doesn't override the child span's sampling when sampled_ is empty), and the tracer-level propagation by ZipkinTracerTest.SpanSampledPropagatedToChild. Happy to add an OTel equivalent of that propagation test if useful, since OTel didn't have one.
Commit Message:
jwt_authn: set sampled=nullopt on JWKS fetch span to honor parent sampling decision
The JWKS fetcher built
AsyncClient::RequestOptionswithout setting the sampledflag, so the "JWT Remote PubKey Fetch" span was always sampled regardless of the
parent span's sampling decision (
RequestOptions::sampled_defaults totrue).Set sampled to
absl::nulloptso the fetch span inherits the parent's decision.Fixes #44431
Additional Description:
The same fix was previously applied to
ext_authzspans in #19343 and to luahttpCallspans in #33200. This applies the equivalent fix to the JWKS fetcher'sasync HTTP client.
Generative AI disclosure: this change was prepared with the assistance of
generative AI (Claude). The author understands the code being submitted.
Risk Level: Low
Testing: Added a unit test case in
TestSpanPassedDownasserting the JWKS fetchspan leaves the sampling decision unset (
RequestOptions::sampled_isabsl::nullopt); the assertion fails without this change (the field defaults totrue) and passes with it. Also built Envoy and ran it against a Datadog Agentingestion rule, confirming the "JWT Remote PubKey Fetch" span now honors the
parent span's sampling decision instead of always being sampled.
ingestion_reasonis set to
rulecorrectly instead ofmanual.Example span (Some information is hidden for security reasons. I can add more details if needed):
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A