filter-out-the-ping-route-url-from-sentry-487 #489

Merged

johnsturgeon merged 3 commits from filter-out-the-ping-route-url-from-sentry-487 into main

2026-09-07 11:00:22 +02:00

johnsturgeon commented

2026-09-07 10:44:54 +02:00

Owner

No description provided.

johnsturgeon added 2 commits

2026-09-07 10:44:55 +02:00

Filtering 'ping' out of the sentry log 37ee3ec529

Merge remote-tracking branch 'origin/main' into filter-out-the-ping-route-url-from-sentry-487

Claude Code Review / claude-review (pull_request) Successful in 37s

Details

Python FastAPI Jinja Linting / build (pull_request) Failing after 3s

Details

Tests / pytest (pull_request) Failing after 3s

Details

62decb4347

forgejo-actions commented

2026-09-07 10:45:32 +02:00

Summary

This PR adds a Sentry before_send_transaction hook to drop /ping health-check transactions (issue #487), which is a clean, correct, and well-commented one-function change. The functional change looks good; the main concern is that the uv.lock diff bundles a large, undisclosed transitive-dependency refresh that is unrelated to the stated purpose.

Worth fixing

uv.lock — unrelated dependency churn bundled into a one-line feature. Beyond the version bump to 2.31.1, the lock refresh bumps ~15 transitive packages (including major jumps like isort 6→9 and rich 14→15) and pulls in two brand-new packages, agent-detector 2.0.0 and mypy-extensions 1.1.0, the former as a new transitive dep of fastapi-cloud-cli 0.25.0. None of this is mentioned in the PR (which has no description) and none of it is needed to filter /ping. This inflates the review surface and quietly expands the supply-chain footprint. Please either split the lock refresh into its own PR or, at minimum, call out in the description that a uv lock refresh was intentional and confirm agent-detector is a trusted upstream addition. Note pyproject.toml:9-12 explicitly frames the pinning policy as "change the mechanism and not the resolution" — a wholesale resolution bump runs against that stated intent.
No test for _drop_ping_transaction (app/main.py:85). It's a pure function that's trivial to unit test, and the repo already has a tests/ suite. A couple of assertions would lock in the behavior and guard the (event.get("request") or {}) fallback: e.g. {"request": {"url": "http://x/ping"}} → None, {"request": {"url": "http://x/picks"}} → returns the event unchanged, and {} (no request key) → returns the event. Worth adding since the whole point of the change is that these events get dropped.

Nits

app/main.py:85 — Sentry's documented idiom for skipping a route entirely is a traces_sampler returning 0 for /ping, which avoids sampling the transaction in the first place rather than building it and discarding it at send time. before_send_transaction works and is perfectly acceptable here; just noting the alternative in case you'd prefer not to pay the tracing cost for a constantly-polled endpoint.

## Summary This PR adds a Sentry `before_send_transaction` hook to drop `/ping` health-check transactions (issue #487), which is a clean, correct, and well-commented one-function change. The functional change looks good; the main concern is that the `uv.lock` diff bundles a large, undisclosed transitive-dependency refresh that is unrelated to the stated purpose. ## Worth fixing - **`uv.lock` — unrelated dependency churn bundled into a one-line feature.** Beyond the version bump to `2.31.1`, the lock refresh bumps ~15 transitive packages (including major jumps like `isort` 6→9 and `rich` 14→15) and pulls in two brand-new packages, `agent-detector` 2.0.0 and `mypy-extensions` 1.1.0, the former as a new transitive dep of `fastapi-cloud-cli` 0.25.0. None of this is mentioned in the PR (which has no description) and none of it is needed to filter `/ping`. This inflates the review surface and quietly expands the supply-chain footprint. Please either split the lock refresh into its own PR or, at minimum, call out in the description that a `uv lock` refresh was intentional and confirm `agent-detector` is a trusted upstream addition. Note `pyproject.toml:9-12` explicitly frames the pinning policy as "change the mechanism and not the resolution" — a wholesale resolution bump runs against that stated intent. - **No test for `_drop_ping_transaction` (`app/main.py:85`).** It's a pure function that's trivial to unit test, and the repo already has a `tests/` suite. A couple of assertions would lock in the behavior and guard the `(event.get("request") or {})` fallback: e.g. `{"request": {"url": "http://x/ping"}}` → `None`, `{"request": {"url": "http://x/picks"}}` → returns the event unchanged, and `{}` (no request key) → returns the event. Worth adding since the whole point of the change is that these events get dropped. ## Nits - `app/main.py:85` — Sentry's documented idiom for skipping a route entirely is a `traces_sampler` returning `0` for `/ping`, which avoids sampling the transaction in the first place rather than building it and discarding it at send time. `before_send_transaction` works and is perfectly acceptable here; just noting the alternative in case you'd prefer not to pay the tracing cost for a constantly-polled endpoint.

johnsturgeon added 1 commit

2026-09-07 10:51:23 +02:00

Updated version

Claude Code Review / claude-review (pull_request) Successful in 1m7s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 28s

Details

Tests / pytest (pull_request) Successful in 14s

Details

Release on merge / release (pull_request) Successful in 0s

Details

854393d496