Repository navigation
feat(agents): issue-59-add-observability-and-alerting-for-mctl - #61
Conversation
Add a Prometheus metrics layer to mctl-telegram covering HTTP requests,
auth failures, rate-limit events, MCP tool invocations and latency,
Telegram client pool size and errors, and session lifecycle counters.
- New internal/metrics package with Registry backed by a fresh (non-global)
prometheus.Registry; HTTPMiddleware records method/route/status_code using
chi route patterns to avoid high-cardinality raw-path labels.
- METRICS_ALLOW_CIDR env var gates /metrics to a CIDR range; when unset
the endpoint is open for Kubernetes PodMonitor scraping.
- auth.Middleware gains a *metrics.Registry parameter and classifies JWT
failure reasons (jwt_expired, jwt_invalid_signature, jwt_invalid_issuer,
jwt_missing_audience, jwt_wrong_audience, bearer_scheme_error, other).
- audit.RateLimiter gains WithMetrics chaining; increments
mctl_rate_limit_events_total{identity_kind} on HTTP 429.
- mcp.Server gains WithMetrics; s.audit() gains startedAt time.Time so
every tool handler observes mctl_tool_invocation_duration_seconds and
increments mctl_tool_invocations_total{tool, status}.
- telegram.ClientPool gains WithMetrics; increments/decrements
mctl_telegram_client_pool_size in acquire/run and increments
mctl_telegram_client_errors_total on non-context-canceled exit.
- db.Store gains WithMetrics; SaveSession increments
mctl_sessions_connected_total; RevokeActiveSession gains a reason
parameter ("disconnect", "idle_expiry", "absolute_expiry") and increments
mctl_sessions_revoked_total{reason}; HardDeleteAccount increments
mctl_sessions_revoked_total{reason="delete"}.
- SweepExpiredSessions split into SweepIdleSessions and
SweepAbsoluteSessions with per-reason metric increments; sweeper updated
to call both in sequence.
- CountActiveSessions DB query added; active session sampler goroutine in
main() refreshes mctl_sessions_active every 60s.
- All wiring added to cmd/server/main.go; /metrics route mounted.
- Tests added: metrics/metrics_test.go, metrics/middleware_test.go; all
existing tests updated for the RevokeActiveSession signature change.
Proposal: platform-gitops/agents-state/mctl-telegram/proposals/issue-59-add-observability-and-alerting-for-mctl/
|
Claude finished @mashkovd's task in 3m 54s —— View job PR Review — feat(agents): observability and alerting for mctl
2 P2 findings, 4 P3 findings. Changes requested before merge. Findings
What looks good
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return h.ServeHTTP | ||
| } | ||
| return func(w http.ResponseWriter, r *http.Request) { | ||
| host, _, splitErr := net.SplitHostPort(r.RemoteAddr) |
There was a problem hiding this comment.
P2 — CIDR guard bypassable via X-Forwarded-For spoofing
middleware.RealIP is applied earlier in the global middleware stack and rewrites r.RemoteAddr from the X-Forwarded-For / X-Real-IP headers. Any unauthenticated caller can send X-Forwarded-For: 10.0.0.1 and the CIDR check passes.
To guard against this, either:
- Mount
/metricson a separate internal listener that never runsRealIP, or - Record the raw TCP IP in a context value before
RealIPruns (first middleware) and read that value here instead ofr.RemoteAddr.
The current guard should not be relied on for access control. Fix this →
| } | ||
| rows, _ := res.RowsAffected() | ||
| if rows > 0 && s.metrics != nil { | ||
| s.metrics.SessionsRevokedTotal.WithLabelValues("delete").Add(float64(rows)) |
There was a problem hiding this comment.
P2 — HardDeleteAccount double-counts already-revoked sessions
HardDeleteAccount deletes all telegram_accounts rows for the user regardless of revoked_at. rows therefore includes rows that were already revoked before this call, and SessionsRevokedTotal{reason="delete"} will be inflated by those already-counted rows.
Fix: query only rows where revoked_at IS NULL to count what is actually being newly revoked, or add a WHERE revoked_at IS NULL sub-count before the delete. Fix this →
| // idle TTL (30 days). Returns the number of rows flipped. Increments | ||
| // mctl_sessions_revoked_total{reason="idle_expiry"} if a metrics registry is | ||
| // wired. | ||
| func (s *Store) SweepIdleSessions(ctx context.Context) (int64, error) { |
There was a problem hiding this comment.
P2 — No unit tests for the three new store methods
SweepIdleSessions, SweepAbsoluteSessions, and CountActiveSessions contain non-trivial SQL and time-arithmetic logic that can vary between SQLite (local-dev) and Postgres (production). The existing store_test.go file already has an in-process SQLite harness — these methods should have coverage there. Missing tests for at least:
SweepIdleSessionsrevokes rows past idle TTL and ignores others.SweepAbsoluteSessionsrevokes rows past absolute TTL.CountActiveSessionsexcludes revoked rows and rows older than one hour.
| return | ||
| case <-ticker.C: | ||
| n, err := store.CountActiveSessions(ctx) | ||
| if err == nil { |
There was a problem hiding this comment.
P3 — Errors from CountActiveSessions are silently dropped
When CountActiveSessions fails the gauge is simply not updated, but no log entry is written. A persistent DB error would leave the gauge stale with no observable signal. Add a slog.Warn or similar on err != nil. Fix this →
| err := s.DB.QueryRowContext(ctx, | ||
| `SELECT COUNT(*) FROM telegram_accounts | ||
| WHERE revoked_at IS NULL | ||
| AND (last_used_at IS NULL OR last_used_at > $1)`, |
There was a problem hiding this comment.
P3 — Metric help text contradicts query semantics for last_used_at IS NULL
The help string says "last_used_at is within the last hour", but the WHERE clause also matches rows where last_used_at IS NULL (sessions connected but never borrowed). Either:
- Update the help to say "non-revoked sessions active or connected within the last hour (last_used_at IS NULL counts as active)", or
- Remove
last_used_at IS NULLfrom the count if the intent is strictly "used within the last hour".
| @@ -31,6 +42,50 @@ func Middleware(p Provider, required bool) func(http.Handler) http.Handler { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
P3 — String-matching on error messages is fragile
classifyAuthError matches against literal substrings like "JWT expired" and "invalid JWT signature" that are sourced from other packages. If those messages are ever renamed the classification silently degrades to "other" with no compile-time or test-time signal. Consider instead exporting sentinel error values (e.g. var ErrJWTExpired = errors.New("JWT expired")) from the provider packages and using errors.Is here.
![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
mctl-telegram/internal/db/store.go
Lines 411 to 413 in 3e45548
Count only active rows as delete revocations
When a user deletes their account after previous disconnects or reconnects, HardDeleteAccount deletes historical rows that were already revoked, but this code adds every deleted row to mctl_sessions_revoked_total{reason="delete"}. That makes the revocation metric overcount deletes for users with session history; count only non-revoked rows before deleting, or use a separate deletion counter for all removed rows.
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| type responseWriter struct { | ||
| http.ResponseWriter | ||
| status int | ||
| wroteHeader bool | ||
| } |
There was a problem hiding this comment.
Because this middleware is installed globally, it also wraps the /mcp Streamable HTTP handler; this custom writer only exposes http.ResponseWriter, so downstream code that needs http.Flusher/http.ResponseController for text/event-stream no longer sees the server's streaming capabilities. In that MCP/SSE path the handler can fail to flush or reject streaming even though the underlying writer supports it, so the wrapper should forward optional interfaces (or use a wrapper that implements Unwrap/Flush) while capturing the status.
Useful? React with 👍 / 👎.
| host, _, splitErr := net.SplitHostPort(r.RemoteAddr) | ||
| if splitErr != nil { | ||
| host = r.RemoteAddr | ||
| } | ||
| ip := net.ParseIP(host) |
There was a problem hiding this comment.
In deployments that set METRICS_ALLOW_CIDR and also run the existing RealIP middleware, r.RemoteAddr may already have been replaced from client-controlled forwarding headers before this handler runs. A public caller can then spoof X-Forwarded-For/X-Real-IP to an address inside the allowed CIDR and scrape /metrics, so the allowlist should be based on the actual connection peer or run before trusting forwarded headers.
Useful? React with 👍 / 👎.
| startedAt := time.Now() | ||
| id := auth.From(ctx) | ||
| if id == nil { | ||
| return mcplib.NewToolResultError("authentication required"), nil |
There was a problem hiding this comment.
For prepare_send_message calls where the identity or required arguments are missing, these returns happen before the only metrics path (s.audit) is reached, so mctl_tool_invocations_total and the duration histogram omit those failed invocations entirely; the same early-return pattern exists in other tool handlers. If alerts rely on tool error rates, malformed or unauthenticated tool traffic will be invisible rather than counted with status="error".
Useful? React with 👍 / 👎.
| mux.Use(middleware.RealIP) | ||
| mux.Use(middleware.Recoverer) | ||
| mux.Use(middleware.Timeout(60 * time.Second)) | ||
| mux.Use(m.HTTPMiddleware()) |
There was a problem hiding this comment.
With the middleware order shown here, the metrics wrapper is inside middleware.Recoverer; when a downstream handler panics, control unwinds out of HTTPMiddleware before its post-next.ServeHTTP counter increment runs, and then Recoverer writes the 500 response outside the metrics wrapper. Those requests disappear from mctl_http_requests_total instead of being counted as 500s, which undermines alerting on server errors.
Useful? React with 👍 / 👎.
| if m != nil { | ||
| m.AuthFailuresTotal.WithLabelValues(classifyAuthError(err.Error()), providerLabel).Inc() | ||
| } |
There was a problem hiding this comment.
This increments mctl_auth_failures_total only when the provider returns an error, but on required routes a request with no Authorization header returns the 401 path below with no metric at all. In production MCP/account/bridge-token traffic, unauthenticated requests are still authentication failures for alerting purposes, so spikes of missing bearer tokens will be invisible unless this branch records a reason such as missing_credentials.
Useful? React with 👍 / 👎.
- middleware: the responseWriter wrapper now passes through http.Flusher and http.Hijacker. The middleware is mounted globally, so it also wraps the MCP endpoint (Streamable HTTP / SSE, needs Flush) and the /bridge websocket (needs Hijack for the upgrade) — masking those interfaces would break streaming and websocket upgrades. - metricsHandler: a misconfigured METRICS_ALLOW_CIDR now fails CLOSED (rejects every request) instead of silently serving /metrics openly. Documented that the CIDR guard is defense-in-depth over a NetworkPolicy since it matches the RealIP-rewritten, spoofable RemoteAddr. - mctl_sessions_active help text and CountActiveSessions doc corrected: the query also counts freshly created, never-used sessions. Noted the partial index that keeps the once-a-minute query cheap at scale.
|
Independent review done; findings addressed in e69a9ca:
|
|
Claude finished @mashkovd's task in 2m 53s —— View job Re-review — e69a9ca follow-up commit
Summarye69a9ca addressed 5 of the 8 original findings. 2 P2s and 1 P3 remain open.
Remaining — still requires changes before merge:
No new issues were introduced by e69a9ca. |
| } | ||
| rows, _ := res.RowsAffected() | ||
| if rows > 0 && s.metrics != nil { | ||
| s.metrics.SessionsRevokedTotal.WithLabelValues("delete").Add(float64(rows)) |
There was a problem hiding this comment.
P2 (carried over, not addressed) — HardDeleteAccount still inflates SessionsRevokedTotal{reason="delete"}
The DELETE FROM telegram_accounts WHERE user_id = $1 deletes every row for the user — active and already-revoked. RowsAffected() therefore counts rows that were already revoked before this call, so the metric double-counts historical revocations as new delete events.
Fix by counting only the active (not-yet-revoked) rows before the delete:
| s.metrics.SessionsRevokedTotal.WithLabelValues("delete").Add(float64(rows)) | |
| rows, _ := res.RowsAffected() | |
| if rows > 0 && s.metrics != nil { | |
| var active int64 | |
| _ = s.DB.QueryRowContext(ctx, | |
| `SELECT COUNT(*) FROM telegram_accounts WHERE user_id = $1 AND revoked_at IS NULL`, | |
| userID, | |
| ).Scan(&active) | |
| if active > 0 { | |
| s.metrics.SessionsRevokedTotal.WithLabelValues("delete").Add(float64(active)) | |
| } | |
| } |
Wait — that races with the DELETE already executed. The correct fix is to count before the delete, or add a WHERE revoked_at IS NULL sub-query to get the count in the same statement as the delete. A safe single-pass approach:
var active int64
_ = s.DB.QueryRowContext(ctx,
`SELECT COUNT(*) FROM telegram_accounts WHERE user_id = $1 AND revoked_at IS NULL`,
userID,
).Scan(&active)
res, err := s.DB.ExecContext(ctx, `DELETE FROM telegram_accounts WHERE user_id = $1`, userID)
...
if active > 0 && s.metrics != nil {
s.metrics.SessionsRevokedTotal.WithLabelValues("delete").Add(float64(active))
}
| n, err := store.CountActiveSessions(ctx) | ||
| if err == nil { | ||
| m.SessionsActiveGauge.Set(float64(n)) | ||
| } |
There was a problem hiding this comment.
P3 (carried over, not addressed) — CountActiveSessions error silently dropped
When the query fails the gauge is left stale with no observable signal. A persistent DB error would suppress all future updates with no log entry. Add a warn log in the error branch:
| } | |
| n, err := store.CountActiveSessions(ctx) | |
| if err == nil { | |
| m.SessionsActiveGauge.Set(float64(n)) | |
| } else { | |
| slog.Warn("active session count failed", "err", err) | |
| } |
| } | ||
|
|
||
| had, err := s.RevokeActiveSession(ctx, uid) | ||
| had, err := s.RevokeActiveSession(ctx, uid, "disconnect") |
There was a problem hiding this comment.
P2 (carried over, not addressed) — No unit tests for SweepIdleSessions, SweepAbsoluteSessions, or CountActiveSessions
These three methods contain non-trivial time-arithmetic SQL that is easy to get wrong (off-by-one on < vs <=, wrong cutoff direction, IS NULL edge cases). The existing in-process SQLite harness in newTestStore can exercise all three. At minimum:
SweepIdleSessions: seed one row withlast_used_atpast the idle TTL and one within it; assert only the expired row is revoked.SweepAbsoluteSessions: seed one row withexpires_atin the past and one in the future; assert only the expired row is revoked.CountActiveSessions: seed a revoked row, a never-used row (last_used_at IS NULL), and a stale row (last_used_atolder than 1h); assert the count is correct.
Without these, SQLite/Postgres time-function divergence or a future refactor can silently break the sweep logic.
…ty-and-alerting-for-mctl - [P1] metrics/middleware.go: add Unwrap() to responseWriter so http.ResponseController (Go 1.20+) can discover interfaces on the underlying writer through the wrapper layer. - [P1] cmd/server/main.go: store the real TCP socket peer address in context via http.Server.ConnContext and read it in metricsHandler for the CIDR allowlist check, preventing X-Forwarded-For header spoofing. - [P2] cmd/server/main.go: move m.HTTPMiddleware() before middleware.Recoverer so panics converted to HTTP 500 are recorded in mctl_http_requests_total. - [P2] mcp/tools.go: call s.audit() on id==nil and empty-argument validation failures in toolPrepareSendMessage so those paths are counted in mctl_tool_invocations_total. - [P2] auth/middleware.go: increment mctl_auth_failures_total with reason=no_token when id==nil and auth is required (missing bearer). Proposal: platform-gitops/agents-state/mctl-telegram/proposals/issue-59-add-observability-and-alerting-for-mctl/
|
Claude finished @mashkovd's task in 3m 30s —— View job Re-review — a6358d7 follow-up commit
a6358d7 addressed all Codex P1/P2 findings. 2 P2s and 1 P3 from prior reviews remain open. No new issues introduced. What a6358d7 fixed
Still requires changes before merge
|
| } | ||
| rows, _ := res.RowsAffected() | ||
| if rows > 0 && s.metrics != nil { | ||
| s.metrics.SessionsRevokedTotal.WithLabelValues("delete").Add(float64(rows)) |
There was a problem hiding this comment.
P2 (carried over, still not addressed) — HardDeleteAccount inflates SessionsRevokedTotal{reason="delete"}
DELETE FROM telegram_accounts WHERE user_id = $1 removes all rows for the user — active and already-revoked. RowsAffected() therefore includes rows whose revoked_at was already set, so this metric increment double-counts historical revocations as new delete events.
Fix: count only non-revoked rows before the delete, then use that count for the metric:
| s.metrics.SessionsRevokedTotal.WithLabelValues("delete").Add(float64(rows)) | |
| rows, _ := res.RowsAffected() | |
| if rows > 0 && s.metrics != nil { | |
| var active int64 | |
| _ = s.DB.QueryRowContext(ctx, | |
| `SELECT COUNT(*) FROM telegram_accounts WHERE user_id = $1 AND revoked_at IS NULL`, | |
| userID, | |
| ).Scan(&active) | |
| if active > 0 { | |
| s.metrics.SessionsRevokedTotal.WithLabelValues("delete").Add(float64(active)) | |
| } | |
| } |
Wait — that races with the already-executed DELETE. The safe approach is to run the COUNT before the DELETE:
var active int64
_ = s.DB.QueryRowContext(ctx,
`SELECT COUNT(*) FROM telegram_accounts WHERE user_id = $1 AND revoked_at IS NULL`,
userID,
).Scan(&active)
res, err := s.DB.ExecContext(ctx, `DELETE FROM telegram_accounts WHERE user_id = $1`, userID)
// ... error check ...
if active > 0 && s.metrics != nil {
s.metrics.SessionsRevokedTotal.WithLabelValues("delete").Add(float64(active))
}TestHardDeleteAccount_RemovesAllRows seeds one already-revoked row and one active row yet does not assert metric values, so this bug passes tests silently.
| } | ||
|
|
||
| had, err := s.RevokeActiveSession(ctx, uid) | ||
| had, err := s.RevokeActiveSession(ctx, uid, "disconnect") |
There was a problem hiding this comment.
P2 (carried over, still not addressed) — No unit tests for SweepIdleSessions, SweepAbsoluteSessions, or CountActiveSessions
All three methods contain non-trivial time-arithmetic SQL and remain without test coverage after three commits. The in-process SQLite harness (newTestStore) is already in place and can exercise them directly.
Minimum cases needed:
SweepIdleSessions: one row withlast_used_atpast the idle TTL, one within; assert only the expired row is revoked and the returned count is 1.SweepAbsoluteSessions: one row withexpires_atin the past, one in the future; assert only the expired row is revoked.CountActiveSessions: one revoked row, one never-used row (last_used_at IS NULL), one stale row (last_used_atolder than 1h); assert the returned count is correct.
These three SQL expressions are where SQLite/Postgres time-function divergence or a future refactor can silently break sweep logic — test coverage is load-bearing here.
| n, err := store.CountActiveSessions(ctx) | ||
| if err == nil { | ||
| m.SessionsActiveGauge.Set(float64(n)) | ||
| } |
There was a problem hiding this comment.
P3 (carried over, still not addressed) — CountActiveSessions error silently dropped
When the query fails, the gauge is left stale with no log entry and no observable signal. A persistent DB failure would suppress all future gauge updates silently.
| } | |
| n, err := store.CountActiveSessions(ctx) | |
| if err == nil { | |
| m.SessionsActiveGauge.Set(float64(n)) | |
| } else { | |
| slog.Warn("active session count failed", "err", err) | |
| } |
- HardDeleteAccount counts only rows that were still active at delete
time toward mctl_sessions_revoked_total{reason="delete"}. It previously
used RowsAffected(), inflating the counter with already-revoked rows.
The active-row count and the delete now run in one transaction.
- Add unit tests for SweepIdleSessions, SweepAbsoluteSessions and
CountActiveSessions (time-arithmetic paths were untested), plus a test
pinning the HardDeleteAccount delete-metric fix.
|
Claude finished @mashkovd's task in 2m 8s —— View job Re-review — d2b2327 follow-up commit
d2b2327 fixed both outstanding P2s. 1 P3 remains. What d2b2327 fixed
Remaining — 1 P3
What looks good in d2b2327
No P1 or P2 findings. The single P3 does not block merge but should be fixed in the same pass. |
| n, err := store.CountActiveSessions(ctx) | ||
| if err == nil { | ||
| m.SessionsActiveGauge.Set(float64(n)) | ||
| } |
There was a problem hiding this comment.
P3 (carried over, still not addressed) — CountActiveSessions error silently dropped
When the query fails the gauge is left stale with no log entry. Add one warn line:
| } | |
| n, err := store.CountActiveSessions(ctx) | |
| if err == nil { | |
| m.SessionsActiveGauge.Set(float64(n)) | |
| } else { | |
| slog.Warn("active session count failed", "err", err) | |
| } |
…rting-for-mctl Resolve conflicts from PR #60 (unauthorized-session detection + tool-call logging) against the observability layer: - internal/mcp/tools.go audit(): keep both the metrics block (#61) and the slog mirror block (#60); the get_unread_messages bridge path keeps the RedactPeer wrapper (#60) and gains startedAt (#61). - RevokeActiveSession now takes a reason — pass it at the three #60 call sites (store.go partial-session and clientpool revokeRejected -> "unauthorized"; revoke_telegram_session tool -> "disconnect"). - The two #60 admin tools (revoke_telegram_session, get_user_audit_log) capture startedAt and pass it to audit() for the duration metric. Also fix the remaining P3: log a slog.Warn when CountActiveSessions fails so a stale mctl_sessions_active gauge leaves a trail.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @mashkovd's task in 3m 2s —— View job Re-review — e723eb2 merge commit
No P1 or P2 findings. All previously open findings are addressed. Good to merge. What e723eb2 fixed
Merge conflict resolution (correct)The merge integrated PR #60 (unauthorized-session detection + tool-call logging) against the observability layer cleanly:
Full finding status across all review rounds
No new issues introduced by the merge. |
Implements accepted proposal
mctl-telegram/issue-59-add-observability-and-alerting-for-mctl.Spec: https://github.com/mctlhq/mctl-gitops/tree/main/platform-gitops/agents-state/mctl-telegram/proposals/issue-59-add-observability-and-alerting-for-mctl/
Generated by mctl-agents Tier 2 implementer. Review carefully — this PR was opened automatically and the spec may have gaps.
Closes #59