Skip to content

feat(selfhost): sampled Sentry spans for review latency - #1904

Closed
nickmopen wants to merge 1 commit into
JSONbored:mainfrom
nickmopen:feat/selfhost-sentry-review-spans
Closed

feat(selfhost): sampled Sentry spans for review latency#1904
nickmopen wants to merge 1 commit into
JSONbored:mainfrom
nickmopen:feat/selfhost-sentry-review-spans

Conversation

@nickmopen

@nickmopen nickmopen commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds sampled Sentry tracing across the review pipeline so operators can answer "why was this review slow?" / "which stage failed?" without digging through scattered logs (#1734). A shared span helper keeps instrumentation consistent and easy to audit; it coexists with the existing OpenTelemetry spans by reusing the same boundaries.

Opt-in & safe by construction

  • No-op until configured. Spans are emitted only when SENTRY_TRACES_SAMPLE_RATE > 0 (default 0) — independent of error capture. resolveTracesSampleRate clamps to [0,1] and treats a malformed value as off, so a typo can't flood the tracer. With sampling off, no span is started and no trace traffic is emitted.
  • Never sensitive payloads. sentrySpanAttributes keeps only the safe, low-cardinality subset (drops secret-keyed + null, keeps finite numbers/booleans, truncates strings) — never prompts, diffs, review bodies, tokens, headers, or config blobs.

What it does

  • withSentrySpan(name, attributes, fn) — runs fn inside a Sentry span (auto-closed, marked errored if fn throws) when sampling is on; a pure pass-through otherwise.
  • withReviewSpan(name, attributes, fn, options) — the shared wrapper that opens one boundary feeding both tracers (OpenTelemetry + Sentry); each side independently no-ops when its backend is off.
  • Wired at the existing trace boundaries for the latency-heavy stages: the queue-job span (whole-review latency — GitHub reads, REES call, AI, gate, publish all run inside it) and the nested AI-provider span. A sampled review therefore produces a connected trace with the major stages visible and slow/failed stages filterable.

Acceptance criteria

  • ✅ Sampling enabled → one review produces a connected trace (queue-job span with the AI-provider span nested).
  • ✅ Sampling disabled → behavior unchanged, no Sentry trace traffic (the helpers reduce to fn()).
  • ✅ Slow/failed stages filterable without exposing sensitive payloads (safe attributes + span error status).

Validation

Node 24, from the repo root:

npm run typecheck        # clean
npx vitest run test/unit/selfhost-sentry.test.ts   # 47 pass

Unit tests cover: rate resolution (default/clamp/non-finite), attribute scrubbing (secrets/null/NaN/non-scalar dropped, truncation), no-op when off (DSN set, rate 0 → no span), span emission + safe attributes when on, withReviewSpan driving both tracers, and error propagation. The instrumented call-site lines (selfhost.queue.job, selfhost.ai.provider) are covered by the existing queue/AI suites. Documented in .env.example.

Part of #998. Closes #1734

Add a shared Sentry span helper and wire it at the review pipeline's existing trace boundaries so operators can
answer "why was this review slow / which stage failed" without digging through logs (JSONbored#1734).

Tracing stays strictly opt-in: spans are a complete no-op until SENTRY_TRACES_SAMPLE_RATE is configured above 0
(default 0), independent of error capture. resolveTracesSampleRate clamps the rate to [0,1] and treats a malformed
value as off. withSentrySpan runs its callback inside a Sentry span only when sampling is on, tagging it with the
safe, low-cardinality attribute subset (secrets/null dropped, strings truncated) — never prompts, diffs, tokens,
or bodies. withReviewSpan composes one boundary across BOTH tracers (OpenTelemetry + Sentry), each independently
no-op when its backend is off, and replaces the existing withOtelSpan boundaries for the queue-job and AI-provider
stages — so a sampled review produces a connected trace (whole-review job span with the AI span nested) where slow
or failed stages are filterable.

Fully unit-tested (sampling off → no span started; on → named span with safe attributes; error propagation; rate
resolution; attribute scrubbing). Documented in .env.example.
@nickmopen
nickmopen requested a review from JSONbored as a code owner June 30, 2026 20:14
@dosubot dosubot Bot added the size:M label Jun 30, 2026
@loopover-orb

loopover-orb Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-06-30 22:29:56 UTC

6 files · 1 AI reviewer · no blockers · readiness 55/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
The diff adds opt-in Sentry tracing behind `SENTRY_TRACES_SAMPLE_RATE`, threads it through the existing queue and AI provider span boundaries, and keeps Sentry span attributes constrained to scalar, non-secret values. The core wiring is coherent: `withReviewSpan` preserves the existing OpenTelemetry boundary while nesting the new Sentry span inside it, and sampling defaults to a no-op. The main maintainability gap is that the new shared wrapper lives in `sentry.ts`, which makes Sentry the owner of a cross-tracer abstraction and will encourage broader modules to import Sentry for non-Sentry concerns.

Nits — 5 non-blocking
  • nit: `src/selfhost/sentry.ts:223` makes `withReviewSpan` a cross-tracer helper inside the Sentry module, so future non-Sentry callers now import from `./sentry` just to get the normal review span boundary; consider moving this wrapper to `otel.ts` or a small neutral tracing module to keep ownership clear.
  • nit: `test/unit/selfhost-sentry.test.ts:776` verifies thrown errors propagate, but the Sentry mock cannot prove the real SDK marks the span as failed; either avoid asserting that behavior in comments or add a narrow SDK-contract test if this repo has a precedent for those.
  • Move `withReviewSpan` out of `src/selfhost/sentry.ts` or document why Sentry intentionally owns the combined tracing boundary; otherwise `src/selfhost/ai.ts`, `pg-queue.ts`, and `sqlite-queue.ts` now depend on the Sentry module for a generic tracing concern.
  • In `test/unit/selfhost-sentry.test.ts`, make the error-path test assert only the wrapper contract this code controls, or enhance the mock to record the callback failure if you want the test name/comment to stay as-is.
  • Readiness score is below the configured threshold — Use the readiness panel as advisory maintainer context; the score does not block this PR.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ No-issue rationale PR body explains why no issue is linked.
Related work ⚠️ 3 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High review scope from cached public metadata (size label size:M; no linked issue context).
Validation posture ❌ 5/25 Preflight is holding this PR; address the blocker before review.
Contributor workload ✅ 10/10 Author activity: 95 registered-repo PR(s), 64 merged, 1 issue(s).
Contributor context ✅ Confirmed Gittensor contributor nickmopen; Gittensor profile; 95 PR(s), 1 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
Contributor next steps
  • Review top overlaps.
  • Add a concise scope and risk note.
  • Fix the blocker.
  • Triage stale or unlinked PRs.
  • Refresh registry data or choose a registered active repo.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
  • Check active issues and PRs before submitting.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added gittensor gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. labels Jun 30, 2026
@JSONbored JSONbored added the gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. label Jun 30, 2026
@loopover-orb loopover-orb Bot removed the gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. label Jun 30, 2026

@JSONbored JSONbored left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is solid, however, we do need to fix this:

The main maintainability gap is that the new shared wrapper lives in `sentry.ts`, which makes Sentry the owner of a cross-tracer abstraction and will encourage broader modules to import Sentry for non-Sentry concerns.

Closing, resubmit once that's fixed and it should be an easy merge. This is a high priority issue so it deserves a gittensor:priority label when merged.

@JSONbored JSONbored closed this Jun 30, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jun 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier.

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

observability(tracing): add sampled Sentry spans for review latency

2 participants