Repository navigation
feat(observability): poll_runs + "last polled" line, opt-in OpenTelemetry and /metrics (#359); 1.64.0 - #387
Conversation
…r pass — started/finished, ats_only, leads_added, and errors naming only that account's failing sources (0053; applytrack_poller gets INSERT on it and nothing else) — read by GET /api/poll/status and shown under the Applications heading as "Last polled 12 min ago · 2 sources failing", the reasons in a keyboard disclosure; pruned by Retention__PollRunDays (30, never the newest pass of either kind); opt-in OpenTelemetry via OTEL_EXPORTER_OTLP_ENDPOINT (ASP.NET Core, HttpClient, Npgsql traces with route templates, host-only outbound URLs, Telegram untraced) and a Prometheus /metrics served only on Metrics__Port, both off by default; meters for LLM latency and errors, agent runs by provider and outcome, and submit-queue depth, none carrying a tenant, host or URL (#359); 1.64.0 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXYffreKMhXLNgc2wdhsiw
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 38 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
📝 SummarySummary by CodeRabbit
WalkthroughThe change adds per-tenant poll-run history and retention, exposes poll status through an API and the web app, and adds opt-in OpenTelemetry and Prometheus instrumentation with configuration and tests. ChangesPoll history and observability
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant PollWorker
participant PollRepo
participant PollRuns
participant PollStatusApi
participant WebApp
PollWorker->>PollRepo: record pass results
PollRepo->>PollRuns: insert tenant poll-run row
WebApp->>PollStatusApi: GET /api/poll/status
PollStatusApi->>PollRuns: read latest tenant passes
PollRuns-->>PollStatusApi: pass times, lead counts, and errors
PollStatusApi-->>WebApp: return poll status
Merge Risk: ⚪ Minimal · up to No identified issue blocks merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Poll status remains tenant-scoped, but enabling the metrics port also creates another route to the application, and failed history writes can leave the displayed status stale. Telemetry privacy coverage is not yet complete. The default production configuration does not publish the metrics port. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 37.04% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 81 functions across 18 files. (11 skipped: 11 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments (28)
api/ApplyTrack.Api/Migrations/0053_poll_runs.sql (1)
11-21: LGTM!api/ApplyTrack.Api/Migrations/Always/poller_role.sql (1)
34-35: LGTM!src/applytrack/db.py (1)
196-213: LGTM!api/ApplyTrack.Api/Data/Retention.cs (1)
137-149: LGTM!api/ApplyTrack.Api.Tests/AccountEndpointTests.cs (1)
207-223: LGTM!api/ApplyTrack.Api.Tests/DbRoleTests.cs (1)
78-87: LGTM!api/ApplyTrack.Api.Tests/RetentionTests.cs (1)
187-211: LGTM!tests/test_poll.py (1)
1518-1629: LGTM!README.md (1)
1118-1150: LGTM!api/ApplyTrack.Api/Data/PollRunRepo.cs (1)
1-79: LGTM!api/ApplyTrack.Api/Endpoints/AppsEndpoints.cs (1)
140-143: LGTM!api/ApplyTrack.Api/Program.cs (1)
85-86: LGTM!Also applies to: 229-233, 359-361
api/ApplyTrack.Api/wwwroot/index.html (1)
70-73: LGTM!api/ApplyTrack.Api/wwwroot/app.js (1)
3734-3773: LGTM!api/ApplyTrack.Api/wwwroot/app.css (1)
238-243: LGTM!api/ApplyTrack.Api.Tests/ObservabilityTests.cs (1)
1-302: LGTM!tests/web/accessibility.spec.js (1)
120-130: LGTM!Also applies to: 1288-1302
.env.example (1)
109-111: 🎯 Functional CorrectnessThe Compose files already map
METRICS_PORTtoMetrics__Port, so the proposed rename is unnecessary.api/ApplyTrack.Api/Observability/AppMetrics.cs (1)
1-78: LGTM!api/ApplyTrack.Api/Llm/OpenAiCompatibleLlmClient.cs (1)
59-77: LGTM!api/ApplyTrack.Api/Agent/AgentWorker.cs (1)
267-271: LGTM!Also applies to: 740-740, 760-760, 777-777, 787-787
api/ApplyTrack.Api/ApplyTrack.Api.csproj (1)
8-8: LGTM!Also applies to: 20-25
docker-compose.production.yml (1)
51-60: LGTM!Also applies to: 111-115
docker-compose.yml (1)
49-59: LGTM!Also applies to: 117-121
pyproject.toml (1)
7-7: LGTM!src/applytrack/__init__.py (1)
5-5: LGTM!BACKLOG.md (1)
49-49: LGTM!api/ApplyTrack.Api/Observability/Telemetry.cs (1)
92-96: 🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟠 Major | ⚡ Quick winSensitive Data Exposure
Reachability: External
Exploitability: Moderate
CWE: CWE-532 — Insertion of Sensitive Information into Log File
⚠️ Unverified finding
Verification did not complete.Scrub the inbound span on requests that end in an exception.
EnrichWithHttpResponseruns only when a response is produced through the normal path. The ASP.NET Core instrumentation setsurl.pathandurl.querywhen the request starts. If a request throws, theEnrichWithExceptionpath can end the span beforeScrubRequestruns. The exported span then keeps the raw path, for example a tenant slug, and the query, for example?token=. This contradicts the stated contract that traces "never" carry the raw path or query. CallScrubRequestfromEnrichWithHttpRequestas well, or fromEnrichWithException, so every span is scrubbed. The route endpoint is resolved later, so keep the call inEnrichWithHttpResponsetoo.Proposed fix
.AddAspNetCoreInstrumentation(o => { o.Filter = ctx => !ctx.Request.Path.StartsWithSegments(MetricsPath); + o.EnrichWithHttpRequest = (activity, request) => ScrubRequest(activity, request.HttpContext); o.EnrichWithHttpResponse = (activity, response) => ScrubRequest(activity, response.HttpContext); + o.EnrichWithException = (activity, _) => + { + if (activity.GetTagItem("url.query") is not null || activity.GetTagItem("url.path") is not null) + activity.SetTag("url.query", null); + }; })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 95066b2d-7029-4b13-ad55-68ad638b4fbd
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
.env.exampleBACKLOG.mdREADME.mdapi/ApplyTrack.Api.Tests/AccountEndpointTests.csapi/ApplyTrack.Api.Tests/DbRoleTests.csapi/ApplyTrack.Api.Tests/ObservabilityTests.csapi/ApplyTrack.Api.Tests/RetentionTests.csapi/ApplyTrack.Api/Agent/AgentWorker.csapi/ApplyTrack.Api/ApplyTrack.Api.csprojapi/ApplyTrack.Api/Data/PollRunRepo.csapi/ApplyTrack.Api/Data/Retention.csapi/ApplyTrack.Api/Endpoints/AppsEndpoints.csapi/ApplyTrack.Api/Llm/OpenAiCompatibleLlmClient.csapi/ApplyTrack.Api/Migrations/0053_poll_runs.sqlapi/ApplyTrack.Api/Migrations/Always/poller_role.sqlapi/ApplyTrack.Api/Observability/AppMetrics.csapi/ApplyTrack.Api/Observability/Telemetry.csapi/ApplyTrack.Api/Program.csapi/ApplyTrack.Api/wwwroot/app.cssapi/ApplyTrack.Api/wwwroot/app.jsapi/ApplyTrack.Api/wwwroot/index.htmldocker-compose.production.ymldocker-compose.ymlpyproject.tomlsrc/applytrack/__init__.pysrc/applytrack/db.pysrc/applytrack/worker.pytests/test_poll.pytests/web/accessibility.spec.js
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Python — lint + test + audit
- GitHub Check: .NET — test + audit
- GitHub Check: Web — WCAG checks
🧰 Additional context used
📓 Path-based instructions (2)
Source excerpt: **Core API: .NET 10** — ASP.NET Core Minimal APIs on Kestrel.
📄 CodeRabbit inference engine (CLAUDE.md)
Files:
api/ApplyTrack.Api/Program.cs
Source excerpt: `api/` — the .NET solution (`ApplyTrack.Api`), with the SPA in `wwwroot/`.
📄 CodeRabbit inference engine (CLAUDE.md)
Files:
api/ApplyTrack.Api/wwwroot/app.cssapi/ApplyTrack.Api/wwwroot/index.htmlapi/ApplyTrack.Api/wwwroot/app.js
🪛 ast-grep (0.45.3)
api/ApplyTrack.Api/wwwroot/app.js
[warning] 3763-3766: Avoid assigning untrusted data to innerHTML/outerHTML or document.write
Context: el.innerHTML = <span>${when}</span>${n ? · <details class="poll-failing"${wasOpen ? " open" : ""}>
${n} source${n === 1 ? "" : "s"} failing
- ${failing.map((f) =>
<li><span class="mono">${escapeHtml(f.source)}</span>: ${escapeHtml(f.error)}</li>).join("")} : ""}Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(inner-outer-html)
[warning] 3771-3771: Avoid using the initial state variable in setState
Context: setInterval(() => { if (!document.hidden && !$("#poll-status")?.hidden) showPollStatus(); }, POLL_STATUS_MS)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🪛 OpenGrep (1.30.0)
api/ApplyTrack.Api/wwwroot/app.js
[WARNING] 3764-3767: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
🪛 Squawk (2.64.0)
api/ApplyTrack.Api/Migrations/0053_poll_runs.sql
[warning] 17-17: Using 32-bit integer fields can result in hitting the max int limit. Use 64-bit integer values instead to prevent hitting this limit.
(prefer-bigint-over-int)
… — every other path there is a 404, so a route opened for a scraper is not a second way into the app around its proxy; inbound spans are scrubbed when the request starts as well as at its response, so one that throws first keeps neither the raw path nor the query (CodeRabbit on #387) (#359) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXYffreKMhXLNgc2wdhsiw
1 participant
What
poll_runs(migration 0053): the Python poller writes one row per account per pass:started_at,finished_at,ats_only(fast lane),leads_added,errors([{source, error}], only the sources that account draws on, plus its own signed-in sources and the pass itself).applytrack_pollergets INSERT only on it.users.last_polled_at(Feature: operator admin CLI (users, allowlist, usage, disable) and a Backup & restore runbook #358) stays the operator's cross-account stamp foradmin usage;poll_runsis the per-pass detail. Pruned by the retention sweep afterRetention__PollRunDays(30), never an account's newest pass of either kind.GET /api/poll/status→{last_polled_at, last_leads_added, failing}. The failing list comes from the newest full pass. If a fast-lane pass is newer, the ATS-board entries come from that pass instead.<details>disclosure. It is vanilla JS, refreshes every minute, is not a live region, and keeps the disclosure open across refreshes.OTEL_EXPORTER_OTLP_ENDPOINT: ASP.NET Core, HttpClient and Npgsql traces plus metrics. Unset, the SDK is not registered at all. Traces keep route templates (never the slug or query) and only scheme+host for outbound URLs. Telegram, which has its bot token in the URL, is not traced./metricsonly onMetrics__Port, checked against the connection's local port (a forged Host header does not open it). The app's port answers 404.applytrack.llm.duration/applytrack.llm.errors,applytrack.agent.runs(provider, outcome, dry_run),applytrack.submit_requests.pending. Every tag comes from a fixed vocabulary. HttpClient metrics dropserver.addressand Npgsql metrics drop the pool name (the connection string)..env.exampleand both compose files pass the new settings through, empty = off.Tests
PollRepo.record_runSQL.ObservabilityTests. Covers: off by default (no MeterProvider,/metrics404); scrape works on the metrics port only and ignores a spoofed Host; the scrape carries no tenant or host; bad port refused; URL composition; trace scrubbing; LLM metrics;/api/poll/statusmerge, tenant isolation and 401. Also: poller role grants (INSERT only onpoll_runs), account-delete cascade, retention pruning.dotnet test1132 passed; NuGet audit clean; ruff/mypy/bandit/pytest green;npm run test:web80 passed.Note: local
pip-auditagainstuv.lockflags anyio 4.14.0 (fixed in 4.14.2). That is out of scope here.Closes #359
Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/
🤖 Generated with Claude Code
https://claude.ai/code/session_01SXYffreKMhXLNgc2wdhsiw