Repository navigation
Conversation
- Match logSamplingDecision if/else style in logNoPermitAvailable
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c875d1b2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
What Does This Do
ApiSecuritySamplerImpl.preSampleRequestdeclines a request because no post-processing permit is available (counter.tryAcquire()fails). The line includes the permit pool size (MAX_POST_PROCESSING_TASKS) and the endpoint hash.preSampleRequestcontract is untouched.Motivation
The system-test
tests/appsec/api_security/test_schemas.py::Test_Schema_Request_Json_Body::test_request_methodis flaky on Java + vertx4 (APPSEC_API_SECURITY): the failing span has no_dd.appsec.s.*tag at all. Timing analysis of a failing run points to the permit pool being exhausted: permits are taken on the request thread and only released byAppSecSpanPostProcessoron theTraceProcessingWorkerthread, which also sends trace payloads synchronously. When more than 4 sampled requests end during a flush, the next one is silently not sampled.Today that decision leaves no trace in the logs, so the hypothesis cannot be confirmed from a debug run. With this log, a run with
DD_TRACE_DEBUG=trueshows exactly which span lost its permit: the new line is emitted on the same thread right afterGatewayBridge.maybeSampleForApiSecuritylogsChecking API Security for end of request handler on span: {spanId}.Additional Notes
DD_API_SECURITY_MAX_CONCURRENT_REQUESTS(theAPPSEC_API_SECURITYscenario sets it to 50), but no tracer implements that key today: dotnet only supported it in v2.46 to v2.49 and removed it in v2.50, and it is not in any tracer's supported configurations. API Security sampling is best effort, so the flake will be handled on the system-tests side.AgentTracer.activeSpan()for trace/span ids: request end can run outside the server span scope (Lambda, async servers), so the active span may be null or unrelated. The precedingGatewayBridgeline carries the correct span id from the request-ended callback.GatewayBridge.maybeSampleForApiSecurity, and limiting it would hide the bursts this is meant to surface.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: APPSEC-71153
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.