fix(metrics): cap the distinct domains the exporter labels by - #10
Merged
Conversation
Prometheus keeps one time series per label value for as long as the process runs, and nothing evicted them, so labelling by request hostname grew the registry and the exported payload with the number of hosts a client asked for. - Past 100 distinct hosts, every further one is reported as "other". A deployer pointing Solverr at a handful of sites keeps full per-domain data and never reaches the cap. - A URL carrying no hostname is now "unknown" rather than labelling a series "None". - Only observable with PROMETHEUS_ENABLED=true, which is off by default. The resolver moved out of the plugin closure to module level, because nested it could hold no state across requests and could not be tested without constructing the plugin. Its two call sites already shared it, so the cap applies to both without them having to agree. Closes #6
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
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.
What changed and why
Both metrics are labelled by request hostname, and Prometheus keeps one time series per label value for as long as the process runs with nothing evicting them. Pointing Solverr at many hosts grew the registry and the exported payload with no limit, in a process that runs for weeks.
Past 100 distinct hosts, every further one is now reported as
other. A deployer pointing Solverr at a handful of sites keeps full per-domain data and never reaches the cap.Provenance
Filed by
/audit-scanon theresourcesdimension, and named as an unfixed inherited issue inHandoff.mdbefore that. The refutation attempt looked for a bound inconfig.pyandmetrics.py, for any pruning, and for a "Deliberately different" ledger entry covering metrics. There is none: it is inherited from upstream rather than chosen.Scope covered
All four sites the issue listed, found with
grep -rn 'labels(\|labelnames' src/ --include=*.py:src/metrics.py:9and:14— the twolabelnamesdeclarations, left as they are: the fix belongs at the value, not the schemasrc/bottle_plugins/prometheus_plugin.py:51and:60— both.labels(domain=...)call sites, now fed by a single bounded resolverThe issue offered a choice between patching both call sites or the shared resolver, and preferred the resolver because two call sites that must agree is how they stop agreeing. The resolver was nested inside the plugin closure, where it could hold no state across requests and could not be tested without constructing the plugin, so it moved to module level.
One thing not in the issue: a URL with no hostname produced
domain=None, which Prometheus renders as the string"None". It is nowunknown, the sentinel the surrounding code already used.Gate A, browser-free suite
169 tests, OK. 161 before, plus 8 covering a host under the cap, a host past it, a known host still reporting itself once the cap is reached, repeats not consuming the cap, both no-hostname cases, the sentinel not consuming a slot, and the test reset.
Gate B, live A/B in the same window
The baseline was built from a detached worktree at
origin/main, not from the main checkout, which has drifted.Mechanism. 105 distinct hostnames driven through each arm, against a cap of 100. Fast-failing DNS lookups rather than 105 solves, since a failed request still exports a domain label, which is the thing under test.
domainvaluesotherpresentThe change holds at the cap plus the two sentinels. The baseline grows one series per host, which is the defect.
No regression. Interleaved solve trials, 3 per arm, all
ok: change 13.6, 11.6, 11.5s against baseline 11.5, 11.6, 11.5s. Nothing here touches the solve path, so this is a no-regression check rather than a proof.Gate C, the consuming chain
byparr-proxyin front of/v1on a private network, which is the right chain for a controller-adjacent change.One search through a container with fresh label state: HTTP 200, 648832 bytes, 26 result rows, 11.5s, and the metrics endpoint carried exactly one label value, the real host, by name. That is the normal deployment shape.
A consequence worth seeing before merging
The cap is first-come-first-served and permanent. Running the same search on the container that had already seen 105 junk hosts, the real host was reported as
other, because all 100 slots were spent.In practice that means a deployment which sees 100 one-off hosts early and later settles on a few real ones would report those real ones as
otherfor the life of the process. There is no eviction, and no way to clear the set without a restart.This is the behavior the issue asked for, and I have not changed it. The issue also said an env allowlist should not be added speculatively, which I have respected. Flagging it because the decision reads differently once you have seen it happen, and an allowlist or an LRU is the obvious answer if you would rather not have that property.
What was not covered
byparr-proxywith the request an indexer produces.PROMETHEUS_ENABLED=true.Review notes
Self-reviewed rather than run through
/pr-review, per this session's standing instruction not to spawn subagents unasked.The pre-commit naming hook rejected the first CHANGELOG entry for the word "scrape" in "scrape payload", the Prometheus sense of the term. The hook is deliberately blunt and the CHANGELOG is a public surface, so I reworded rather than loosening the pattern.
Closes #6