merge upstream changes - #1
Open
CoolDuke wants to merge 494 commits into
Open
Conversation
At this level we have no way to know if those selectors will be on the same node (combining them into a single downstream query -- e.g. aggregation imposes that limitation). So instead we simply disallow NodeReplace for anything with multiple selectors as children. Fixes #456
This is causing some weird problems; we'll take the perf hit to be correct for now
panic) Related to #420
Signed-off-by: Morten Mjelva <morten.mjelva@cognite.com>
Bumps [github.com/prometheus/exporter-toolkit](https://github.com/prometheus/exporter-toolkit) from 0.6.1 to 0.7.3. - [Release notes](https://github.com/prometheus/exporter-toolkit/releases) - [Changelog](https://github.com/prometheus/exporter-toolkit/blob/master/CHANGELOG.md) - [Commits](prometheus/exporter-toolkit@v0.6.1...v0.7.3) --- updated-dependencies: - dependency-name: github.com/prometheus/exporter-toolkit dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
promxy built web.Options without setting NotificationsGetter/NotificationsSub,
leaving them nil. The notifications handlers call them unconditionally, so any
request to /api/v1/notifications or its SSE variant /api/v1/notifications/live
panicked with an invalid memory address / nil pointer dereference.
Getting these endpoints actually working took three parts:
- Construct a notifications.Notifications manager (the same type upstream
Prometheus uses) and wire its Get/Sub methods into web.Options
(maxSubscribers=16, matching upstream's default). This alone stops the panic.
- Forward Flush() from ApacheLogRecord to the underlying ResponseWriter. The
logging wrapper only implemented Write/WriteHeader, so the SSE handler's
http.Flusher assertion failed and returned 500 "Streaming unsupported".
- Populate the stream with the notification events that are meaningful for a
proxy: ConfigurationUnsuccessful (added on config-reload failure, cleared on
the next successful reload; threaded through reloadConfig so it covers both
SIGHUP and the HTTP reload path) and ShuttingDown (added at the start of
graceful shutdown). StartingUp/WAL-replay is omitted -- promxy has no local
TSDB WAL to replay.
Verified end-to-end against a live SSE subscriber: both endpoints return 200
(text/event-stream for the SSE variant), a bad-config reload emits
{ConfigurationUnsuccessful, active:true}, recovery emits active:false, and
SIGTERM emits {ShuttingDown, active:true}.
Fixes #795
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds a test exercising promxy's server-side remote-read endpoint (/api/v1/read): a remote-read client queries promxy, which fans out to two downstream Prometheus server_groups and returns the merged raw samples. The topology mirrors production — two v1 API downstreams, promxy serving the vendored remote.NewReadHandler backed by ProxyStorage, and a client speaking the remote-read wire protocol. Each group carries a distinct az label so a correct fan-out yields two series. Requests the SAMPLES response type explicitly: ProxyStorage does not implement ChunkQuerier, so the STREAMED_XOR_CHUNKS path is unsupported today. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Pv6 dialing
Downstream hosts that resolve to both IPv4 and IPv6 could become unreachable
when one address family (typically IPv6 in Kubernetes) is blackholed. Go's
net.Dialer implements RFC 6555 "Happy Eyeballs" dual-stack fallback, but the
fallback only begins after FallbackDelay (default 300ms) -- longer than
promxy's default 200ms dial_timeout, so the dial times out before IPv4 is ever
attempted.
Expose two knobs on http_client:
- dial_network (tcp/tcp4/tcp6): hard-pin an address family; "tcp4" forces IPv4.
- fallback_delay: maps to net.Dialer.FallbackDelay so the fallback can fire
within the dial timeout (or be disabled with a negative value).
Defaults are unchanged: empty dial_network behaves as "tcp" and a zero
fallback_delay uses Go's default, so existing configs dial exactly as before.
Fixes #713
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Promxy hardcodes the alert GeneratorURL to a link to its own graph page for the alert expression, matching upstream Prometheus. When promxy is the central evaluation point for many backends and operators triage alerts elsewhere (Grafana alerting, an incident manager, a per-tenant dashboard), that URL is not useful -- and points at the wrong backend for series that originated in e.g. VictoriaMetrics. Add an opt-in pkg/alerttemplate that renders the GeneratorURL from Go templates configured under promxy.alert_templates: a default template, reusable named templates, and label-matched rules (first match wins). Templates are compiled once on config (re)load behind a RWMutex and selected per-alert in sendAlerts; with no configuration promxy falls back to the exact built-in URL, so default behavior is unchanged. Reimplementation of the approach in #737, rebased onto the post-Prometheus-3.5 master and scoped down (no template directory / .tmpl loader, no CLI flags -- inline named templates cover the use cases in #736). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`http_headers` is supported on each `alerting.alertmanagers` entry via the inlined Prometheus http_client_config, letting promxy inject custom headers on every request to an Alertmanager (e.g. a gateway that authenticates via a signature header). Add a commented example to the sample config. Closes #693 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A `label_filter` is the only per-target wrapper that makes a network call
at construction time (the initial `LabelValues` sync). When a target was
unreachable at startup that error escaped `loadTargetGroupMap`, which spun
its retry loop forever so the servergroup never became Ready and promxy
never started -- regardless of `ignore_error` (which only wraps the query
path, applied later).
Add an `on_sync_error` option to `LabelFilterConfig`, keeping the failure
mode on the feature it governs rather than overloading `ignore_error`:
abort - fail the initial sync, blocking startup until it succeeds
(default; preserves historical behavior)
open - proceed unfiltered, sending all queries downstream until synced
closed - proceed but filter out everything (skip the target) until synced
For open/closed the client no longer fails construction on a failed initial
sync and instead retries in the background until the first success. `closed`
fails safe (an unloaded filter returns nothing rather than leaking the whole
downstream), which is what the issue reporter wanted for authz/routing.
Fixes #732
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…NKS (#356) A stock Prometheus remote_read client negotiates STREAMED_XOR_CHUNKS first (remote.AcceptedResponseTypes), which routes promxy's /api/v1/read handler to ProxyStorage.ChunkQuerier -- previously a stub returning errors.New("not implemented"), so every default remote_read against promxy got HTTP 500. Promxy has no native chunk source, so ProxyChunkQuerier reuses the existing sample-based ProxyQuerier.Select and re-encodes the result into chunks via storage.NewSeriesSetToChunkSet. The streaming API requires series sorted by label set, so the wrapper sorts (ProxyQuerier.Select does not). Adds TestRemoteReadFromPromxyStreamedChunks, which drives the high-level remote.Client (real chunked negotiation/decoding) and asserts the same fan-out the SAMPLES test checks. Shared topology setup is factored into startRemoteReadPromxy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `build` workflow spent ~10m almost entirely in buildx. The Dockerfile cross-compiles (`$BUILDPLATFORM`, `CGO_ENABLED=0`), so the four arches aren't QEMU-emulated — they're four native cross-compiles. But every push did `COPY . …` → full `go build` of promxy + all of vendor, once per platform, from a cold Go build cache, with no layer cache between runs. - Dockerfile: add per-arch `--mount=type=cache,target=/root/.cache/go-build` mounts so the Go build cache is reused across the four platform builds. - build.yml: drive the build via docker/build-push-action@v6 with cache-from/to type=gha so that cache and the image layers also persist across runs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
setup-go's built-in cache keys only on go.sum, so the entry is written once
and then frozen ("cache hit occurred on the primary key, not saving cache").
As a result GOCACHE never rolls forward: every PR recompiles the packages
that changed since that snapshot and re-runs all go-test results from
scratch. The heaviest package, the integration suite in test/ (~112s), is
fully go-test cacheable but never benefits.
Manage the caches explicitly instead:
- Module cache (~/go/pkg/mod): stable key on go.sum.
- Build + test-result cache (~/.cache/go-build): master runs write a fresh
entry each run (github.run_id makes the key unique); PRs restore the most
recent entry via restore-keys but never save. GitHub cache scoping lets a
PR read its base branch's caches, so every PR builds/tests off master's
warm base and only recompiles / re-runs what it actually changed.
PRs stay read-only (no extra runner, no per-PR cache churn); master, which
runs regardless, does the writing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The integration suite in test/ spun up a fresh promxy per subtest and each one blocked ~5s in proxyStorage.Ready(). Root cause: a server group only becomes Ready after its first target sync, and Prometheus's discovery manager emits that first sync on the first tick of its update interval — which defaults to 5s. The actual query evaluation in each subtest is sub-second, so ~20 subtests × ~5s of pure startup wait was the whole run. Expose that interval as servergroup.DiscoveryUpdateInterval (defaulting to the current 5s, so production behavior is unchanged) and set it to 10ms in the test package's init. Measured on the full test/ package with -race: ~112s -> ~23s, still passing. The remaining ~23s is genuine serial evaluation work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.51.0 to 0.52.0. - [Commits](golang/crypto@v0.51.0...v0.52.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-version: 0.52.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.79.3 to 1.83.1. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.79.3...v1.83.1) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-version: 1.83.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
…arallel Update the prometheus fork to pick up jacksontj/prometheus#5. parser.Walk's goroutine fan-out was gated on "no NodeReplacer installed", which had both halves backwards: - We always install a NodeReplacer (cmd/promxy/main.go), and every downstream fetch happens under it -- ProxyStorage.NodeReplacer runs the query for each pushed-down subtree, and populateSeries calls Select per selector. So the walk that does the I/O was fully sequential: `sum(a) + sum(b) + sum(c) + sum(d)` against a 100ms stub backend made 4 calls with peak concurrency 1. - The walks that did fan out were the read-only ones, which do no I/O. Their visitors keep unsynchronised per-walk state, and upstream's rules.buildDependencyMap writes a plain map from its visitor -- so a config reload could die with "fatal error: concurrent map writes". The fork swaps the gating: parallel with a NodeReplacer, sequential without. The same stub now shows peak concurrency 4. It also carries the lint, formatting and license-header fixes that had left the fork's own CI red before any of this. Two regression tests: - test/rule_group_test.go replays the reload path (the manager's RuleDependencyController over a group of interdependent rules). It reports the reported race under -race against the old fork. - pkg/proxystorage/parallel_fetch_test.go asserts sibling subtrees of a query are fetched concurrently, so the fan-out can't silently regress again. Fixes #809 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P7WBcFL81Qi8n9eg7DNDXm
Two changes that have to land together, because the second exposes a
latent ordering assumption in the first.
Exemplar dedup
--------------
MultiAPI.QueryExemplars grouped results by series fingerprint and then
concatenated the exemplar lists. Inside a server group the fan-out is
across HA replicas of the same Prometheus, which observed the same
exemplars, so every exemplar came back once per replica. Every other
fan-out path applies anti-affinity or set semantics; this one did not.
proxyExemplarQuerier.Select one layer up had the same shape, so nothing
downstream collapsed them either.
Dedupe on (timestamp, value, labels) -- the identity Prometheus itself
uses -- not on timestamp alone, since a series legitimately carries
several exemplars at one timestamp with different trace IDs. The
first-result path also aliased the downstream's exemplar slice; it now
builds its own.
Fan out in completion order
---------------------------
All six fan-out methods allocated one channel per API and received from
them in index order, under a comment reading "wait for results as we get
them". Total latency was unaffected, but the early abort was: each
method can return as soon as enough replicas of one HA bucket have
failed that the result cannot be assembled, and that check could not run
for a fast-failing target at index 5 until slow targets 0-4 returned. A
doomed query stayed open with its sibling requests still running.
Use a single channel buffered to len(apis) and receive in completion
order. Three methods still need index ordering for their *merge*, so
they park results in per-API slots and merge after the loop:
- Series and Metadata are first-writer-wins, so arrival order leaked
into which downstream won.
- scatterMerge's output is label-sorted, but mergeAntiAffinity
resolves duplicate samples in the order sets are handed to it, so
arrival order could change which replica's samples are returned.
LabelValues and LabelNames sort at the end and are order-independent.
The exemplar sort was keyed on timestamp alone, which was deterministic
only because results used to arrive in index order; it is now a total
order over the same fields the dedupe key uses.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
Truncation in the time filters was open-coded in each method, and two of
those copies had drifted.
RelativeTimeFilter's Series, GetValue and QueryExemplars compared the
request end against the filter end with Before where the other three
methods use After. The inverted test fires when the request end is
already inside the group's window and pushes it *forward* to the window
edge, so the filter widens the request instead of narrowing it. Against
a group configured `end: -12h` with `truncate: true`, a request for the
hour ending 24h ago went downstream asking for everything up to 12h ago:
requested [2026-09-01T20:53, 2026-09-01T21:53]
downstream [2026-09-01T20:53, 2026-09-02T09:53] # +12h
GetValue is the raw-sample path, so a `foo[1h]` selector fetched
thirteen hours of samples instead of one, from every target in the
group. Series is also wrong rather than merely wasteful: it reports
series that exist only in the extra window.
AbsoluteTimeFilter.LabelNames dropped the IsZero guards that every other
method has. With only `start` configured -- the natural way to say "this
group has nothing before date X" -- tf.End is the zero time.Time, every
real end is after it, and the end was rewritten to 0001-01-01. The group
then returned no label names for any query, surfacing as silently
missing values in autocomplete and label_values() rather than an error.
Collapse all ten range-taking call sites onto a single truncateWindow
helper so the clamp exists once. QueryRange keeps its own block: it also
has to hold the result on the requested step grid.
The new tests exercise every range-taking method rather than one, so a
regression in a single method can't hide behind the others. Verified
against the unfixed code: the widening test fails for exactly Series,
GetValue and QueryExemplars, and the unset-edge test fails for exactly
LabelNames.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
dynamicAntiAffinity exists to compute one number -- half the median
inter-sample gap. To get there it allocated two full []model.Time
slices, one per side, purely to strip timestamps out of []model.SamplePair,
then a third for the gaps, then sort.Slice'd the whole thing to read out
a single median. histogramTimestamps did the same copy for the histogram
path.
This runs per series, per pairwise merge, on every HA merge when
anti_affinity_dynamic is on -- which the config documents as the right
setting for any group with mixed scrape intervals.
Read timestamps through a small accessor so []model.SamplePair and
[]model.SampleHistogramPair both feed the estimator with no copy. A type
parameter rather than an interface, so the slice is not boxed and the
accessor does not allocate. The gaps slice, pre-sized, is the only
allocation left.
before 11086 ns/op 24632 B/op 5 allocs/op
after 4347 ns/op 8192 B/op 1 allocs/op
(1024 samples per side.)
Behaviour is unchanged: positive deltas only, anchored on a with b
borrowed only when a yields fewer than 3 gaps (and a's gaps kept, not
discarded), median at gaps[len/2], returning median/2. Tests check
against the pre-rewrite implementation as an oracle over 18 shaped cases
plus 2000 seeded random pairs, and pin the allocation count so the
copies cannot come back.
slices.Sort replaces sort.Slice rather than a partial selection: it
needs no reflect-based swapper (which is what made the old call
allocate), and on gap data -- which is nearly all identical values --
pdqsort measured faster than a hand-rolled quickselect, so the simpler
code is also the quicker one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
…stogram check
fillStaleNaNGaps exists because an isolated sample from a sparse
pushdown result bleeds forward through the engine's 5m lookback -- its
own doc comment names present_over_time returning 1 at a single step.
But the defensive allocation bound gated the fill on the result already
being dense:
if expected > 0 && expected <= int64(len(present))+10_000 { ... }
An 11,000-step range query -- Grafana's default ceiling -- returning 500
samples gives 11000 > 500+10000, so the fill was skipped and the bleed
came back. The sparser the result, the more likely the guard tripped.
The bound was there for allocation safety, and it is no longer needed:
the fill is now a linear merge that walks the step grid and the
(already ordered) samples together, pre-sizing its output. The step
count comes from promxy's own EvalStmt, not from downstream data, so it
is already bounded by the query.
That also removes the quadratic sort. The markers used to be appended
after the real samples and insertion-sorted, and since both halves are
already ordered the cost was the inversion count between them:
steps before after speedup
1,000 590,262 38,360 15x
5,000 8,741,491 158,438 55x
11,000 41,466,865 321,175 129x ns/op per series
41ms per series at 11k steps; a present_over_time query returning 100
series spent ~4s of request time in that one function.
Second change, same area: in the Call branch the result was materialized
twice -- once by containsLossyHistogram, purely to answer a boolean, and
again by the fill. Fold the histogram detection into the fill's walk and
drop the intermediate per-series copy. The instant-query path (no fill)
still uses containsLossyHistogram, as do the four other pushdown
branches. Per pushed-down call, versus the original two-pass shape:
-51% time, -76% bytes, -48% allocations
The returned set is still a fresh, unconsumed copy -- the detection
drains the source cursor, so the data has to be rebuilt for
UnexpandedSeriesSet either way.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
The filter is map[labelName]map[labelValue]struct{} -- already indexed
both ways. FilterLabelMatchers ignored that twice: it ranged the outer
map to find one key, then ranged every value calling matcher.Matches(v),
even for an equality matcher that could be a single map hit.
Cost per matcher, per target, per query, all before any I/O, against a
__name__ filter of 100k names:
before after
equal, hit 405.6 us 13.5 ns
equal, miss 561.0 us 10.8 ns
regexp, miss 888.7 us 53.3 ns
The miss column is the case label_filter exists to serve -- deciding not
to send a query for a metric this group does not have. Three matchers
across ten targets was ~17ms of CPU to make that decision.
Index the outer map, answer MatchEqual with one lookup, and take the
SetMatches() fast path for regexps whose pattern is literal
alternatives (upstream populates it only for the case-sensitive literal
path, where Matches(s) is exactly slices.Contains(setMatches, s), so
this is semantics-preserving). MatchNotEqual and MatchNotRegexp reduce
to counting how much of the set is excluded. Genuine regexps still scan
and are unchanged; the benchmark keeps that case so the remaining worst
path stays visible.
The value maps are shared across goroutines through the atomic, so all
of this is strictly read-only.
Tests cross-check every matcher type against a reference implementation
that just calls matcher.Matches over the whole set -- 468 cases across
13 filters -- so the fast paths cannot drift from the semantics they
replace.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
…ections
HTTPClientConfig inlines prometheus' config_util.HTTPClientConfig, which
already declares `enable_http2`. So the key was already accepted in
promxy's config -- parsed, then silently ignored, because promxy builds
its own transport instead of calling config_util.NewRoundTripperFromConfig.
Wire it up rather than adding a second knob for the same thing.
It matters because the transport sets both TLSClientConfig and
DialContext (the latter is needed for dial_network / dial_timeout /
fallback_delay), and net/http skips its automatic HTTP/2 wiring whenever
either is set unless ForceAttemptHTTP2 is explicitly true:
case !t.ForceAttemptHTTP2 && (t.TLSClientConfig != nil || t.Dial != nil ||
t.DialContext != nil || t.hasCustomTLSDialer()):
// Be conservative and don't automatically enable http2 ... Issue 14275.
So every server group ran HTTP/1.1 regardless of scheme, holding up to
max_idle_conns_per_host sockets per target instead of multiplexing.
Left off by default rather than flipped on, because h2 is not
unambiguously better for this traffic shape: promxy issues a modest
number of large sequential response bodies per target, where h2's
per-stream flow control can be slower than h1 with a warm pool, and it
puts every concurrent query for a host behind one TCP connection. Note
this diverges from prometheus' own DefaultHTTPClientConfig, which sets
EnableHTTP2: true -- promxy never seeds that default onto this struct,
so the effective default here is false and current behaviour is
preserved. The doc comment covers the trade-off and the fact that
max_idle_conns_per_host means something quite different under h2.
Transport construction moves into newDownstreamTransport so the result
is testable before the auth round-trippers wrap it.
This is promxy as an HTTP *client*; it is unrelated to the inbound h2
behaviour under investigation in #781.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
BREAKING: the exported field ServerGroup.Cfg is replaced by the method ServerGroup.Config(). Readers must call Config() and nil-check the result; writers must go through ApplyConfig. ApplyConfig assigned s.Cfg and s.client with no synchronization, while both were read by RoundTrip on every downstream request and by the Sync goroutine -- which NewServerGroup starts *before* ApplyConfig ever runs. loadTargetGroupMap alone reads ~25 fields off s.Cfg. There is no happens-before edge between the write and those reads. The struct carried a `TODO: lock/atomics on cfg and client` acknowledging it. This is a latent hazard rather than a live race today: ProxyStorage. ApplyConfig is the only non-test caller and it constructs brand-new ServerGroups on every reload, cancelling the old ones, so no in-use group is reconfigured in place. Nothing enforces that invariant, and the exported mutable field invites breaking it. Hold both as atomic.Pointer and read through accessors. Readers take a single snapshot per operation rather than re-loading per field -- loadTargetGroupMap in particular could otherwise straddle two configs mid-loop. ApplyConfig publishes the client before the config, so a reader that sees the new config sees the client built from it; a side effect is that a transport construction failure no longer leaves a group with a new config and a stale client. RoundTrip now returns an error instead of nil-panicking if called before any config is applied. httpClient() stays unexported -- nothing outside the package needs it, so the exported surface does not grow beyond Config(). The concurrency test drives 200 ApplyConfig calls against 4 goroutines hammering RoundTrip plus the group's own Sync loop. Reverting the fields to plain struct members makes it fail under -race with two reports, one on each read path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
…it a failure ServeHTTP discarded every WriteFloatSample return and only checked the final Flush. When a scraping client hangs up mid-body -- scrape_timeout, cancellation -- the writes started failing immediately but promxy kept formatting and attempting to write the entire remaining result set, noticing only at the end. On a large federation payload that is a lot of wasted CPU per aborted scrape, and the log line said nothing about how far it got. Give the encoder a sticky error (Err()) rather than an error return on WriteFloatSample: the write path performs ~15 individual bufio writes per sample, so a per-call return would either check each one or probe at the end, and probing at the end *is* Err(). It also keeps the hot loop's allocation-free structure intact -- BenchmarkEncoderLean is still 3 allocs/op for 5000 samples. Cost in the loop is one deref and nil check per sample. Classify the failure before logging it. A scrape cancelled by its client is normal operation, not a promxy failure: context.Canceled, EPIPE, ECONNRESET, net.ErrClosed, and http2 stream/connection errors now log at debug with a samples-written/total count, while genuine encoding failures stay at error. This is what is behind the "federation failed: http2: stream closed" noise in #781. That text comes from net/http's *bundled* h2 server, whose sentinels are unexported and only reachable by message, hence the string check alongside the typed ones. It does not fix whatever is cancelling those scrapes -- it stops promxy reporting the client's disconnect as its own error, and gives the sample counts needed to tell the two apart. golang.org/x/net moves from indirect to direct in go.mod; it was already vendored and marked explicit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
… deterministically Two independent fixes to knobs that did not do what they said. remote_read ignored the server group's timeout. The remote.ClientConfig hardcoded a 2m deadline while the HTTP path used cfg.Timeout, so a group tuned to fail fast on the JSON API would still hang for two minutes on remote_read -- the path carrying the largest payloads, and so the one where a stuck request is most expensive. Note the two timeouts do not mean the same thing, and this is a behavior change for anyone who already sets both remote_read and a short timeout. promxy's cfg.Timeout is wired to Transport.ResponseHeaderTimeout, which bounds only the wait for response headers; remote.ClientConfig.Timeout becomes a context deadline over the entire read, body included. Reusing the knob is therefore a strictly tighter bound on remote_read than on the HTTP path. That seems clearly better than ignoring it -- two minutes was a leftover default, not a considered value -- but a deployment with a short timeout and large remote_read payloads may need to raise it. Unset still means 2m, so the default is unchanged. MetadataHandler trimmed to `limit` by ranging the map and deleting past a counter. Go randomizes map iteration order, so two identical requests against an unchanged downstream returned different subsets. The endpoint guarantees no particular ordering (Prometheus is non-deterministic here too), but it should at least be reproducible, so the survivors are now the lowest metric names by byte order. A negative limit previously deleted everything by accident; it is now clamped to zero, which does the same thing on purpose and without an out-of-range slice. Both fixes are extracted into helpers (newRemoteReadConfig, trimMetadataToLimit) so they can be tested without standing up a server group or an HTTP handler, following the newDownstreamTransport pattern. Note this does not make the metadata result fully deterministic: promxy also forwards the raw limit downstream, so each target truncates its own response arbitrarily before the union is trimmed here. Making the whole result well-defined would mean dropping the downstream limit and paying for full metadata dumps from every target, which is a separate call given how large those dumps get on a fleet with many metric names. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
… helper The step-invariant optimization only ever applied to one of the seven pushdown sites. queryRangeAt recognizes a subtree pinned to a single timestamp by @ -- the result is then identical at every step -- and issues one instant query at the @ timestamp, replicating the vector across the step grid. It was wired into the *parser.Call branch and nowhere else. The other six sites (the SUM/MIN/MAX/TOPK/BOTTOMK/GROUP, COUNT and COUNT_VALUES arms of AggregateExpr, VectorSelector, and both BinaryExpr helpers) each inlined the same eight-line QueryRange/Query block verbatim. So `sum(foo @ 1234)` over a 3,000-step range made the downstream evaluate the same instant 3,000 times and ship 3,000 identical samples per series, where one would do. The new tests measure this on a 6-step range: seven of the eight shapes issued a range query returning 6 samples per series before, and issue a single instant query after. Collapsing the seven copies also closes the gap where those sites bypassed the pre-epoch hasNegativeFractionalSecond guard that the helper's instant path provides. The helper absorbs the instant-request case (s.Interval == 0) that each site used to handle in its own else branch, so every site is now a single call, and it is renamed queryDownstream since it is no longer only about range queries. While collapsing them: four sites checked a `var err error` that nothing ever assigned to (the three AggregateExpr arms and Call), so the check was dead and the real error on result.Err() went unchecked at the point of the request. This was not a lost error -- an errored SeriesSet stashed into UnexpandedSeriesSet still surfaces when the engine expands it, and TestPushdownSurfacesDownstreamError passes both before and after -- but the checks read as error handling while doing nothing, and the other four sites already used result.Err(). They now all do, and the two unused declarations are gone. TestStepAlign_E2E_AtModifier changes with this. An @-pinned subtree now takes the instant path, which StepAlignClient does not re-stamp (it wraps QueryRange only; an instant query has no grid to snap), so the pinned value is the backend's sample at exactly the @ timestamp instead of the grid-snapped one. That is the better answer, and the test now asserts the value rather than only step-invariance. Its stub gained a Query method: it previously modeled only QueryRange, so it returned no data on the path the @ case now takes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
The native_histogram metadata cache called Metadata on the server group's merged client, which fans out to every target: each refresh pulled a complete /api/v1/metadata body -- every metric name, help string and unit -- from every replica, only to keep the histogram-typed names. A server group is a set of HA replicas of each other (the assumption anti-affinity merging is already built on), so one replica's metadata answers for the whole group. MultiAPI gains MetadataOnePerKey, which queries a single API per Key() fingerprint -- the same grouping requiredCount is enforced over -- rotating the starting member between calls and failing over to the remaining members of a group when the chosen one errors. The cache uses that instead of the fan-out; a group whose targets are all down still logs and keeps the previous snapshot, which is right for a name-keyed cache whose contents change on deploys. Measured by the new test (5k metric names, 20 targets): 973KB per refresh, down from 19.5MB. Note that ErrorWrap -- the outermost per-target wrapper a server group builds -- does not forward Key(), so in practice every target of a group lands in one fingerprint bucket and a refresh is exactly one request. The per-key grouping is what keeps the method correct for callers whose members are not all interchangeable. No metric filter or limit helps here: /api/v1/metadata cannot filter by type, and `limit` truncates the name space arbitrarily, which would silently drop histogram names from the cache. MultiAPI.Metadata has no dedup to lean on -- it fans out and merges first-writer-wins. The new path records downstream latency under call="metadata" in server_group_request_duration_seconds, rather than the "query" label the existing (copy-pasted) fan-out path uses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
ProxyQuerier.Select carried a `// TODO: switch based on sortSeries bool(first
arg)` and returned whatever order the fan-out produced. Callers that pass
sortSeries=true feed several Select results into storage.NewMergeSeriesSet --
/federate (pkg/federate) and the vendored /api/v1/series handler both do, one
Select per match[] -- and that k-way merge assumes each input is ordered by
labels.Compare. Given an out-of-order input it does not error; it emits
duplicate series.
Nothing below Select supplied that order. MergeSeriesSets sorts its inputs only
when it has more than one set to merge, so a single server group with a single
target returns the set straight from the JSON decode. And the downstream's
ordering is only meaningful in terms of the labels the downstream sent: promxy
rewrites those afterwards. AddLabelClient sets the server-group labels with
labels.Builder.Set, which overwrites, so a server group whose labels collide
with a downstream label reorders a perfectly sorted result. With server-group
`labels: {job: sg}` and downstream series
up{job="a",pod="x"} up{job="b"} (sorted: first < second)
the client stack yields
up{job="sg",pod="x"} up{job="sg"} (sorted: second < first)
Reproduced end-to-end against a real Prometheus API server (the promqltest-backed
promclient.CreateTestServer) driving the federate handler with two match[]
selectors; before the fix the response contained `up{job="sg",instance=""}`
twice, which a scraping Prometheus rejects as a duplicate sample for the
timestamp. metric_relabel_configs reaches the same state by a second route,
since relabel.Process can rename labels arbitrarily.
Select now sorts when the caller asks and passes the set through untouched when
it does not. The promql engine's path (Select(ctx, false, ...), engine.go:1025)
is unchanged and still streams -- a test pins that Select does not advance the
underlying set when sortSeries is unset. MergeSeriesSets keeps its
len(sets) == 1 fast path for the same reason: sorting there would materialize
every hot-path query.
The metadata branch (hints.Func == "series") is sorted too; MultiAPI.Series
concatenates the per-target label sets, so it arrives in target order.
promclient.sortedSeriesSet is exported as SortSeriesSet for reuse, and
ProxyChunkQuerier.Select drops its own copy of the same sort in favor of asking
ProxyQuerier for one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
MergeValues has had no non-test caller since the storage.SeriesSet refactor: instant and range results are converted with ModelValueToSeriesSet and merged by promclient.MergeSeriesSets, which folds through MergeSampleStream directly. Only tests reached MergeValues, so it was carrying its own dead scalar/string/vector merge policy plus an aliasing bug -- the model.Vector branch stored the caller's *model.Sample in the result and then mutated its Value in place, corrupting the input matrix of whichever downstream happened to be merged second. Removing the function removes the bug with it. MergeSampleStream, the part that is still live, keeps every test it had. The two test suites that drove it through MergeValues now drive it through a local mergeMatrix helper that groups streams by fingerprint and folds collisions through MergeSampleStream -- exactly what the deleted Matrix branch did. That includes the differential oracle in promclient (TestMergeSeriesSetsMatchesMergeValues, renamed TestMergeSeriesSetsMatchesMatrixMerge), which is unchanged in substance: both sides of that comparison always shared MergeSampleStream, so what it pins is the SeriesSet plumbing -- conversion, grouping, iteration -- not the merge primitive. What is retired is the table coverage of the nil/scalar/string/vector branches of MergeValues. Those exercised merge policy promxy no longer has anywhere: nothing merges model.Value any more, and instant-vector results are merged as single-sample series through the SeriesSet path. This is a breaking change for anyone importing promxy as a library and calling promhttputil.MergeValues. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
…window
The dynamic label_filter sync asks the downstream for label values over the
full time domain:
c.LabelValues(ctx, label, nil, model.Time(0).Time(), model.Now().Time())
No matchers, epoch to now -- once per dynamic label, per target, per
sync_interval. An unbounded start forces the downstream to consult every block
on disk; for the documented `__name__` case that means enumerating every metric
name the downstream has ever held, repeatedly and forever. A servergroup with N
targets, L dynamic labels and a 5m sync_interval asks that question N*L times
every 5 minutes.
Add a `sync_lookback` knob that bounds the start to `now - sync_lookback`,
which lets an operator scope the sync to the recent blocks.
The default is unset/0, which keeps today's unbounded behavior exactly.
Narrowing the window by default is not safe: promxy has no way to know the
downstream's retention, and a label value whose samples all fall outside the
window disappears from the filter -- which makes promxy skip a servergroup that
should have been queried. That is a silent wrong-results failure, and it is
strictly worse than the sync cost it would save. So the tradeoff is documented
on the config field (and in the example config) and left to the operator.
Also hoist the `now` used for the sync out of the per-label loop, so every
label in one sync shares a single time range.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
…ovides
I went looking at this to "fix" ErrorWrap not forwarding Key(), on the
theory that it makes MultiAPI's per-key grouping inert. It does make it
inert, but that is what makes HA work, and forwarding Key() would be a
serious regression rather than a fix.
MultiAPI enforces requiredCount per fingerprint bucket, and a target's
bucket comes from Key(). A server group wraps each target in ErrorWrap
before calling NewMultiAPI with requiredCount=1. ErrorWrap does not
implement APILabels, so the type assertion in NewMultiAPI fails, every
target gets the zero fingerprint, and they all land in one bucket --
"any one replica answering is enough", which is the whole point of a
server group.
Give ErrorWrap a Key() that forwards to the wrapped client and targets
split by their discovered labels (AddLabelClient's key is the target's
non-reserved labels merged with the group's, so it differs per target
whenever SD or relabeling attaches any). One bucket per target, times
requiredCount=1 per bucket, means every replica must answer -- one down
replica fails every query. Measured, not reasoned: with the forwarding
added, the new ErrorWrap subtest fails with "a down replica failed the
query: error in target: replica down".
So this changes no behavior. It documents the invariant on the type and
adds two guards so the next person to notice the inertness finds out why
before shipping it:
- TestMultiAPIToleratesDownReplica, whose "targets behind ErrorWrap"
case builds the arrangement a server group actually builds and
asserts a down replica is survivable. It also asserts the distinct-key
case removes fault tolerance, so the consequence is stated rather than
implied.
- TestErrorWrapDoesNotExposeKey, which fails on the specific edit with
a message pointing at the doc comment.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
Follows the MergeValues removal: the same model.Value-era helper, dead for the same reason. Nothing has called it since labels were moved onto the per-target AddLabelClient wrapper, which builds a new labels.Labels rather than mutating a decoded value in place. It had no callers at all -- not even tests -- so unlike MergeValues there is no oracle to preserve and nothing to move. It carried the same aliasing hazard MergeValues did, and would have been a live bug had anything called it again: the model.Vector branch writes straight into `item.Metric`, and a Vector's elements are *model.Sample, so it mutates the caller's samples. It also returned an error that was unconditionally nil. Removing an exported symbol, hence the `!`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
decodeVector left `sample` nil when a result entry carried neither a
"value" nor a "histogram" key, and appended []chunks.Sample{nil} anyway.
Nothing failed at decode time: DecodeSeriesSet returned a SeriesSet with
a nil Err() and a series whose single sample was nil, and the nil
dereference only fired later, when something iterated it -- inside the
PromQL engine, several layers from the response that caused it. The
engine's recover() turns that into an opaque "unexpected error" 500 plus
a stack-trace log rather than a decode error naming the bad field.
A `null` array entry hit the same path: jsoniter's ReadObject consumes
the null and returns "" immediately, so no key is ever seen.
Report the error through iter.ReportError, as the other decode failures
in this file do, and return no series so a nil sample can never escape
the decoder even if a future caller ignores iter.Error. The whole
response is rejected rather than silently dropping the malformed entry:
a truncated vector is indistinguishable from a real result, and promxy
would merge the partial answer with other server groups' data.
This deliberately diverges from prometheus/common, whose
model.Sample.UnmarshalJSON accepts the same body and yields a sample of
value 0 at timestamp 0 -- fabricated data is a worse outcome than a
surfaced error for a body no conformant API should send.
decodeMatrix is unaffected: an entry with neither "values" nor
"histograms" produces a sample-less series, never a nil sample, which
matches what model.SampleStream decoding gives for that body. The scalar
path always constructs a floatSample and already reports malformed input
through the iterator. Both are pinned by tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TCScW9ZoyzjdxKaKYQNQY4
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
20 participants
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.
No description provided.