Make operator metrics MongoDB read preference and concern configurable - #2510
Make operator metrics MongoDB read preference and concern configurable#2510thjaeckle wants to merge 2 commits into
Conversation
The operator metrics (both the count based "custom-metrics" and the "custom-aggregation-metrics") so far used the same MongoDB readPreference and readConcern as all normal searches (query.persistence). This is unwanted e.g. when searches are configured with a costly readConcern like "linearizable" for strong consistency, while the periodic operator metric queries would rather run cheaply (readConcern "local") and/or offloaded to a secondary node. Add two optional, independent persistence configs under operator-metrics, each falling back to the general query.persistence config when absent (keeping existing behavior unchanged): - custom-metrics-persistence for the count based "custom-metrics" - custom-aggregation-metrics-persistence for the "custom-aggregation-metrics" Aggregation metrics use a dedicated persistence instance, so MongoThingsAggregation- Persistence just resolves the dedicated config (else query.persistence). The count metrics share the SearchActor / MongoThingsSearchPersistence path with user counts, so SudoCountThings carries optional readPreference/readConcern overrides which are applied per query in MongoThingsSearchPersistence.sudoCount(...). Addresses the readConcern part of eclipse-ditto#2163; the per-metric "index-hint" already allows differing hints per aggregation metric, so the shared namespace-hint fallback is left untouched here. Signed-off-by: Thomas Jäckle <thomas.jaeckle@beyonnex.io> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SearchActor now invokes the 5-arg sudoCount(query, headers, indexHint, readPreference, readConcern) overload, so waitForQueries must stub that overload on the ThingsSearchPersistence mock instead of the 3-arg one - otherwise the sudo count returns an unstubbed (null) source and the graceful-shutdown assertions fail. Signed-off-by: Thomas Jäckle <thomas.jaeckle@beyonnex.io> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hu-ahmed
left a comment
There was a problem hiding this comment.
Tightly-scoped change and the aggregation half is exactly right — MongoThingsAggregationPersistence is used only by OperatorAggregateMetricsProviderActor, so binding the read settings once at construction can't leak into user-facing search. The new SudoCountThings fields being optional and additive also makes this safe across a rolling upgrade. I verified locally that both touched modules compile clean and the new tests pass (SudoCountThingsTest 12/12, DefaultOperatorMetricsConfigTest 2/2). The equals/hashCode fix in DefaultOperatorMetricsConfig is a nice drive-by — both metric maps were previously ignored.
One substantive issue, and it's about how the "optional" part behaves.
The config block is all-or-nothing, but it's documented as if it were per-key
DefaultOperatorMetricsConfig (line 76) decides via hasPath at the block level. Once the block exists, DefaultSearchPersistenceConfig fills any key you left out from its own ConfigValue defaults — primaryPreferred / default — and not from query.persistence. So a half-filled block doesn't inherit the missing half; it silently replaces it.
I ran this against the branch's config classes, simulating a deployment with ditto.mongodb.options.readPreference = secondaryPreferred and readConcern = majority:
A) no block configured -> inherits query.persistence (correct)
B) block sets readPreference only -> readConcern silently becomes "default"
C) block sets readConcern only -> readPreference silently becomes "primaryPreferred"
D) block sets both -> exactly as configured (correct)
Case C is the one that bites: an operator asking only for the cheap read concern gets the periodic metric queries pulled back onto the primary — the opposite of this PR's purpose — with no warning and no log line.
I originally suggested per-key inheritance, but I'm happy to take the whole-block reading instead: if you configure this block, you configure all of it. That's a perfectly coherent design. It just needs three things to actually hold, because right now nothing enforces it and the docs say the opposite:
1. The Helm chart currently generates half-filled blocks itself. In search-extension.conf.tpl (line 151) readPreference is the gate and readConcern is conditional, so the chart's minimum-viable path renders exactly the block the design forbids:
$ helm template t . --set ...customAggregationMetricsPersistence.readPreference=nearest
custom-aggregation-metrics-persistence {
readPreference = "nearest" # no readConcern -> silently "default"
}
Worth dropping the {{- if .readConcern }} and giving readConcern a value in values.yaml, so a rendered block is never partial. Related: readConcern can't currently be set on its own at all through the chart, which is awkward given "searches stay linearizable, metrics run local" is the motivating example in the PR description.
2. Nothing enforces completeness in code. If the block is meant as a full replacement, I'd require both keys when it's present and fail with a DittoConfigError otherwise. At minimum, log the effective values at startup — there's precedent, MongoThingsSearchPersistence already logs Query readConcern=<...> readPreference=<...> on construction, and the metrics path deserves the same visibility.
3. The wording says the opposite of the behaviour. values.yaml (line 1732) and search.conf (line 406) both say "When unset (the default), the general query.persistence settings are used", which reads per-key. It should say: when the block is omitted entirely you inherit query.persistence; if you configure it, set both keys, because an omitted key falls back to primaryPreferred / default.
This third point is the one I'd want fixed whichever design wins — docs promising per-key inheritance over code doing block-level replacement is what turns a design choice into a trap.
What
Makes the MongoDB
readPreferenceandreadConcernused by the operator metrics configurable independently from the read settings used for normal searches (query.persistence).Two optional, independent config blocks are added under
ditto.things-search.operator-metrics:custom-metrics-persistencecustom-metricscustom-aggregation-metrics-persistence$groupcustom-aggregation-metricsBoth are optional and fall back to the general
query.persistenceconfig when absent, so existing deployments are unaffected.Why
Closes #2163.
The operator (aggregation) metrics so far inherited the global search
readConcern/readPreference. That is unwanted when searches are configured with an expensivereadConcern(e.g.linearizable) for strong consistency: the periodic operator metric queries would rather run cheaply (readConcern = local) and/or be offloaded to a secondary node (readPreference = secondaryPreferred) without weakening the user-facing search guarantees.How
MongoThingsAggregationPersistence, so it resolves the dedicatedcustom-aggregation-metrics-persistenceconfig (elsequery.persistence) and binds read preference + read concern at construction.SearchActor→MongoThingsSearchPersistence.count/sudoCountpath with user-facing counts, soSudoCountThingsgains optionalreadPreference/readConcernfields (@since 3.9.7, mirroring the existingindexHint), applied per-query viacollection.withReadPreference(...).withReadConcern(...). Unknown values are logged and ignored, not fatal.4.6.1.Out of scope
The
hintsub-point from #2163 is intentionally left out: aggregation metrics already support a per-metricindex-hint, so hints can already differ per metric. Decoupling the shared namespace-hint fallback can be tracked separately.