test(debugger): budget limit for capture expressions#7111
Conversation
|
|
🎉 All green!🧪 All tests passed 🔗 Commit SHA: bfc6cda | Docs | Datadog PR Page | Give us feedback! |
Add a Live Debugger budget test verifying that probes configured with capture expressions respect the 1 snapshot per second limit. Java and Python are marked as a known bug (DEBUG-5730) since capture-expression probes are not yet rate limited there.
e4490c0 to
f5f7c5b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4490c0cc6
ℹ️ 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".
| lines=self.method_and_language_to_line_number("Budgets", self.get_tracer()["language"]), | ||
| ) | ||
|
|
||
| def test_log_line_capture_expression_budgets(self): |
There was a problem hiding this comment.
Add the new test to the scenario map
This new test is not added to tests/test_the_test/scenarios.json, which is the map loaded by utils/scripts/compute_libraries_and_scenarios.py to translate manifest nodeids and changed test files into scenarios. Because the new manifest entries use this exact nodeid, future PRs that only adjust this test’s manifest status will not select DEBUGGER_PROBES_SNAPSHOT, so the CI impact calculation can silently miss the scenario. Please regenerate/update the scenario map entry for this test.
Useful? React with 👍 / 👎.
|
|
||
| # Probe budgets aren't exact and can take time to be applied, so we allow a range of 1-20 snapshots with | ||
| # capture expressions for 150 requests, matching the 1/s snapshot limit. | ||
| assert 1 <= snapshots_with_captures <= 20, ( |
There was a problem hiding this comment.
I'm slightly concerned about the stability of this test. From experience, every time we;ve introduced time-based assertions, we've eventually been hit by the fact that we don;t control the underlying machine the test runs on, which introduces a lot of non-determinism.
In the end, we often had to expand the asserted window significantly to reduce flakiness, which defeats the purpose of the assertion.
Let's give this a try, but if it turns out that we get too many failures that cannot be correlated with a real issue, we'll need to rethink this approach.
There was a problem hiding this comment.
AFAIK the lack of stability for the debugger tests was always a missing waiting signal, we should be fairly deterministic as long as we gate on the proper signals
cbeauchesne
left a comment
There was a problem hiding this comment.
Approved, with one comment :)
Motivation
Extends the Live Debugger budget tests to ensure that probes configured with capture expressions respect the 1 snapshot per second limit, the same budget already enforced for full-snapshot log probes.
Changes
probe_capture_expressions_budgets.json: a line probe on theBudgetsmethod withcaptureSnapshot: falseand acaptureExpressionsentry on theloopsparameter.Test_Debugger_Probe_Budgets::test_log_line_capture_expression_budgets: fires 150 requests and asserts that only 1-20 snapshots carrying capture expressions are emitted (matching the existingtest_log_line_budgetstolerance for the ~1/s budget).DEBUG-5730) inmanifests/java.yml: capture-expression probes currently emit on every invocation (150/150) and are not yet rate limited.Testing
Ran
DEBUGGER_PROBES_SNAPSHOTlocally for Java:got 150 ... should be 1-20), confirming the budget is bypassed.DEBUG-5730bug declaration, the test is skipped as expected.🤖 Generated with Claude Code