Per upstream server connection metrics via hidden and derived metrics - #13506
Open
cmcfarlen wants to merge 10 commits into
Open
Per upstream server connection metrics via hidden and derived metrics#13506cmcfarlen wants to merge 10 commits into
cmcfarlen wants to merge 10 commits into
Conversation
The tests asserted absolute metric ids and iterator positions, which only hold when the case runs first against an otherwise empty store. Any other test case that creates a published metric makes them fail. Assert on relative state instead so the cases can run in any order.
Hidden metrics are stored but never published. Using a separate Storage instance rather than a per-metric flag makes them structurally unreachable from the published store, so no metric consumer can expose them by omission. Gauge and Counter each gain createHiddenPtr overloads which return the same correctly typed pointer as createPtr, so a hidden metric is read and written with the normal typed mutators and no cast is needed at the call site.
Storage::create() had no exhaustion check, so filling the last blob let the following bookkeeping call addBlob() and write one past the end of _blobs. Refuse the final slot instead and return the reserved bad_id, which keeps addBlob() from ever being reached in a full store and costs one slot out of 8M. The guard in addBlob() was also off by one against the access it protects, since the write is to _blobs[++_cur_blob], and being a debug_assert it was compiled out of release builds entirely. Make it a release_assert against MAX_BLOBS - 1.
Hidden metrics are invisible to normal queries by design, which makes them hard to debug. Add an opt-in rec type bit, deliberately outside RECT_ALL so hidden metrics are never returned unless explicitly requested. The rec type also has to be accepted by the JSONRPC request decoder, which validates each requested type against a whitelist and rejects the whole request otherwise. No wire or schema change is needed, as rec_types is already an untyped list of ints.
Derived metrics could only sum their sources. Add an op to the spec so a derived metric can also take the max or min across its sources, which is what an aggregate over instantaneous gauges needs. The accumulator is seeded from the first source rather than from zero, since a zero seed is only correct for SUM and would clamp MIN to <= 0. op defaults to SUM, so existing specs are unaffected.
A source given by name or id that does not resolve was still passed to lookup(), which masks the unresolved id down to the reserved bad_id slot. The aggregate then silently included that slot's value instead of skipping the source, with no error reported. Resolve each source first and skip it if it does not resolve. This is observable under MAX and MIN, where the bad_id value can become the winning one; under SUM it happened to be hidden by bad_id holding zero.
derive() only accepts a fixed initializer_list, which does not work for aggregates whose sources are discovered as the process runs. Calling it repeatedly for one derived name does not help either: it appends a separate entry per call, all targeting the same metric, so each update overwrites the others with its own subset and the last writer silently wins. add_source() accumulates sources into a single entry instead. Registering a source that is already present is a no-op, so a caller that may re-register the same source need not track that itself.
Add a developer guide page for the metrics registry covering the hidden store, how it differs from the published one and why it is a separate store rather than a flag, and the derived metric aggregation ops including when derived values are recomputed and what that means for a sampled maximum.
Two problems with the per group peak, which is reported as the "max" field of the connection tracker group dump. update_max_count() made a single compare_exchange_weak attempt with no retry, so a racing update, or a spurious failure of the weak form, silently discarded the sample. Retry until the value is stored or is no longer the largest. It was also only called when a maximum was configured. With metrics enabled and no configured maximum, the count was reserved and then discarded, so the peak stayed at zero. Pass the reserved count through in that case too.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends ts::Metrics with a hidden metrics store and richer derived-metric aggregation, then uses those facilities to publish per-upstream-server connection metrics (with configurable publication levels) while shifting aggregation work onto the periodic stats sync task rather than the connection hot path.
Changes:
- Add a hidden metrics store (
Metrics::hidden_instance()), plus derived-metric enhancements (MAX/MINops and incrementalDerived::add_source()). - Implement per-upstream-server connection metrics based on hidden per-group metrics and derived per-hostname aggregates; update
traffic_ctl metric matchto optionally include hidden metrics. - Expand unit/integration tests and documentation to cover hidden/derived metrics and the new per-server connection metric behavior.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gold_tests/origin_connection/per_server_connection_max.test.py | Extends gold test coverage for per-server connection metrics levels, hidden visibility, and multi-group aggregates. |
| tests/gold_tests/jsonrpc/metric_match_include_hidden.test.py | Adds an end-to-end JSONRPC test ensuring --include-hidden is accepted and still returns published metrics. |
| src/tsutil/unit_tests/test_Metrics.cc | Expands unit tests for derived ops, add_source behavior, hidden store semantics, and blob boundary handling. |
| src/tsutil/Metrics.cc | Implements hidden store instance, store exhaustion guard, derived ops (SUM/MAX/MIN), and incremental source registration. |
| include/tsutil/Metrics.h | Adds public APIs for hidden metrics and derived-metric ops/add_source; exposes hidden metric creation helpers. |
| src/iocore/net/ConnectionTracker.cc | Moves per-group connection metrics into hidden store, registers derived per-host aggregates, and (level 2) mirrors group metrics into published store. |
| include/iocore/net/ConnectionTracker.h | Introduces MetricLevel, documents publication levels, adds hostname aggregate naming helper, and fixes max-count update loop. |
| src/proxy/http/HttpSM.cc | Ensures peak count is updated even when metrics are enabled without a configured maximum. |
| src/traffic_ctl/traffic_ctl.cc | Adds traffic_ctl metric match --include-hidden CLI option and usage hint. |
| src/traffic_ctl/CtrlCommands.h | Extends record_fetch API to optionally include hidden metric record types. |
| src/traffic_ctl/CtrlCommands.cc | Wires --include-hidden into metric match RPC requests. |
| include/shared/rpc/RPCRequests.h | Adds metric record-type set including RECT_HIDDEN_METRIC. |
| src/mgmt/rpc/handlers/records/Records.cc | Allows JSONRPC decoding of RECT_HIDDEN_METRIC as an opt-in record type. |
| include/records/RecDefs.h | Adds RECT_HIDDEN_METRIC bit outside RECT_ALL. |
| src/records/RecCore.cc | Enables matching hidden metrics during record lookups when explicitly requested. |
| src/records/RecordsConfig.cc | Updates metric_enabled validation range to include level 2. |
| doc/developer-guide/internal-libraries/Metrics.en.rst | Adds internal documentation for hidden metrics and derived metric aggregation APIs/semantics. |
| doc/developer-guide/internal-libraries/index.en.rst | Adds Metrics docs to internal libraries index. |
| doc/appendices/command-line/traffic_ctl.en.rst | Documents traffic_ctl metric match --include-hidden. |
| doc/admin-guide/monitoring/statistics/core/http-connection.en.rst | Documents per-server connection metrics, naming, and aggregate behavior. |
| doc/admin-guide/files/records.yaml.en.rst | Documents metric_enabled levels and metric_prefix configuration. |
| {RECT_CONFIG, "proxy.config.http.per_server.connection.min", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL} | ||
| , | ||
| {RECT_CONFIG, "proxy.config.http.per_server.connection.metric_enabled", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "[0-1]", RECA_NULL} | ||
| {RECT_CONFIG, "proxy.config.http.per_server.connection.metric_enabled", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "[0-2]", RECA_NULL} |
Contributor
Author
There was a problem hiding this comment.
Good catch, fixed to ^[0-2]$.
Confirmed the mechanism: recordRegexCheck in RecUtils.cc does regex.compile(pattern) && regex.exec(value), which is an unanchored search, so 10 matched on the 0. The anchored form is also the dominant convention in this file (124 entries use ^...$ versus 6 that do not, including a ^[0-1]$).
For completeness: the looseness was pre-existing rather than introduced here, since the previous value was [0-1], equally unanchored, and it was never a functional bug because the config update handler clamps the value. Two other unanchored [0-2] entries remain elsewhere in the file; happy to anchor those too, though it seemed out of scope for this PR.
Comment on lines
+247
to
+251
| The aggregates are recomputed periodically, currently every 5 seconds, rather than on every | ||
| connection event, so a reader sees a value up to that interval old. This applies to | ||
| ``current_connection_max`` as well: it reports the maximum across groups as of the last sample, not | ||
| a running peak. To obtain the peak over a longer window, compute a maximum over time from this gauge | ||
| in the monitoring system. |
Contributor
Author
There was a problem hiding this comment.
Correct, and fixed. The paragraph now says that every published per server metric is sampled on that interval, and calls out explicitly that the level 2 per group metrics are mirrored by the same periodic mechanism rather than written as connections open and close.
I also added that --include-hidden reads the internal per group metrics directly and so is not subject to the sampling delay, which seemed worth stating since it is the difference between the two ways of observing the same underlying value.
cmcfarlen
force-pushed
the
net-per-server-conn-metrics
branch
from
August 6, 2026 16:02
a1264ac to
b5e77cd
Compare
Aggregate the per group connection counts into per hostname metrics, using hidden metrics as the inputs and a derived metric for the aggregation, so the work happens on the stat sync task rather than on every connection. metric_enabled becomes a level rather than a flag: 0 disables, 1 publishes only the per hostname aggregates, 2 also mirrors the per group metrics into the published store. The per group metrics themselves are always created in the hidden store, so changing the level at runtime only changes what is registered for publication and never has to move a metric between stores. Aggregates are registered only for the 'both' match type, the only one with more than one group per hostname; for 'host' the group name is already the bare hostname and the two would collide on a single name. current_connection_max is the largest current count among a hostname's groups, sampled, rather than a monotone peak, so it falls again as traffic drains and a maximum over time can be computed downstream.
cmcfarlen
force-pushed
the
net-per-server-conn-metrics
branch
from
August 6, 2026 16:04
b5e77cd to
b096ed7
Compare
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.
Publishes per-upstream-server connection metrics aggregated per hostname, using hidden metrics as the inputs and a derived metric for the aggregation, so the aggregation work happens on the stat sync task rather than on every connection.
metric_enabledbecomes a levelproxy.config.http.per_server.connection.metric_enabledchanges from a flag to a level:0121, plus the per group metrics are mirrored into the published store.The per group metrics are always created in the hidden store whenever metrics are enabled at all. Only what is registered for publication varies by level, so changing the level at runtime never has to migrate a metric between the two stores.
Metrics
Three hidden per group metrics (
current_connection,total_connection,blocked_connection) feed four published per hostname aggregates: those three summed across the hostname's groups, pluscurrent_connection_max, theMAXof the groups' current counts.current_connection_maxis deliberately the maximum of the groups' instantaneous counts, not a monotone high-water mark. It answers "how close is the busiest group of this hostname toper_server.connection.maxright now", which matters because that limit is enforced per group rather than per hostname. Because it rises and falls, a monitoring system can compute max-over-time over any window from it; a monotone value would collapse the time dimension and only report that a peak happened at some point, not when. There is no per groupcurrent_connection_max— it exists only as an aggregate.Aggregates are registered only for the
bothmatch type, the only one with more than one group per hostname. Forhost,Group::metric_namealready returns the bare FQDN, so a host aggregate would collide with the single group's own name on one metric.No new hot-path work
TxnState::reserve,releaseandblockedare unchanged. They already branch on_count_metric != nullptrand use the typed mutators, andcreateHiddenPtrreturns the same pointer types, so those branches keep working with the metrics simply being hidden now.git diff master -- include/iocore/net/ConnectionTracker.hshows no logic added inside them.Also fixes the per group peak count
Group::_count_max, reported as themaxfield of the connection tracker group dump, had two defects, both pre-existing:update_max_count()made a singlecompare_exchange_weakattempt with no retry, so a racing update — or a spurious failure of the weak form, which is permitted by the standard and does occur on LL/SC architectures — silently discarded the sample.Testing
tests/gold_tests/origin_connection/per_server_connection_max.test.pyis extended to cover:traffic_ctl metric match per_server --include-hidden.match: both, with different concurrency per group (2 and 3) so theSUM(5) and theMAX(3) are distinguishable rather than coincidentally equal. Nothing previously covered a multi-group aggregate.Aggregates are recomputed on
ET_TASKeveryREC_RAW_STAT_SYNC_INTERVAL_MS(5000 ms) and no record drives that interval, so the test sleeps past a tick rather than trying to configure a faster one; reading too early would silently compare against zeros.The autest passes. Documentation is added for
metric_enabledandmetric_prefix, neither of which was documented before, plus the metrics themselves under the monitoring guide.Co-authored-by: @serrislew