feat(quoter-bot): add opt-in opentelemetry stack - #206
Conversation
Adds @repo/telemetry: an env-gated OTLP/HTTP+JSON pipeline (traces + metrics), diagnostics_channel-based undici instrumentation with unconditional URL redaction, a sanitizing span processor that strips exception events and status messages before export, and a withActiveSpan cycle wrapper. The quoter bot starts it in index.ts, derives metrics from the shipped monitoring-record stream, wraps each setup/bootstrap/ladder cycle in a quoter-bot.cycle span, and gains a grafana/otel-lgtm compose profile. Telemetry is strictly opt-in, best-effort, and sanitized; TIB-2026-09-07 records the design. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@prd-carapulse[bot] can you review and approve if all good |
|
@codex review |
|
✅ Fresh verification passed: 7 test files, 35 tests. • Temporary probe absent. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![prd-carapulse[bot]](https://avatars.githubusercontent.com/in/2960675?s=60&v=4){kind=small}
There was a problem hiding this comment.
One blocking metrics-state bug:
bots/quoter-bot/src/infrastructure/observability/telemetry-metrics.utils.ts:210-216: after abook.observedtransition fromquotingtoempty, the absent rate fields are not recorded, so the cumulative synchronous gauges retain their old values. I reproducedbest_rate_bps=420remaining after the next export reportedquoting=0. This leaves stale rates visible as current. Please clear/remove unavailable rate series (for example with an observable-gauge state registry) and add a quoting→empty, two-collection regression test.
CI is green and the remaining reviewed telemetry/redaction paths look good.
Synchronous gauges re-export their last value forever, so an emptied book side kept showing its final rates beside quoting=0. Book rate gauges are now observable, mirroring a registry that each book.observed replaces with exactly the rates it carries, and the metric exporter selects delta temporality for observable gauges so the SDK stops carrying unreported series forward (OTLP gauge points carry no temporality field, so the wire format is unchanged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The blocking metrics-state bug is fixed in f891b50 (pushed just after your review of 7267073, alongside Devin's identical finding): book rate gauges are now observable, backed by a registry each @prd-carapulse[bot] can you review and approve if all good |
|
✅ Approved Stale-rate fix works. Regression passed locally: 2 test files, 8 tests. Telemetry/quoter-bot typechecks, lint, and formatting passed. GitHub Test check remained in progress; other checks passed. |
There was a problem hiding this comment.
Re-reviewed f891b50. The book-rate registry now replaces each market/side snapshot, observable gauges omit absent rates, and delta temporality prevents SDK carry-forward. The quoting→empty regression passes.
Local verification: 2 test files / 8 tests, telemetry and quoter-bot typechecks, lint, and formatting. No blocking findings.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72670739e0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| transactions.add( | ||
| 1, |
There was a problem hiding this comment.
When invalidate processes all active groups, OfferInvalidationService emits one offer-invalidation.transaction-submitted record per group even though invalidateBatch submitted a single transaction and all records share the same hash. This unconditional increment therefore reports an N-group batch as N submitted transactions, contradicting the instrument's transaction-count description. Deduplicate submitted records by transaction hash or rename/model the metric as per-group submission events.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2b65d33: submitted-phase increments deduplicate on a bounded memory of recent transaction hashes, so an N-group batched invalidation counts as one submitted transaction (test included).
🤖 Addressed by Claude Code
One-shot setup-check/bootstrap/ladder invocations now wrap their cycle in the same quoter-bot.cycle span as the monitors, and handled cycle failures (failed/halted results, not-ready reports) flip the span to an error status through a new withActiveSpan failed predicate. The diagnostic classifier only passes error-class-shaped tokens, the span sanitizer reduces any legacy http.url attribute to the redacted origin, cycle durations keep marketId, batched invalidation submissions count as one transaction, deploy:railway synchronizes the OTEL_* variables, and the env override's exporter contract is documented. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b65d33ebd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The cycle span now wraps the shared-queue enqueue, so mutation-queue contention under the combined start lifecycle is part of the trace (per-market work time stays in cycle.completed.durationMs). A throwing errorName projection or failed predicate can no longer replace the operation's own outcome, and the documented OTEL_METRIC_EXPORT_INTERVAL now flows through docker compose and the Railway variable sync. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ec03cc821
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
One-shot setup-check/bootstrap/ladder results now project through the shared monitoring records, so their cycle.completed and observation metrics match monitor mode; value-taking root options (--keystore, --private-key) no longer suppress the auto-verbose stream; trace and metric registration are isolated per signal so one failed exporter cannot discard the other; and metric export intervals beyond Node's timer maximum fall back to the default instead of hot-looping. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0fa5bdfbb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Adds the missing value-taking root option and a drift test that scans the CLI's declared options, so a future value-taking flag cannot fall out of QUOTER_BOT_ROOT_VALUE_OPTIONS silently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c6ee386bf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39ae9c5f2a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Some RPC providers encode keys as subdomain labels, so url.full and server.address (spans and the duration metric alike) now keep only the last two hostname labels with subdomains collapsed to [redacted]; IP literals and one- or two-label hosts pass through unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
All review rounds since your last pass are addressed on d7ff6f4 — the stale-gauge fix you flagged plus six rounds of codex findings (codex has now approved: "Didn't find any major issues"). @prd-carapulse[bot] can you review and approve if all good |
Shape validation was not a sanitization boundary: an adversarial fragment can look exactly like an error-class name. Only the enumerated classifications the SDK and undici actually raise pass through; every other diagnostic collapses to OtelDiagnostic, with adversarial regressions covering SecretError-shaped tokens. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Fixed in ef3ffed: @prd-carapulse[bot] can you review and approve if all good |
|
✅ Approved: PR #206 at • Review: fixed allowlist closes diagnostic-name disclosure path; adversarial regressions pass. |
There was a problem hiding this comment.
Re-reviewed ef3ffed. The fixed allowlist closes the diagnostic-name disclosure path: only enumerated classifications survive, while SecretError, TenantCredentialException, and credential-shaped tokens collapse to OtelDiagnostic. No blocking findings.
Local verification on Node 24.14.1: telemetry suite (7 files / 26 tests), @repo/telemetry typecheck, root lint, format check, and knip pass. GitHub Lint, Typecheck, and Dead-Code checks pass; Test is still running.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Nice work on the redaction design. Two thoughts before this lands:
|
|
Thanks — both points taken, and the probe result is genuinely useful: it falsifies the TIB's central bundling claim as written. Amendment drafted (staged locally, lands with the next signed push — 1Password is currently locked):
🤖 Addressed by Claude Code |
The bundle-cannot-patch claim was wrong: the createRequire banner keeps CJS require paths hookable, so the AWS SDK non-goal is a redaction deferral, not a bundle limit. The sdk-node rejection now rests on measured audit surface (21 pkgs/0.77MB vs 52/2.75MB + a protobufjs install script vs 117/4.2MB), and a diagnostics_channel-to-logs subscriber is recorded as the considered per-origin-latency alternative with the causality case only traces answer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Keeps both TIB index entries and maps the new guardrail.book-cleared / guardrail.book-crossed monitoring events into the guardrail metric counter, which the exhaustiveness check demanded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Merged main (0f5fb1d): one INDEX.md conflict resolved keeping both TIB entries, and the new @codex review @prd-carapulse[bot] can you review and approve if all good |
|
✅ Approved • Review: new guardrail events increment distinct counter series; merge integration and TIB amendment checked. |
There was a problem hiding this comment.
Re-reviewed 0f5fb1d, including the main merge and TIB amendment. No blocking findings in the reviewed diff.
- Direct collection probe confirms guardrail.book-cleared and guardrail.book-crossed pass the shipping allowlist and increment quoter_bot.guardrail.events with distinct type/workflow/marketId/side dimensions.
- Rechecked diagnostic allowlisting, URL/exception redaction, empty-book gauge removal, batched-submission deduplication, and cycle/CLI integration. Their scoped regression tests pass.
- Local verification on Node 24.14.1: 1,375 tests passed across 95 files in telemetry, observability, and quoter-bot, excluding fork e2e. Telemetry/observability/quoter-bot typechecks, root lint, format:check, knip, quoter-bot jsdoc:check, contracts build, and quoter-bot build passed. Built CLI --version returned 0.1.0.
- GitHub Lint, Typecheck, and Dead-Code passed; Test is still running. Approval is code-review approval, not a claim that CI or merge requirements are complete. Fork e2e was not run locally.
No repository source changes or merge performed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f5fb1d51a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The merged contract lists both booleans as safe grouping dimensions, and collapsing an actionable crossing, an unclearable condition, and a cooldown-held replacement into one series hid the distinction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9184d77e2f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The bootstrap/ladder one-shot factories run readiness preflight and removed-market cleanup before returning, so the span now wraps service construction too — a failed preflight produces an errored cycle span instead of orphan requests. Also documents two deliberate boundaries: the registrable domain stays visible in redacted hosts (public identity by threat model), and offer-invalidation submissions are submitted-only in the transactions metric because MonitoringWorkflow has no invalidation member for a contract-valid settled record. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eff91819d2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
One-shot record projections move to a new observeRecord runtime seam that reaches the observability sinks only, restoring the documented single-result stdout contract (--json setup-check emits one report again); monitor streams keep writing records through writeEvent. Monitor and start service construction — readiness preflight and removed-market cleanup — now runs inside a quoter-bot.startup span so preflight requests stop being orphan traces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46996cf339
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The stderr fallback logger is now built against an empty environment whenever telemetry is on and the monitoring logger is absent, so a partial Better Stack setup no longer silences otel.* lifecycle events (and still emits logship.misconfigured exactly once). Inside startBotTelemetry every lifecycle emission routes through a non-throwing logger shim, so a throwing injected logger can no longer break the never-throw startup or never-reject shutdown guarantees. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9942b854c3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
2 participants
Adds an opt-in OpenTelemetry stack to the quoter bot, per TIB-2026-09-07 — the follow-up TIB the kill-switch TIB deferred until a curator asked for OTel.
Why
The monitoring-record stream answers "what did the bot decide" but not "where did cycle time go":
cycle.completed.durationMsis one number per market per cycle, with no per-request view separating a slow RPC provider from API pagination or queue wait (the problem class behind #168). Better Stack log-metric expressions are also the only aggregation surface today.What
@repo/telemetrypackage (standalone per TIB-2026-08-04, not bot-kit growth):startBotTelemetryregisters OTLP/HTTP+JSON trace and metric export only whenOTEL_EXPORTER_OTLP_ENDPOINT(or a signal-specific variant) is set — otherwise nothing registers and no telemetry code runs. Undici instrumentation ridesdiagnostics_channel, the only auto-instrumentation that survives the production esbuild bundle;sdk-node/auto-instrumentations were rejected for exactly that reason (plus their gRPC/protobuf dependency tree understrictDepBuilds).exceptionevents and status messages before export (the undici error path recordserror.messageverbatim otherwise); SDK diagnostics reduce to a rate-limited classification token; the OTLP endpoint/headers are never logged. Wire-level tests assert the exported payload of both a successful and a failing request carries no path, query, or free-form error text.quoter_bot.*) are derived from the same shipped monitoring records the Better Stack path uses, so the two streams cannot disagree; attributes stay inside the monitoring contract's grouping dimensions (txHash/groupId/errorNamenever become metric attributes). Each setup/bootstrap/ladder cycle runs inside aquoter-bot.cyclespan so outbound request spans nest under it — three call-site wraps, no control-flow change. A configured OTel endpoint auto-enables the safe--verbosestream exactly like Better Stack config does.docker compose --profile otel upruns a pinnedgrafana/otel-lgtm(collector, Tempo, Prometheus, Loki, Grafana); README documents the operator surface, env vars, metric inventory, and limits (AWS SDK calls ridenode:http, so they appear inside cycle spans but not as child spans).Reviewer notes
withActiveSpanrethrows unchanged, no-op without an SDK).otel.*warnings; shutdown flush is bounded at 10 s), strictly sanitized.logship.misconfiguredunder partial Better Stack config (found by executing the built bundle).http.client.request.durationhistogram to a local OTLP capture server; the built CLI boots with telemetry enabled and exits cleanly.RPC_URL_8453) were not run locally; everything else passes (typecheck,lint,format,knip,jsdoc:check, 3053 tests).🤖 Generated with Claude Code