Add hidden metrics and MAX/MIN/incremental derived metric aggregation - #13505
Open
cmcfarlen wants to merge 8 commits into
Open
Add hidden metrics and MAX/MIN/incremental derived metric aggregation#13505cmcfarlen wants to merge 8 commits into
cmcfarlen wants to merge 8 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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends ts::Metrics with (1) a separate hidden-metrics store for high-cardinality internal values and (2) richer derived-metric aggregation (SUM/MAX/MIN plus incremental source registration), and wires this through traffic_ctl metric match --include-hidden via JSONRPC, with accompanying tests and documentation.
Changes:
- Add
Metrics::hidden_instance()plusGauge::createHiddenPtr/Counter::createHiddenPtrto record internal metrics that are structurally unreachable from the published registry. - Enhance derived metrics with
Op { SUM, MAX, MIN }, correct accumulator seeding, addDerived::add_source(), and fix handling of unresolved sources. - Add
traffic_ctl metric match --include-hiddenend-to-end support (including JSONRPC request decoding), plus unit + gold tests and new internal library docs.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/gold_tests/jsonrpc/metric_match_include_hidden.test.py | New gold test validating JSONRPC accepts the new rec type and traffic_ctl flag. |
| src/tsutil/unit_tests/test_Metrics.cc | Expands Metrics unit coverage; removes order-dependent assertions; adds tests for ops/add_source/hidden store. |
| src/tsutil/Metrics.cc | Implements hidden instance; fixes blob exhaustion; adds derived ops + add_source and unresolved-source skipping. |
| src/traffic_ctl/traffic_ctl.cc | Adds --include-hidden option and usage for traffic_ctl metric match. |
| src/traffic_ctl/CtrlCommands.h | Extends record_fetch signature to accept an include-hidden flag; adds option key constant. |
| src/traffic_ctl/CtrlCommands.cc | Passes --include-hidden into the JSONRPC record lookup request. |
| src/records/RecCore.cc | Adds hidden-metric enumeration to regex record lookup. |
| src/mgmt/rpc/handlers/records/Records.cc | Allows RECT_HIDDEN_METRIC in JSONRPC request decoding (opt-in). |
| include/tsutil/Metrics.h | Public API additions for hidden metrics and derived ops/add_source. |
| include/shared/rpc/RPCRequests.h | Adds METRIC_REC_TYPES_INCLUDE_HIDDEN for include-hidden requests. |
| include/records/RecDefs.h | Defines RECT_HIDDEN_METRIC = 0x40 deliberately outside RECT_ALL. |
| doc/developer-guide/internal-libraries/Metrics.en.rst | New internal library documentation for hidden + derived metrics behavior and constraints. |
| doc/developer-guide/internal-libraries/index.en.rst | Adds Metrics to internal libraries index. |
| doc/appendices/command-line/traffic_ctl.en.rst | Documents traffic_ctl metric match --include-hidden. |
Comment on lines
+619
to
+626
| RecRecord tmp; | ||
|
|
||
| tmp.rec_type = RECT_PROCESS; | ||
|
|
||
| tmp.name = name.data(); | ||
| tmp.data_type = type == ts::Metrics::MetricType::COUNTER ? RECD_COUNTER : RECD_INT; | ||
| tmp.data.rec_int = val; | ||
| callback(&tmp, data); |
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.
Adds two facilities to
ts::Metrics, plus atraffic_ctloption to inspect the first, and fixes three pre-existing defects found along the way. A follow-up PR uses these for per-upstream-server connection metrics; this PR is independently useful and stands on its own.Hidden metrics
A second
Storageinstance, reached throughMetrics::hidden_instance(), for high-cardinality intermediate values that are worth recording but not worth publishing.Gauge::createHiddenPtr/Counter::createHiddenPtrreturn the same correctly typed pointer ascreatePtr, so a hidden metric is read and written with the ordinary typed mutators with no cast at the call site.A separate store rather than a per-metric "hidden" flag is deliberate: it makes hidden metrics structurally unreachable from the published store, so no consumer can expose one by forgetting to check a flag.
Since that also makes them hard to debug,
traffic_ctl metric match --include-hiddenlists them. The rec type bit for this (RECT_HIDDEN_METRIC = 0x40) sits deliberately outsideRECT_ALL(0x3F), so hidden metrics are returned only when explicitly asked for and never as a side effect of a broad query.Derived metric aggregation
MAXandMINin addition toSUM. The accumulator is seeded from the first source rather than from zero, since a zero seed is only correct forSUMand would clampMINto<= 0.Derived::add_source(), for aggregates whose sources are discovered while the process runs rather than known at startup. Repeatedly callingderive()for one derived name does not work for this: 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 into a single entry, and re-registering an existing source is a no-op.Pre-existing defects fixed
Storage::create()had no exhaustion check. Filling the last blob let the following bookkeeping calladdBlob()and write one past the end of_blobs. Verified by temporarily shrinkingMAX_BLOBS: the current code segfaults, and thedebug_assertinaddBlob()does not catch it because it is off by one against the access it guards (_blobs[++_cur_blob]) — and being adebug_assert, it is compiled out of release builds entirely. Now arelease_assertagainstMAX_BLOBS - 1, withcreate()refusing the final slot and returning the reservedbad_id.lookup(), which masks the unresolved id down to the reservedbad_idslot, so the aggregate silently included that slot's value. Observable underMAX/MIN, where thebad_idvalue can become the winning one; underSUMit was hidden bybad_idholding zero.Testing
--order randacross many seeds. Each fix was checked against the unfixed code first to confirm the new assertions actually discriminate.tests/gold_tests/jsonrpc/metric_match_include_hidden.test.pycovers the--include-hiddenRPC round trip end to end. This is load-bearing: the rec type also has to be accepted by the JSONRPC request decoder, which validates each requested type against a whitelist and rejects the entire request otherwise. That was invisible to every build-level check and only showed up end to end.traffic_serverwith two temporary hidden metrics registered: absent frommetric match, present with--include-hidden, and absent from a broadproxy.processquery of ~900 published metrics.-Wwithnitpicky = True.Co-authored-by: @serrislew