Reframe weekly 404 monitor alert to lead with volume trend#268
Conversation
The weekly-404-monitor Slack report led with "Uncovered broken URLs: N (+M new this week)" — a raw distinct-URL count. In the current low-sample window (instrumentation is only weeks old) most broken URLs are one-off bot/crawler/old-bookmark hits, so that count churns heavily week-over-week and overstates the problem even when total 404 volume falls. - run_404_report.py: add REPORT_MIN_HITS (default 5) splitting uncovered URLs into significant gaps (>= threshold) vs long-tail noise; add trend_pct, report_min_hits, significant_uncovered_count, significant_new_gaps_count, top_significant_uncovered, long_tail_count to the JSON summary. Legacy counts kept for continuity; CSV unchanged. - SKILL.md: reframe Slack format to lead with the 404 volume trend, list only significant gaps, and roll the long tail into a single line. Clarify reporting threshold vs the Phase 2 automation threshold. Co-Authored-By: Oz <oz-agent@warp.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR reframes the weekly 404 monitor around total 404 volume, adds a configurable reporting threshold for significant uncovered URLs, and updates the Slack guidance to roll long-tail traffic into a summary line. I did not find security-specific findings, and the provided spec context contains no approved or repository spec commitments for this PR.
Concerns
- The new Slack trend template should define how to display weeks with no prior-week baseline so it does not render a null percentage.
- The new
REPORT_MIN_HITSenvironment knob would be more robust if it rejected non-positive or non-integer values explicitly.
Verdict
Found: 0 critical, 0 important, 2 suggestions
Approve with nits
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| *Total 404s this week:* {N} ({+N / -N vs last week}) | ||
| *Uncovered broken URLs:* {M} ({+N new this week}) | ||
| *404 volume:* {total_404s_this_week} this week — {▼|▲} {abs(trend_delta)} ({trend_pct}%) vs {total_404s_last_week} last week |
There was a problem hiding this comment.
💡 [SUGGESTION] trend_pct is null when the prior-week total is 0, so this template can render None%/null%; define a no-prior-week-baseline display before relying on the percentage placeholder.
| # in the Slack headline. Anything below this is long-tail noise (one-off | ||
| # bot/crawler/old-bookmark traffic) and is rolled up into a single count so | ||
| # it doesn't dominate the report. Tunable via env; see SKILL.md. | ||
| report_min_hits = int(os.environ.get("REPORT_MIN_HITS", "5")) |
There was a problem hiding this comment.
💡 [SUGGESTION] Validate REPORT_MIN_HITS as a positive integer before using it; 0, negatives, or non-numeric values either reintroduce long-tail noise or fail with a traceback.
The SQL groups broken URLs by their raw value (only ?/# stripped), so trailing-slash, case, and scheme/host variants (/foo, /foo/, /Foo, https://host/foo) came back as separate rows that were never re-merged after normalise_url. The same page was counted multiple times in the uncovered / new-gap counts and could appear twice in the top list, and its hits were split across variants (sometimes dropping a real gap below the reporting threshold). Add aggregate_by_norm() to collapse rows by normalise_url (summing hits) for both current and prior week. report_rows, prior_gaps, and current_urls now derive from the aggregated maps, and report_rows is re-sorted by hits. SKILL.md steps 1/3/6 document the per-page aggregation. Co-Authored-By: Oz <oz-agent@warp.dev>
- parse_min_hits(): treat REPORT_MIN_HITS as a positive integer; fall back to
5 (with a stderr warning) for non-numeric, zero, or negative values instead
of crashing or reintroducing long-tail noise.
- format_trend(): pre-format the headline volume line and return a
"no prior-week baseline yet" string when last week had 0 events, so the
Slack template ({trend_summary}) never renders a null percentage.
- SKILL.md: headline uses {trend_summary}; document the no-baseline fallback
and the positive-integer requirement for REPORT_MIN_HITS.
Co-Authored-By: Oz <oz-agent@warp.dev>
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR reframes the weekly 404 monitor around total 404 volume trend, aggregates raw broken URL rows by normalized page, and splits uncovered gaps into significant vs long-tail buckets for Slack reporting. I did not find blocking correctness, security, or spec-alignment issues; the attached spec context contains no approved spec commitments.
Concerns
- Non-blocking: aggregation currently runs after the existing raw query limit, so some low-hit variants can still be omitted before their hits are summed.
Verdict
Found: 0 critical, 0 important, 1 suggestions
Approve with nits
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| print(f" {len(current_week)} unique broken URLs found", file=sys.stderr) | ||
| # Collapse trailing-slash / case / host variants so each logical page is | ||
| # counted once and its hits are summed (see aggregate_by_norm). | ||
| current_agg = aggregate_by_norm(current_week) |
There was a problem hiding this comment.
💡 [SUGGESTION] Because query_404_events still applies LIMIT 500 before this aggregation, variants outside the raw top 500 are dropped before their hits can be summed. If the goal is accurate thresholding for variant-split pages, aggregate by normalized path in SQL or remove the raw cap before applying any report limit.
Addresses the remaining Oz review nit on PR #268: LIMIT 500 was applied to raw broken_url variants before aggregate_by_norm summed them, so a page whose hits split across trailing-slash/case/host variants could fall outside the top 500 and be undercounted against the reporting threshold. Normalisation + aggregation now happen in SQL so the cap applies to normalised pages; the Python aggregate_by_norm pass remains as an idempotent safety net. Co-Authored-By: Oz <oz-agent@warp.dev>
2 participants
Why
This week's
weekly-404-monitorSlack alert raised concern with "Uncovered broken URLs: 277 (+251 new this week)", but it wasn't a regression: total 404 volume actually dropped (483 → 395, −88). The monitor is only ~3 weeks old, and at ~1.4 hits per distinct URL the uncovered/new-gap counts are dominated by one-off bot/crawler/old-link traffic that churns ~90% week-over-week. No redirects were removed.The report also led with a raw distinct-URL count (the noisiest metric for this dataset) and counted trailing-slash/case/host variants of the same page as separate gaps.
What
1. Reframe the alert (
run_404_report.py,SKILL.md)REPORT_MIN_HITSenv knob (default 5) splitting uncovered URLs into significant gaps (hits >= threshold) vs long-tail noise.trend_pct,report_min_hits,significant_uncovered_count,significant_new_gaps_count,top_significant_uncovered,long_tail_count. Legacyuncovered_count/new_gaps_countretained for continuity.2. Collapse duplicate page variants in SQL (
run_404_report.py,SKILL.md)broken_url, so/foo,/foo/,/Foo, andhttps://host/fooarrived as separate rows. Becauseaggregate_by_norm()ran in Python after the query'sLIMIT 500, a page whose hits split across variants could fall outside the top 500 and be dropped before its hits were summed (flagged in Oz review).query_404_eventsnow normalises and groups by path in SQL (mirroringnormalise_url), so trailing-slash/case/host variants are summed into one row before the cap is applied. Each logical page is counted once and hit-splitting can no longer push a real gap below the threshold.aggregate_by_norm()is retained as an idempotent Python safety net.This week's data under the new format
Known caveat / follow-up
Coverage is matched on the normalized key (trailing slash + case stripped), but Vercel matches redirect sources exactly (slash- and case-sensitive). So a variant like
/foo/can be marked "covered" when only/foois a redirect source, even though/foo/actually 404s live (verified:/getting-started308s but/getting-started/404s). This can under-report a real slash-only gap. Not addressed here — the clean fix is a liveHEADcheck againstdocs.warp.devfor the top candidates, tracked as a follow-up.Notes
REPORT_MIN_HITS; revisit once more weeks of data accumulate.python3 -m py_compile; verifiednormalise_urlis idempotent andaggregate_by_normcollapses variants. End-to-end run needsMETABASE_API_KEYand live BigQuery (cloud env only), so not run locally.vercel.jsononmaincarries ~1,954 redirects, but there are no true duplicates — the/x+/x/pairs are intentional dual-form coverage (Vercel exact-matches). It's near Vercel's 2,048 build-failure ceiling, so bulk redirects is the path if it keeps growing.Conversation: https://staging.warp.dev/conversation/908b9678-fe69-4c85-aba1-207134484cca
Co-Authored-By: Oz oz-agent@warp.dev