polygon/heimdall: collapse V1/V2 fetch scaffold into fetchVersioned generic#22167
Closed
yperbasis wants to merge 2 commits into
Closed
polygon/heimdall: collapse V1/V2 fetch scaffold into fetchVersioned generic#22167yperbasis wants to merge 2 commits into
yperbasis wants to merge 2 commits into
Conversation
Cover all nine versioned fetchers in both Heimdall API versions via the mocked HTTP request handler: URL paths and pagination queries, response transforms, count string parsing, fetch error wrapping, invalid milestone index mapping to ErrNotInMilestoneList, and recoverable-error retry behavior. Safety net for collapsing the repeated V1/V2 fetch scaffold.
…eneric Every versioned fetcher repeated the same scaffold: switch on c.Version(), fetch the V1 or V2 response type with retry, then apply the per-version transform. Introduce fetchVersioned/fetchVersionedEx generics that own that flow, and convert all nine fetchers to them. URL construction stays at call sites since paths and pagination queries differ per endpoint and version. fetchVersionedOpts carries the two per-site knobs: isRecoverableError (FetchMilestone's pruned-milestone retry decision) and wrapFetchErr, which wraps only fetch errors so FetchSpan's spanID context and FetchMilestone's ErrNotInMilestoneList mapping keep applying exactly where they did before, leaving transform errors untouched. V2 string counts keep strconv.Atoi semantics via parseCount. Both version URLs are now built eagerly; MakeURL can only fail on parsing the base URL string, which is version-independent, so the error surface is unchanged. A follow-up idea to rewrite poshttp.FetchWithRetryEx on top of cenkalti/backoff was considered and dropped: bridging client.closeCh into a context plus reproducing the select's priority between ctx.Err(), ErrShutdownDetected and the last attempt error (which can itself wrap context.Canceled) would need a per-call goroutine and post-hoc error disambiguation, and backoff's Notify hook never fires on the final attempt, changing the attempt-log cadence.
Sahil-4555
pushed a commit
to Sahil-4555/erigon
that referenced
this pull request
Jul 3, 2026
…ontech#22168) The five `GetOrCreate{Histogram,Counter,Gauge,SummaryExt,GaugeVec}` methods repeated the same ~32-line double-checked-locking block; each copy differed only in the constructor and type assertion. They collapse onto `getOrCreate[T prometheus.Metric]` / `getOrCreateVec[T prometheus.Collector]`, and each method is now a 3-liner. The GaugeVec copy is where clone mutation had gone wrong: its type guard compared `reflect.TypeFor[*prometheus.GaugeVec]()` to itself — always false, i.e. dead (and unreachable while `namedMetricVec.metric` was concretely typed `*prometheus.GaugeVec`). The generic widens the field to `prometheus.Collector` and performs a real assertion with the same error shape as the scalar family (`TestSetGetOrCreateGaugeVecWrongType`). `registerMetricVec` is removed: its re-check-and-insert was a duplicate of the inline double-checked insert. First commit adds characterization tests (+127 lines) pinning existing GetOrCreate* behavior — same-instance returns, wrong-type errors, describe-once semantics — green against the old code before the refactor. **Update (third commit):** `getOrCreate`/`getOrCreateVec` were themselves clones differing only in which registry (`m`/`a` vs `vecs`/`av`) they touched. They now delegate to a single `getOrCreateIn` over a shared `namedEntry[M]` entry type (`namedMetric`/`namedMetricVec` become type aliases), with the wrappers widening `T` through an adapter closure so the store stays compile-time safe. The double-checked locking becomes one critical section: the fast path took the same mutex anyway, and the create funcs are pure `parseMetric` + constructor calls, so creating under the lock eliminates the create-twice-drop-loser race. The unlock is deferred because prometheus constructors can panic on inputs reachable through the public API (a `quantile`/`le` const label, a negative summary window); a non-deferred unlock would strand the set's mutex. Also drops the dead scalar `namedMetric.isAux` — never assigned, so the `ListMetricNames` filter on it was unreachable. Net −169 lines in set.go. Verified: package tests incl. `-race`, `make lint` clean, full `go build ./...`. Part of a dedup series; siblings erigontech#22165, erigontech#22166, erigontech#22167.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every fetcher in
client_http.gorepeated the same scaffold:if c.Version() == HeimdallV2 { FetchWithRetry[V2] → transform } else { FetchWithRetry[V1] → &resp.Result }. This collapses the nine fetchers ontofetchVersioned[TV1, TV2, T](plus an opts variant carryingisRecoverableErrorfor FetchMilestone andwrapFetchErrfor FetchSpan'sspanID=%dwrapping, which applies to fetch errors but not transform errors — preserved exactly).URL construction (paths, pagination,
latest) stays at call sites; V1/V2 response structs untouched; V2 countstrconv.Atoisemantics kept.Before the refactor, two table-driven test functions pin current behavior (24 subtests: all 9 fetchers × V1/V2 routing/paths/pagination/transforms, spanID error wrapping,
ErrNotInMilestoneListmapping, and retry counts proving the recoverable-error threading) — green against the old code first, green after.Considered and rejected: converting
poshttp.FetchWithRetryExtocenkalti/backoff/v4— backoff'sNotifynever fires on the final attempt and bridging the client'scloseChwould need a per-call goroutine plus error disambiguation that re-implements the current wait; details in the refactor commit body.Net −41 lines in production code (~300 → ~200 at call sites) plus +429 of pinning tests. Verified:
go test ./polygon/..., scopedgolangci-lintclean (×2),make erigonandmake integration.Part of a dedup series; sibling fixes in #22165, #22166.