Skip to content

Conversation

@cataphract
Copy link
Contributor

Description

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@cataphract cataphract requested review from a team as code owners January 5, 2026 17:29
@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2026

Codecov Report

❌ Patch coverage is 73.97260% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.68%. Comparing base (98a31cb) to head (3df4e17).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
appsec/src/extension/request_lifecycle.c 74.19% 10 Missing and 6 partials ⚠️
appsec/src/extension/ddtrace.c 62.50% 2 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (73.97%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3557      +/-   ##
==========================================
- Coverage   61.78%   61.68%   -0.10%     
==========================================
  Files         139      139              
  Lines       13051    13114      +63     
  Branches     1712     1728      +16     
==========================================
+ Hits         8063     8090      +27     
- Misses       4225     4253      +28     
- Partials      763      771       +8     
Files with missing lines Coverage Δ
appsec/src/extension/commands_helpers.c 69.69% <100.00%> (-1.12%) ⬇️
appsec/src/extension/tags.c 80.31% <100.00%> (-0.04%) ⬇️
appsec/src/extension/tags.h 100.00% <ø> (ø)
appsec/src/helper/network/proto.hpp 93.22% <ø> (ø)
appsec/src/extension/ddtrace.c 66.41% <62.50%> (-0.09%) ⬇️
appsec/src/extension/request_lifecycle.c 65.86% <74.19%> (+0.57%) ⬆️

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98a31cb...3df4e17. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cataphract cataphract force-pushed the glopes/rfc-1076 branch 5 times, most recently from 0d833f9 to 90c760b Compare January 7, 2026 12:34
@cataphract
Copy link
Contributor Author

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ab7aab7b46

ℹ️ 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".

Comment on lines +933 to +936
if (_request_blocked) {
mlog_g(
dd_log_debug, "Request was blocked; not sampling for API security");
return 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Skip sampling only for pre-shutdown blocks

The new _request_blocked guard here only suppresses sampling if a block/redirect was detected before _calc_sampling_key runs. However _calc_sampling_key is executed before dd_request_shutdown, and _request_blocked is only set when helper actions are processed (via dd_req_lifecycle_set_blocked in commands_helpers.c). If the WAF blocks based on response data during request_shutdown, _request_blocked is still false at this point, so a sampling key is still sent and schema sampling can occur for blocked responses. If the intent is “never sample blocked requests,” this misses response-time blocks; consider deferring sampling until after the shutdown verdict or clearing the key when a block/redirect is returned.

Useful? React with 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acceptable, the main point of not sampling blocked requests is to avoid calculation of the schema for the blocked response; that won't happen when blocking happens on rshutdown, as there won't be a subsequent libddwaf call capable of submitting the block response.

@cataphract cataphract merged commit 4a3a987 into master Jan 7, 2026
1996 of 2007 checks passed
@cataphract cataphract deleted the glopes/rfc-1076 branch January 7, 2026 14:15
@github-actions github-actions bot added this to the 1.16.0 milestone Jan 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants