Retract per server metrics when the mode changes - #13666
Draft
cmcfarlen wants to merge 8 commits into
Draft
Conversation
A metric name, once created, was published for the life of the process. Any metric whose name or publication policy depends on a runtime changeable setting could therefore never retract a name it had already published, so such a setting only ever took effect for names created after the change. Unlisting takes a slot out of the store's listing. It keeps its slot, its name and its atomic, so lookup by name still resolves and creating the name again relists it with its value intact. Iteration skips it, which is what removes it from traffic_ctl, the JSONRPC record lookup and stats_over_http without any of them changing.
Each iterator captures its own bound, and exhaustion was judged against that. A subrange whose stop iterator was made later held a larger bound, so the walk could pass its own bound and go on comparing unequal to a stop that was still live, with operator++ unable to make progress. Two find() calls with a metric created between them was enough. Exhaustion between two positional iterators is now judged against the earlier of the two bounds, so such a subrange ends at the earlier snapshot. The sentinel keeps its own answer, since its bound means nothing.
It asserted the store had at least one listed metric left, which depends on what other sections put there. A listed metric of its own says the same thing without that coupling.
Exhaustion is a property of an iterator's own snapshot bound, so two taken at different times can compare equal to each other while disagreeing about end. That is not a total equivalence relation, which makes these unfit for a generic algorithm; only same snapshot comparisons, and comparison against end, are meaningful.
The previous note disclaimed the equivalence relation while the type still declared input_iterator_tag, which advertises what it then denied. A snapshot is the sequence: iterators from different ones are no more comparable than iterators into different containers, so mixing them is unspecified rather than broken, and within one snapshot equality is the relation an input iterator requires.
metric_aggregate is dynamic and overridable, but the publication decision is made when a group is constructed and a published metric name was never removable. A name published while the setting was 0 therefore kept reporting for the life of the process, leaving per group and per hostname metrics side by side at metric_aggregate 2. AGGREGATE_ONLY now tombstones the per group names it declines to publish. A group is rebuilt on the first connection after its count falls to zero, so the change converges as groups go idle.
ATS metric names separate a qualifier with a dot, as in proxy.process.eventloop.time.max, not an underscore. The aggregate added in apache#13506 has only ever existed on master, so renaming it now costs nothing. Also wait for the reconfigure in the retraction autest: http_config_cb schedules it a second out, so a request made as soon as traffic_ctl returns is still served by the previous configuration.
The requirement for the suppressed-per-group mode was a single metric per hostname, the max, rather than the sums as well. Mode 2 is now that max alone, and mode 3 is the sums and the max, for when the totals are wanted too. Mode 1 is unchanged. The sums are withdrawn the same way the per group metrics are when a mode stops asking for them.
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.
Stacked on #13616, which adds
ts::Metrics::unlist. Until that merges this branch carries its commit too, so review the last three commits here. Draft for that reason, not because the work is unfinished.Problem
proxy.config.http.per_server.connection.metric_aggregateisRECU_DYNAMICand overridable, but the decision it drives — which per server metric names get published — was made once, in theConnectionTracker::Groupconstructor, and a published metric name could not be withdrawn. So the setting only ever took effect for names created after it changed.Seen in production. A box that ran for a while at
metric_aggregate0 before being switched to 2 reports both shapes, and no reload removes the first set:The metric store hands out ids in allocation order and
traffic_ctlprints them that way, so the dump is a timeline: every<fqdn>.<ip>:<port>name was created before the first aggregate name, with no interleaving. The config change took effect for everything after it; what came before was unretractable.What the modes mean now
The suppressed-per-group mode was specified as a single metric per hostname — the max — not the sums as well. Mode 2 is that, and mode 3 is new for when the totals are wanted too:
AGGREGATE_NONEAGGREGATE_GROUPAGGREGATE_MAXAGGREGATE_SUMMode 1 is unchanged.
AGGREGATE_ONLYis gone; 2 and 3 replace it.The constructor now reduces to three independent decisions — publish the sums, publish the max, publish the per group metrics — each of which either registers a derived source or unlists the name. That reads better than the nested condition it replaces, and it is what makes 3 → 2 withdraw the sums rather than leave them behind.
An out of range value from a plugin is normalised to
AGGREGATE_GROUPonce, at the top of the constructor, rather than being implicit in the conditions.Metric rename
current_connection_maxbecomescurrent_connection.max. ATS separates a qualifier with a dot —proxy.process.eventloop.time.max,.events.max— not an underscore. The metric only exists on master, from #13506, so the rename is free now and would not be after a release carries it.What converges, and when
A change is applied per group, when that group is next constructed, which happens on the first connection after its count last fell to zero.
Group::release()is called fromPoolableSession::release_outbound_connection_tracking(), so it is the upstream session closing that erases the group, not the transaction ending. With origin keep alive on, a pooled session holds a group open and that group keeps whatever setting it was built with; a group that never goes idle never re-evaluates.Two consequences worth knowing rather than discovering:
Both are documented at the enum and in
records.yaml.en.rst.Tests
src/iocore/net/unit_tests/test_ConnectionTracker.ccis new. It drives the production sequence in process: run atAGGREGATE_NONEso the per group names publish, switch, open and close another connection, assert the names are gone and the aggregates are there. Nine sections cover each mode, both switch directions for the sums, and the no-aggregate fallback at 2 and 3.Getting that harness right took a correction worth recording:
TxnState::release()only decrements, so a test using it never erases the group and nothing is re-evaluated. It has to follow the real path —TxnState::drop()into the session, thenGroup::release().per_server_connection_max.test.pygainsAggregateRetractionTest, which drives traffic at 0, asserts the per group name is published so the later assertion cannot pass vacuously, raises the setting withtraffic_ctl, drives traffic again and asserts the withdrawal. It disables origin keep alive so group churn is deterministic, and waits after thetraffic_ctlcall becausehttp_config_cbschedules the reconfigure a second out — without that wait the next request is still served by the previousHttpConfigParams, which looks exactly like a failure to retract.MultiGroupAggregateTestgains anExcludesExpression. Every assertion in that file was aContainsExpression, which is why the original leak went unnoticed; a test that only checks for presence cannot catch a metric that should not be there.Verified the autest fails without the fix: with the
unlistcall disabled, exactly one assertion fails, the retraction one. Four autests pass in a Fedora 44 container on the CI image — the threeper_server*tests andslow_post, which exercises the same constructor throughconnection.maxenforcement.