Repository navigation
fix(mcp): drop the health verdict from clickstack_service_health - #3634
Conversation
The critical/degraded/healthy status and "no action needed" summaries judged services from service-wide totals, which can miss a regression in one endpoint, pod or segment and stop the agent investigating. Report the threshold flags, score and raw values instead. HDX-5877
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 2a97fde The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
the
review/tier-2
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
PR Review
2 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 2 minor 2 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep Review✅ No critical issues found. This is a focused, well-tested refactor of the agent-facing No P2 findings. 🔵 P3 nitpicks (3)
Reviewers: correctness, adversarial, api-contract, kieran-typescript, testing, maintainability, project-standards, previous-comments, agent-native, learnings-researcher were dispatched; findings synthesized from the orchestrator's full-diff analysis and the completed previous-comments reviewer (no actionable prior feedback — all 5 prior comments are automated bots). Testing gaps: No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
E2E Test Results✅ All tests passed • 462 passed • 1 skipped • 1017s
Tests ran across 4 shards in parallel. |
…alth An unflagged service with new errors could push a threshold-flagged one out of topN. Also print n/a instead of "n/ams" for a missing p95, pin the no-comparison summary, and note the span attribute rename (review major, minor, deep-review P2/P3).
|
|
||
| describe('rankServices', () => { | ||
| it('sorts by status, then by score with volume as a multiplier', () => { | ||
| it('sorts flagged services first, then by score with volume as a multiplier', () => { |
There was a problem hiding this comment.
🔵 minor — Renamed 'flagged services first' test never exercises the new crossedThreshold key in compareAssessments
Every non-steady service in this fixture is flagged and has a higher score than steady, so the test still passes if the crossedThreshold term is deleted from compareAssessments (rank.ts:191). The new buildReport test only covers the re-sort in report.ts. Add a case where an unflagged service outscores a flagged one, for example a high-volume service going from 1.0% to 1.9% errors against a chronic high_error_rate service with score 0, and assert that the flagged service ranks first.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| 'same window yesterday and ranked worst first. Each service gets a status ' + | ||
| '(critical / degraded / healthy), flags, and a one-line summary of which ' + | ||
| 'changes crossed a threshold. New or growing error groups come first, ' + | ||
| 'same window yesterday and ranked by how far each moved the wrong way. ' + |
There was a problem hiding this comment.
🔵 minor — Tool description says services are 'ranked by how far each moved the wrong way', but the actual order is flagged first, then new_errors, then score
A chronic high_error_rate service with score 0, or a flagged service with new errors, ranks above an unflagged service with a larger score. The description is the first thing agents read, so state the real order to match USAGE (report.ts:144): 'threshold-flagged services first, then services with new or growing errors, then by magnitude'.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
Summary
clickstack_service_healthno longer tells the agent whether a service is critical, degraded or healthy. A healthy service used to get "report this service as steady, no action needed". That verdict comes from service-wide totals, so a regression confined to one endpoint, pod or attribute segment can sit under every threshold while the tool tells the agent to stop looking. In thedeploy-regressioneval, main calls the service with the bug "Healthy".Each service now carries only evidence:
status: critical / degraded / healthyflagsandscorestayHealthy: … normal variance; report this service as steady, no action needed.No metric crossed a threshold vs yesterday: error rate 0.50% vs 0.49%, p95 120ms vs 115ms, requests 3000 vs 2900.CRITICAL: error rate 8.00% vs 0.50%.Crossed thresholds: error rate 8.00% vs 0.50%.statusCounts: {critical, degraded, healthy}servicesFlaggedmcp.service_health.critical_count,degraded_countmcp.service_health.flagged_countThe tool description and
usagenow say that thresholds apply to service-wide totals and point toclickstack_tablebreakdowns before a service is ruled out. "No flags is a valid result for a routine check" stays, so status reports are not pushed into escalating.statusCountsand per-servicestatusare gone from the response. Both are MCP output read by agents; nothing else in the repo consumes them.Evals
A/B with
packages/hdx-eval: branch (slot 99) vsmain(slot 98), both built from02f6fd060, identical seeded data,claude-opus-4-6.service-health-checkerror-root-causesegmented-regressiondeploy-regressionquiet-saturationThe change is neutral on scores. Per-run spread within an arm is 15–20 points, so no difference is significant (the largest has t ≈ 1);
segmented-regressionwent from −11 after 3 runs to +13 over the next 5. Routine checks did not get more alarmist: the judge scoredservice-health-check8 points higher on the branch. Its regex false-alarm checks scored it lower, but they mostly match negated phrases such as "not ongoing" and "not escalation-worthy". Agents make a few more tool calls in most scenarios once the tool stops saying "no action needed".How to reproduce
Starting point: local full stack (
yarn dev) with trace data, and an MCP client such asnpx @modelcontextprotocol/inspectorconnected to the local MCP endpoint (seeMCP.md).On
main:clickstack_list_sourcesand copy akind: "trace"source ID.clickstack_service_healthwith{ "sourceId": "<id>" }.status, and steady services read "Healthy: … no action needed."On this branch:
statusfield. Summaries state the crossed thresholds or the current vs comparison values, andsummary.servicesFlaggedreplacesstatusCounts.How to test on Vercel preview
N/A — non-UI change.
References
clickstack_service_health)