feat: add solver metrics - #108
Conversation
2ed273f to
86732ad
Compare
c03df44 to
6929732
Compare
ebf878c to
faf9b75
Compare
- add bounded workflow, RPC, txmanager, and sender-account metrics - retain authoritative snapshots and nonce/reorg lifecycle semantics - document metric migration and multi-lane deployment contracts Tests: GOTOOLCHAIN=go1.26.5 go build ./... Tests: GOTOOLCHAIN=go1.26.5 go test -race -cover ./... Tests: GOTOOLCHAIN=go1.26.5 golangci-lint run
38e9c93 to
d5f5385
Compare
Commit the six native Dashboard Schema v2 specs without deployment wrappers or provisioning tooling. Remove the obsolete migration guide for superseded metric names.
oxsteins
left a comment
oxsteins
left a comment
There was a problem hiding this comment.
Reviewed with a focus on behavior changes vs stage, metric metadata correctness, and dashboard-vs-code consistency. Build and the full test suite pass locally. The metrics layer itself is well designed: naming is coherent, label sets are bounded by construction, units/types/help text check out (one exception noted inline on oev_hotpath_seconds), and every metric name referenced by the six dashboards resolves to a registered family.
Inline comments below cover the substantive issues, nits marked as nits. A few points with no single line to anchor on:
- IncludedUnconfirmed handling: completing fills/redeems on OutcomeIncludedUnconfirmed instead of failing them is a behavior change vs stage in uniswapx, rfq, lifi and 3f. It checks out (the outcome only occurs at manager shutdown after a success-status receipt, so retrying in-process is moot), but it deserves a line in the PR description. Residual effect: a reorg during the shutdown window can overcount fill/redeem success metrics, and in uniswapx it also resets the fade breaker.
- solver_bot_txmanager_admission_wait_duration_seconds is the only registered metric no dashboard queries. Keep it, but give it a p95 panel next to the admission rejections panel; it is the leading indicator for nonce-lane saturation, rejections are the trailing one.
- All dashboard queries filter on kubernetes_pod and hardcode namespaces, pod-name regexes, and datasource uid "prometheus". Please double check against the cluster scrape relabeling (kube-prometheus default is pod, not kubernetes_pod); if the label name differs, every panel in all six dashboards renders empty. The dashboards also declare no uid, so provisioned URLs will not be stable across re-provisioning.
- nit: with the webhook strategy, uniswapx observeQuotedAmounts labels amount series with request-controlled tokenOut on the unauthenticated quote endpoint. The built-in strategy bounds tokenOut to inventory routes, so this is defense-in-depth only, but a permissive webhook would let requesters mint unbounded series.
| httpSrv := observability.NewHTTPServer(cfg.Observability.Addr, metrics, health) | ||
| metrics.SetSolvers(solverNames) | ||
| httpSrv := observability.NewHTTPServer(cfg.Observability.Addr, metrics) | ||
| go observability.ServeUntil(ctx, httpSrv, log) |
There was a problem hiding this comment.
ServeUntil runs on the root signal ctx, so at SIGTERM this server (serving both /metrics and /healthz) shuts down within its short grace while txmanager keeps draining on context.WithoutCancel for up to shutdownPreparationTimeout + pendingTimeoutMs + replacementIntervalMs plus its own drain. Every terminal outcome recorded during that window (confirmed/reverted fills, gas, fees, the primary funnel this PR adds) lands in a registry nothing can scrape and is lost at process exit, and a liveness probe on /healthz fails mid-drain, so the pod can be killed during the same-nonce cancellation lifecycle the shutdown budget exists to protect. Suggest keeping the server alive until after <-txDone, e.g. serve on a context cancelled only once the tx manager drain completes.
| // FilterAuthorizedRoutes filters by the adapter-wide marketMaker/owner/isFiller authorization and | ||
| // preserves every non-zero-adapter input route. It intentionally accepts adapter-only projections so | ||
| // startup validation does not depend on whether a solver has already resolved token-pair metadata. | ||
| func (r *Reader) FilterAuthorizedRoutes(ctx context.Context, routes []Route, filler common.Address) ([]Route, error) { |
There was a problem hiding this comment.
This drops the compactRoutes() call the base had here, which silently fixes a real stage bug (adapter-only routes from validateDirectAuthorization were all discarded by the zero token filter, flagging every configured adapter unauthorized) and changes startup authorization behavior for rfq, lifi and uniswapx direct mode. The new comment and tests cover it well, but please call it out in the PR description; a shared-code behavior change hiding in a metrics PR is easy to miss when someone bisects auth behavior later.
| return | ||
| } | ||
| outcome := completion.result.Outcome | ||
| if !outcome.Included() { |
There was a problem hiding this comment.
The base uniswapx_fills_total{outcome=filled|failed|not-admitted} classification is gone: FillWorkflowSpec only declares fill/success, so this branch and the NotAdmitted branch above emit no solver-level metric, and the old not-admitted counter assertion was deleted rather than migrated. txmanager_requests_total{label="uniswapx-fill"} covers tx-level failures, but sustained admission-control rejections and a failure denominator for the fill funnel are now invisible. Consider declaring failure/not_admitted outcomes on the fill event.
| }, | ||
| "spec": { | ||
| "editorMode": "code", | ||
| "expr": "(sum by (outcome) (((sum by (job, instance, outcome) (increase(solver_bot_txmanager_requests_total{namespace=~\"$namespace\",kubernetes_pod=~\"$pod_regex\",job=~\"${job:regex}\",instance=~\"${instance:regex}\",label=\"redeem\"}[$__range]))) > 0) or label_replace(increase(solver_bot_workflow_events_total{namespace=~\"$namespace\",kubernetes_pod=~\"$pod_regex\",job=~\"${job:regex}\",instance=~\"${instance:regex}\",solver=\"3f-bridge-facilitator\",event=\"redeem\",outcome=\"success\"}[$__range]), \"outcome\", \"confirmed\", \"job\", \".*\"))) > 0", |
There was a problem hiding this comment.
This expr double-counts. The left branch is aggregated to (job, instance, outcome) but the right branch keeps its full label set (solver, strategy, event, namespace, ...), so the or never deduplicates and the outer sum by (outcome) adds both whenever both have data. The units also differ: txmanager adds 1 per tx while the workflow event adds float64(count) finalized requests per tx, so the confirmed slice shows roughly tx count plus request count.
| } | ||
| s.requestStateRefresh() | ||
| s.releaseReservationByAuction(r.ID) | ||
| if reservation, released := s.releaseReservationByAuction(r.ID); released { |
There was a problem hiding this comment.
Settlement metrics only record when a live reservation is released, but pruneReservations frees by on-chain nonce on every state refresh, so a refresh that reads the post-settlement nonce before the liquidation-result frame arrives drops settled_success/settled_failed and the amounts silently (base counted every failed frame). The same gating applies to wins at line 84 (markReservationWon finds nothing after a restart or prune) and to the TTL prune anchored at r.at, which can classify a just-won reservation as unresolved and lose its later settlement. Each is individually deliberate, but together the enqueued/won/settled funnel undercounts during normal operation and disagrees with the breaker, which still counts the failure.
| quoteRefresh chan struct{} | ||
| discounts discounts.Provider | ||
| metrics *lifiMetrics | ||
| operations lifiOperationObservers |
There was a problem hiding this comment.
nit: operations mirrors metrics.operations and the two can diverge silently; quotes_test.go already builds a Solver with metrics set and operations zero, so its suspendQuotes records nothing and the omission is invisible. Reading through metrics (behind a nil-safe accessor, since a bare s.metrics.operations would panic on nil) would remove the second field.
| } | ||
| resp, err := s.quotes.quote(ctx, &in.Body) | ||
| decision, err := s.quotes.quote(ctx, &in.Body) | ||
| s.metrics.observeQuoteDecision(decision.outcome) |
There was a problem hiding this comment.
nit: the decision outcome is recorded before the errors.As split below, so client 400s (badRequestError) and dependency failures share outcome=error. A misbehaving client is indistinguishable from a chain-read outage in the metric even though this call site already distinguishes them for the HTTP status.
| } | ||
| body = b | ||
| } | ||
| method := boundedRPCMethod(body) |
There was a problem hiding this comment.
nit: this parses the request body even when metrics is nil (the plain Dial path discards the result), and nullResultFallbackRequest unmarshals the same body again, so every RPC pays two parses; with metrics on, each response is also copied (up to 64KB) and fully decoded a second time inside Close on the caller's path. Bounded but avoidable. Also, 3xx responses get labeled http_4xx and each redirect hop re-enters RoundTrip and counts as another request.
| failed-liquidations counters, a `skips_total{reason}` vector, a hot-path latency histogram, and deposit | ||
| gauges. The breaker halts bidding after N failed liquidations in a rolling window, | ||
| and immediately on a `blacklisted` frame. | ||
| - **Metrics** on the shared registry (`deps.Metrics.Registerer()`, nil-safe): bounded workflow events |
There was a problem hiding this comment.
nit: Registerer() is not nil-safe; it dereferences the receiver and panics on a nil *Metrics, which is why every factory hand-guards with if deps.Metrics != nil. Either make it return a no-op registerer on a nil receiver or fix the claim here.
| s.logSkip(a.ID, d) | ||
| return | ||
| } | ||
| if s.dryRun { |
There was a problem hiding this comment.
nit: dry-run would_bid no longer produces any bid event or amount series (base oev_bids_total explicitly counted would-bids), so validating strategy bid sizing in observe mode via the amount funnel is no longer possible and dashboards show a dead solver during a dry run.
Preserve terminal operation outcomes, expose dropped observations, and complete solver lifecycle coverage across integrations.\n\nHarden RPC and shutdown observability, make the Grafana dashboards portable, and document the revised metric contracts and behavior changes.
Keep the datasource selection portable while providing the typed empty current value required by Grafana's v2 variable union. Document how repository UIDs map to stable Dashboard resource metadata.
oxsteins
left a comment
oxsteins
left a comment
There was a problem hiding this comment.
Re-reviewed the two fix commits. This is a thorough pass: everything from the previous review is resolved except one partial, the fixes come with tests pinning the new behavior, and build plus the full test suite pass locally. Highlights that check out: the observability server now outlives the txmanager drain (WithoutCancel + defer ordering), the threef redeem panel no longer double-counts, the OEV bid lifecycle survives nonce prunes and restarts, unknown observations are visible via solver_bot_workflow_dropped_observations_total, the shared OperationTimer removes the seven hand-rolled timing blocks, and the dashboards dropped kubernetes_pod and hardcoded namespaces in favor of query variables with stable uids.
Partial: fill failure/not_admitted outcomes landed in the shared FillMetrics but only uniswapx records them. An rfq fill that reverts on-chain still emits no fill event (inline comment below), and an included-but-reverted receipt bypasses both the failure branch and the success gate in both solvers.
The inline comments below are about the new code the fixes introduced, nits marked as nits. The only one I would treat as blocking is the 3xx fallback regression in internal/chain/fallback.go. One body-level nit: the top-level "uid" key is not a Dashboard Schema v2 spec field, so strict provisioners may prune or reject it; the README convention covers manual imports, just something to keep in mind if tooling ever round-trips the raw files.
|
|
||
| resp, err := t.base.RoundTrip(attempt) | ||
| var inspectedOutcome *rpcOutcome | ||
| if err == nil && resp.StatusCode < 500 && resp.StatusCode != http.StatusTooManyRequests { |
There was a problem hiding this comment.
With redirects now disabled via ErrUseLastResponse, a 3xx lands in this success branch and is returned to the RPC client as-is, so a primary that starts issuing 301/307 (http to https upgrade, LB migration) makes every request fail with a decode error even while healthy fallback endpoints remain. On the previous head the same setup worked because http.Client followed the redirect. Classifying 3xx like 5xx/429 (close, record http_3xx, continue to the next endpoint) restores the resilience. TestDialDoesNotFollowRPCRedirects only covers a single-endpoint dial, so the missing-fallback case is untested.
| reservation = candidate | ||
| return true | ||
| }) | ||
| if lifecycleKey == "" { |
There was a problem hiding this comment.
When a liquidation-result frame has an empty id and empty tx hash, lifecycleKey is empty and this branch returns won:true, settled:true without touching the lifecycle map, so every replay of such a frame (WS reconnect resubscribe) increments bid{won} and bid{settled_*} again with wins that never matched a reservation. The breaker path next door deliberately fails closed on missing identity; fabricating a won transition per frame is new. Suggest not emitting won here, or counting it under a distinct outcome so replays are visible.
| } | ||
| return record | ||
| } | ||
| if len(s.bidLifecycleOrder) >= maxSeenAuctions { |
There was a problem hiding this comment.
The FIFO eviction at maxSeenAuctions evicts the oldest record regardless of whether that bid is still in flight. If 1024 lifecycle inserts happen between a bid's auction-result (won counted) and its liquidation-result, the record is evicted, settleReservationByAuction recreates it fresh and counts won a second time, with the settled amount decoupled from the original bid. Evicting only settled records first, or skipping records tied to live reservations, would keep the exactly-once guard under sustained auction bursts.
| anchor = r.wonAt | ||
| } | ||
| expired := now.Sub(anchor) > reservationTTL | ||
| if expired && r.won { |
There was a problem hiding this comment.
nit: a won reservation pruned at TTL is counted as unresolved here, but its lifecycle record survives, so a liquidation-result arriving later still emits settled_success/failed. The same bid then lands in two terminal counters and won != settled + unresolved. Probably acceptable (late settlement is the rarer, more useful signal), but worth a comment or help-text note so funnel reconciliation dashboards do not read the overlap as a bug.
| } | ||
| s.requestStateRefresh() | ||
| s.releaseReservationByAuction(r.ID) | ||
| lifecycleKey := strings.TrimSpace(r.ID) |
There was a problem hiding this comment.
nit: the lifecycle key is TrimSpace'd here but reserve() and markReservationWon key by the raw auction id, so an id with surrounding whitespace creates two records and won gets counted twice (once per key). Unlikely with well-formed feed ids, but trimming once at parse (or using the same normalization on all three paths) closes the asymmetry.
| e.log.Error(res.Err, "fill included but confirmation wait failed", | ||
| "orderId", orderID, "attempt", attempt, "tx", res.Hash.Hex()) | ||
| } | ||
| if e.metrics != nil { |
There was a problem hiding this comment.
FillMetrics.Observe returns early when the receipt status is not Successful, and rfq neither declares nor records the new FillOutcomeFailure that uniswapx got, so an rfq fill tx that reverts on-chain emits no fill event at all while the order is marked submitted. Dashboards built on the new fill outcomes will show uniswapx failures but stay blind to rfq fill reverts. Wiring the same ObserveOutcome(FillOutcomeFailure) here (and on the reverted-receipt case) would complete the coverage the fill-outcome extension started.
| s.metrics.addQuotedAmount(tokenOut, "output", amountOut) | ||
| } | ||
|
|
||
| func (s *Solver) quotedPairIsBounded(tokenIn, tokenOut common.Address) bool { |
There was a problem hiding this comment.
quotedPairIsBounded re-loads quoteState at observation time rather than the snapshot the quote was produced from. A concurrent invalidateQuotes/beginFillPlanning (any order claim, capacity change, or breaker) can nil the state between quote() returning and observeQuotedAmounts running, so a genuinely served quote skips amount recording while the quoted event still increments; amount totals undercount versus quoted counts exactly under fill load. The drop is fail-closed so cardinality stays bounded, but bounding against the snapshot used by quote(), or against configured routes, would remove the race.
| outcome = observability.ExternalOperationDegraded | ||
| } | ||
| s.log.V(1).Info("redeem scan", "adapter", target.Adapter.Hex(), "ready", len(ready)) | ||
| observability.ObserveOperation(ctx, s.operations.redeemableRefresh, outcome, scanDuration) |
There was a problem hiding this comment.
nit: with the ctx.Err() case now handled only inside ObserveOperation (which rescues Error, not Degraded), a shutdown that cancels mid-pass after one adapter read succeeded records outcome=degraded instead of skipped, so every deploy emits spurious degraded samples for the refresh operations (same pattern in reconcile and discoverAndOffer). Rescuing Degraded on ctx.Err() too, or checking ctx before classifying, would keep deploy noise out of degraded-ratio alerts.
| resp.Body = &cancelOnClose{ReadCloser: resp.Body, cancel: cancel} | ||
| } else if inspectedOutcome != nil { | ||
| outcome := *inspectedOutcome | ||
| resp.Body = newClassifiedRPCBody(resp.Body, cancel, func(closeErr error) { |
There was a problem hiding this comment.
nit: this closeErr re-classification is dead code: hasNullRPCResult replaced resp.Body with a NopCloser over the buffered bytes, whose Close always returns nil, so classifyRPCFailure(closeErr) can never run. Harmless, but it suggests read/close failures on the restored body are observable when they are not.
2 participants
Summary
Behavior changes versus
stageOutcomeIncludedUnconfirmedcomplete LI.FI, RFQ, UniswapX, and 3F fill/redeem work instead of scheduling an in-process retry. This outcome is emitted only during txmanager shutdown after a successful receipt was observed, so retrying in the terminating process is not useful. Residual caveat: a reorg after process exit can overcount fill/redeem success telemetry; for UniswapX it can also clear the local fill-failure breaker. These metrics are operational telemetry, not an accounting ledger.liquidlane.FilterAuthorizedRoutespreserves adapter-only route projections during direct-authorization startup checks. The prior compaction path discarded routes whose token fields were intentionally unresolved, incorrectly reporting every configured adapter as unauthorized for RFQ, LI.FI, and UniswapX direct mode.Verification
go build ./...go test -race -cover ./...golangci-lint run