Repository navigation
Conversation
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae85e485f1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Metric-value evaluation failures preserve their error state through signal completion, allowing the collector to report them instead of silently dropping the signal. Runtime behavior remains unverified.
🤖 Bits Code Review · Commit ae85e48
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f185074a0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Thanks for tracking this down, the diagnosis is spot on: the exception escapes sample() and the catch-all in _close_signals / _dd_debugger_hook drops the signal before it reaches the collector. The overall shape of the fix looks right to me. A few things before this goes in:
PR description overclaims the effect on probe status. Pushing a COND_ERROR signal does not flip the probe from installed/active to ERROR. In collector.py a COND_ERROR signal only bumps the guardrails.evaluation.errors metric and enqueues a log carrying evaluationErrors; probe status only goes to ERROR via ProbeRegistry.set_error, which is only called for install-time failures. So after this change the user gets an evaluation-error log (same as for a failing condition), and the probe simply never becomes EMITTING. That's a good outcome, but could you update the description to match?
Guardrail metric tagging (not in the diff). Since value errors reuse COND_ERROR, the collector will tag them error_kind: "condition". Either a distinct state/tag, or at least a comment noting the reuse is deliberate, would keep the guardrail dashboards honest. Same for sharing condition_error_limiter with genuine condition errors: probably fine, but worth stating as a choice.
Nice touches: naming only the type for non-numeric values so nothing sensitive lands in the message, and covering OverflowError.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d8f62e301
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7d582e6f2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
🎉 All green!🧪 All tests passed 🔗 Commit SHA: 9165234 | Docs | View more details | Give us feedback! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5adad70bdd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Found a bug while building on top of metric probes for dynamic instrumentation. The bug came from an agent attempting dictionary lookups with dot notation (
cfg.max_ms) which applied without error, reportedACTIVEbut never emitted results (silly agent, dot notation doesnt work for dictionaries but i know expression language is tough and does nudge you to use it). Doing dictionary lookups with bracket notation (cfg["max_ms"]) worked fine in the same location. Part of the confusion here is whatACTIVEmeans in python, it reportsACTIVEas soon as it can validate the line is fine and can be installed, but it does not test the expression at this time. the expression tests after the first call, which is when it can callset_error.because we were swallowing that error, we never flippedUpdate: we do not report invalid conditions that can never fire asACTIVEtoERROR.ERRORin a regular case, we only report their errored state in the snapshot but we keep the probes asACTIVE. This PR matches that logic, and allows us to report this error at all.The core bug was that instead of reporting this was an error, we swallowed the exception:
probe.value(scope)raises insidesample().sample(),exit()ordo_exit()caught it, so all their remaining lines are skipped.continuewhich skips thecollector.push(signal)making it a silent failure.The proposed fix:
probe.value(scope)COND_ERRORorSKIP_COND_ERRORand returns normally._close_signal()without an exception, so we can callcollector.push(signal), which is now set toCOND_ERRORorSKIP_COND_ERROR.