test(monitoring): encode prod telemetry target inventory (#9587) - #11099
test(monitoring): encode prod telemetry target inventory (#9587)#11099aryanorastar wants to merge 3 commits into
Conversation
…re#9587) Add a declared expected-targets inventory, static contract tests for scrape wiring/routing/no-data semantics, and PR CI selector coverage for monitoring contract sources — without live Grafana or Instatus changes. Co-authored-by: Cursor <cursoragent@cursor.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 5 files
Confidence score: 4/5
backend/tests/unit/test_monitoring_telemetry_contract.pyhas two contract-test blind spots: it only checks inventory->scrape existence (not scrape->inventory) and uses a loose substring match for enforced jobs, so undeclared scrape jobs or incorrectly enforced rules can slip through CI and reach production monitoring unnoticed — add the reverse set assertion and assert against the specific rule field/expression structure instead of raw substring presence.backend/scripts/select_backend_unit_tests.pynow depends on two separate source tuples for monitoring, which can drift and cause monitoring-related changes to skip the contract tests (or run them unexpectedly), weakening PR safety signals — centralize the paths in a sharedMONITORING_CONTRACT_SOURCESconstant and reference it from both selection points.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/scripts/select_backend_unit_tests.py">
<violation number="1" location="backend/scripts/select_backend_unit_tests.py:318">
P3: Adding or removing a monitoring source now requires editing two independent tuples; if they drift, a changed file can be ignored or selected without the monitoring tests. A shared `MONITORING_CONTRACT_SOURCES` tuple reused by both branches would keep the selector contract consistent.</violation>
</file>
<file name="backend/tests/unit/test_monitoring_telemetry_contract.py">
<violation number="1" location="backend/tests/unit/test_monitoring_telemetry_contract.py:68">
P2: The existence check is one-directional, so a scrape job added to prod additionalScrapeConfigs without a matching inventory entry passes silently. Add the reverse assertion so added-but-undeclared jobs are caught on PRs, matching the stated drift-detection goal; both sets are currently equal so this stays green.</violation>
<violation number="2" location="backend/tests/unit/test_monitoring_telemetry_contract.py:119">
P3: The enforcement check only asserts the enforced job name appears somewhere in the serialized alert rule (via substring search), so it would still pass if the job name appeared in an annotation or label but was accidentally removed from the `up{job=~"..."}` selector that actually triggers the alert. This is a static string-membership tripwire rather than a check that the job is wired into the query expression, so it can silently miss selector drift. Consider parsing the PromQL expression (e.g. extracting the `up{job=~"..."}` alternation from `data[].model.expr`) and asserting the enforced job is a member of that alternation, or at minimum label this test/assertion explicitly as a static tripwire per AGENTS.md guidance.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if job.get('coverage_status') != 'enforced': | ||
| continue | ||
| assert job.get('coverage_alert') == 'omi-journey-scrape-missing' | ||
| assert job['name'] in blob, ( |
There was a problem hiding this comment.
P3: The enforcement check only asserts the enforced job name appears somewhere in the serialized alert rule (via substring search), so it would still pass if the job name appeared in an annotation or label but was accidentally removed from the up{job=~"..."} selector that actually triggers the alert. This is a static string-membership tripwire rather than a check that the job is wired into the query expression, so it can silently miss selector drift. Consider parsing the PromQL expression (e.g. extracting the up{job=~"..."} alternation from data[].model.expr) and asserting the enforced job is a member of that alternation, or at minimum label this test/assertion explicitly as a static tripwire per AGENTS.md guidance.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/unit/test_monitoring_telemetry_contract.py, line 119:
<comment>The enforcement check only asserts the enforced job name appears somewhere in the serialized alert rule (via substring search), so it would still pass if the job name appeared in an annotation or label but was accidentally removed from the `up{job=~"..."}` selector that actually triggers the alert. This is a static string-membership tripwire rather than a check that the job is wired into the query expression, so it can silently miss selector drift. Consider parsing the PromQL expression (e.g. extracting the `up{job=~"..."}` alternation from `data[].model.expr`) and asserting the enforced job is a member of that alternation, or at minimum label this test/assertion explicitly as a static tripwire per AGENTS.md guidance.</comment>
<file context>
@@ -0,0 +1,157 @@
+ if job.get('coverage_status') != 'enforced':
+ continue
+ assert job.get('coverage_alert') == 'omi-journey-scrape-missing'
+ assert job['name'] in blob, (
+ f"enforced job {job['name']!r} must appear in omi-journey-scrape-missing expr"
+ )
</file context>
Co-authored-by: Cursor <cursoragent@cursor.com>
…ardware#9587) Three real gaps cubic flagged: - test_additional_scrape_jobs_exist_in_prod_values was one-directional: it checked every inventory job exists in prod additionalScrapeConfigs, but not the reverse. A job added straight to prod values without a matching inventory entry is undeclared monitoring drift and would silently pass. Added the reverse assertion. - test_enforced_coverage_alert_includes_declared_jobs fell back to substring-searching json.dumps(scrape_rule) when data[].model.expr was empty, so a job name appearing in any unrelated field (uid, title, a label) could pass even though the actual alerting expression was wrong or missing. Require expr directly and fail loudly if extraction failed, instead of silently widening the search surface. - select_backend_unit_tests.py had the same 6-item monitoring source tuple duplicated verbatim in AREA_TESTS and in is_selectable_backend_path's intentional-exception check — a classic copy-paste drift risk (a future maintainer changes one without the other, and CI selection silently stops matching the monitoring unit contract). Extracted MONITORING_CONTRACT_SOURCES as the single source both now reference. Verified: ran all 11 test_monitoring_telemetry_contract.py assertions directly against the real prod config files (backend/tests/unit/conftest.py pulls in google-cloud deps not installed in this sandbox, so bypassed pytest collection and called each test function directly) — 11/11 pass, including the new stricter reverse/expr checks, confirming no hidden pre-existing drift in the real inventory/values/alert-rules files. Sanity-checked is_selectable_backend_path and the AREA_TESTS monitoring mapping resolve identically after the dedup. black --check clean on both files. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed on `b8078ce5`:
Verified against the real config files, not mocks: ran all 11 assertions in `test_monitoring_telemetry_contract.py` directly (bypassed pytest collection since `conftest.py` pulls in google-cloud deps not available in this sandbox) — 11/11 pass including the new stricter checks, confirming there's no hidden pre-existing drift in the actual inventory/values/alert-rules files that the old one-directional test was missing. Sanity-checked the selector script resolves identically after dedup. `black --check` clean. |
Git-on-my-level
added
needs-maintainer-review
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for tightening this after the earlier review. The current code direction looks useful: the inventory is checked both ways against prod additionalScrapeConfigs, the enforced coverage assertion now reads the Grafana expression directly, and the monitoring test-selection paths are centralized.
One blocking item remains before this can merge: the Repo Checks / Hygiene gate is failing on failure-class-protocol because the fix(backend): address cubic review on telemetry contract test (#9587) commit does not include the required Failure-Class: FC-<slug> | new | none declaration. Since this PR changes workflow/test-selection behavior and production monitoring contracts, I’m also leaving it for human maintainer review after CI is green.
Suggested next step: amend the fix commit message/body with the appropriate failure-class declaration (likely Failure-Class: none if this is not tied to an existing/new semantic failure class), then rerun CI.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
|
@Git-on-my-level the Could you dismiss the Changes Requested / re-review when you have a moment? Happy to escalate with |
|
@Git-on-my-level need human response The only remaining merge blocker is your stale Changes Requested from the Failure-Class hygiene note. On current head
Please dismiss the Changes Requested / re-review so the merge gate can clear. No further author code work pending. |
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I rechecked the current head (b8078ce5) against the earlier automation feedback.
What looks good now:
- The inventory/prod
additionalScrapeConfigscheck is now bidirectional, so undeclared prod scrape jobs should fail the contract. - The enforced coverage check now reads the Grafana alert expression directly instead of falling back to matching the whole serialized rule.
- The monitoring test-selection sources are centralized via
MONITORING_CONTRACT_SOURCESand select the monitoring contract tests for the relevant chart/alert/inventory paths. - Direct execution of the 11 telemetry contract assertions passes locally against the real repo files; the targeted selector check also selects the expected monitoring tests. Full local pytest collection was blocked by missing sandbox deps (
googlefrombackend/tests/unit/conftest.py), but the GitHub backend unit suite is green on this head.
I’m not seeing another code-change blocker from this pass. Because this PR changes workflow/test-selection behavior and production monitoring contract coverage, I’m still leaving final merge sign-off to a human maintainer rather than formal automated approval.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
|
@Git-on-my-level need human response Merge is still blocked by your earlier Changes Requested from the Failure-Class hygiene note, even though your latest pass on Current state:
Please dismiss the stale Changes Requested / leave a human APPROVE or clear the gate so this can land. No further author code work pending. |
|
@Git-on-my-level need human response — dismiss stale CR. Head |
|
@Git-on-my-level follow-up: PR #11099 is fully CI-green at b8078ce, with @kodjima33 approval. Your latest re-review found no code-change blocker and confirmed the 11 telemetry assertions plus selector coverage. The older CHANGES_REQUESTED review remains the only merge blocker; please dismiss it or leave the formal maintainer approval when ready. |
3 participants
Summary
backend/charts/monitoring/expected-targets.prod.yaml.omi-journey-scrape-missingcoverage for enforced jobs, Telegram receiver +instatus_componentlabels, and scrape-healthnoDataState.backend/charts/monitoring/**contract sources into unit-test selection so inventory/values drift is caught on PRs.Test plan
pytest tests/unit/test_monitoring_telemetry_contract.pypytest tests/unit/test_workflow_contracts.py::test_selector_docs_and_flat_utils_do_not_force_full_suite_via_globsFailure-Class: none