hot restart: propagate programmatic stat tags across restart

[bpalermo]

Commit Message: hot restart: propagate programmatic stat tags across restart

Additional Description:

Stats created with programmatic tags (via Scope::*FromStatNameWithTags / the tag-aware *FromTaggedName API) embed their tag values into the flat stat name while keeping a clean tagExtractedName and a set of tags. This works on a fresh process, but breaks across a hot restart:

• The parent serializes each counter/gauge to the child as name string → value only (HotRestartingParent::Internal::exportStatsToChild); tags() / tagExtractedName() are dropped.
• The child's StatMerger::mergeCounters re-creates the stat from the name alone, which runs only regex tag extraction. With no matching stats_tags regex, the merged stat gets empty tags and tagExtractedName == the full mangled name.
• That no-tags stat is created before the child's own code first touches the stat, so it wins the central-cache slot. The later tagged creation returns the already-mangled stat (stats are immutable post-creation). The tag values end up baked into the metric name with empty labels (e.g. as exported to Prometheus).

This is the gap behind the long-standing // TODO(snowp): Propagate tag values during hot restarts in stat_merger.cc. Regex-extracted tags survive only because extraction re-derives them from the name; programmatic tags have nothing to re-derive from.

This change actually propagates the tags:

• hot_restart.proto: new TaggedMetric (tag-extracted name + repeated Tag) and counter_tags / gauge_tags maps on Reply.Stats.
• The parent emits an entry only for tags the child could not re-derive from the name: it re-runs the store's TagProducer on the flat name and skips the stat when extraction reproduces the stored tags (the common case — including all default tag regexes). The stats transfer payload is therefore unchanged except for stats that actually carry programmatic tags. A nullable Store::tagProducer() accessor (default nullptr; only ThreadLocalStoreImpl overrides) supports the comparison.
• The child decodes the metadata and StatMerger re-creates the stat with the original tags via new Scope::counterFromMergedStatName / gaugeFromMergedStatName. Following stats: new tags friendly scope based API #45146 these have non-virtual-breaking defaults that delegate to the tag-aware counterFromTaggedName / gaugeFromTaggedName (which retain the metadata on tag-aware scopes and IsolatedStoreImpl); the legacy ThreadLocalStore ScopeImpl — which intentionally drops tag metadata on the *FromTaggedName path because it cannot compose tag-extracted names with its prefix in general — overrides them to honor the components, which arrive fully resolved. The full name is still reconstructed via DynamicContext (dynamic spans preserved), so the cache key matches the stat the child independently creates — the fix removes the first-write-wins poisoning rather than working around it.

Risk Level: Medium — changes stats labels after a hot restart for tag-bearing stats. Runtime guarded.

Testing: New tests: stat_merger_test (ProgrammaticTagsSurviveMerge, ProgrammaticGaugeTagsSurviveMerge, ProgrammaticTagsLostWithoutMetadata pinning the pre-fix symptom), hot_restarting_parent_test (ExportsTagMetadataOnlyWhenNotRederivable, TagMetadataAppliedToMergedStats end-to-end, TagMetadataIgnoredWhenRuntimeFlagDisabled), and thread_local_store_test (MergedStatNameHonorsSuppliedTags on both the legacy and tag-aware scopes). Built/run via RBE.

Docs Changes: N/A

Release Notes: Added (changelogs/current/bug_fixes/stats__hot-restart-propagate-programmatic-stat-tags.rst).

Runtime guard: envoy.reloadable_features.hot_restart_propagate_stat_tags (revert to legacy name-derived behavior).

[repokitteh-read-only]

Hi @bpalermo, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #45674 was opened by bpalermo.

see: more, trace.

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #45674 was opened by bpalermo.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #45674 was ready_for_review by bpalermo.

see: more, trace.

[botengyao]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses a bug where stats created with programmatic tags lost their labels across hot restarts. It introduces mechanisms to transmit tag-extracted names and tag values from the parent process to the child, allowing the child to correctly re-create the stats with identical labels. The review feedback focuses on minor performance and readability optimizations, including reserving space in maps and protobuf repeated fields to prevent unnecessary reallocations, and simplifying absl::Span construction by utilizing implicit conversions.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[paul-r-gall]

please fix format and address ai-gen comments.

[RyanTheOptimist]

/wait

[bpalermo]

Updated — changes since the review:

• Merged main (no force-push): stats: new tags friendly scope based API #45146 (tag-aware scope API) landed in the meantime and refactored exactly this area. The merge adapts the fix to it and makes the diff smaller: counterFromMergedStatName/gaugeFromMergedStatName now default to the tag-aware *FromTaggedName (correct on tag-aware scopes and IsolatedStoreImpl with zero changes there), and only the legacy ThreadLocalStore::ScopeImpl — which intentionally drops tag metadata on that path — overrides them (via the tag-aware TagStatNameJoiner). The previous getOrCreate*Base signature refactor is gone.
• Payload gating (new): while re-validating I realized the parent was emitting tag metadata for any stat with tags — which, with the default tag regexes, is most stats, bloating the hot-restart stats transfer for everyone. The parent now re-runs the store's TagProducer on the flat name and skips stats whose tags the child re-derives anyway; only true programmatic tags are transmitted. Added a nullable Store::tagProducer() accessor (default nullptr) to support this.
• AI review comments addressed: the two reserve() suggestions applied; the two absl::Span conversion suggestions no longer apply (that code was removed in the stats: new tags friendly scope based API #45146 adaptation) — replied inline.
• Format fixed: clang-format (18.1.8) + the two spell-check errors (inlines → embeds).
• Coverage: added tests for the previously uncovered paths — parent-side gating, end-to-end export→merge tag propagation, runtime-flag-off legacy behavior, the gauge merge path, and the merged-stat creation methods on both legacy and tag-aware scopes. (The earlier tsan/arm64 failures were unrelated flakes: NetworkExtensionDiscoveryIntegrationTest, QUIC keepalive idle timeout.)

cc @paul-r-gall

[ggreenway]

@paul-r-gall please review

[ggreenway]

@wbpcode any concerns with relation to your recent stat tags changes?

[wbpcode]

Thanks for calling this out. This is a great fix. I guess this bug is there for very very very long time because we have added the *FromWithTags for very very very long time.

Because the 1.39 release is approaching, let's merge this to 1.40. And I added some comments. Thanks again.

[wbpcode]

I guess we may needn't this new method but could use the counterFromTaggedName() directly?

[bpalermo]

Calling counterFromTaggedName() directly would reintroduce the bug: the merge scope is the legacy ScopeImpl, whose counterFromTaggedName drops the tags when a pre-joined tagged_name is supplied (it can't compose tag metadata with its prefix in general — that's the contract from #45146). The merged stat would then be created with no tags and win the central-cache slot. counterFromMergedStatName exists so the legacy scope can honor the fully-resolved components from the parent (safe on the merge path specifically, because the scope prefix is empty), while TagScopeImpl maps it back onto counterFromTaggedName, which it does honor. Happy to restructure if you'd rather change counterFromTaggedName's contract on the legacy scope instead, but that would affect all existing callers of the tag-aware API.

[wbpcode]

Sorry. But I didn't get you here. Did you mean a stat with specific tags may cannot be re-constructed because if we specific the full name, the tags will be ignored (for legacy scope), if we not specific the full name, we may lost the correct full name?

if I understood correctly, how this API fix it? Could you share a specific example? Like for example, a stat that created with *FromStatNameWithTags("xxxxx", {{"tag", "tagv"}}), how your API hope? seems the legacy scope still will ignore the input tags because there is a full_name?

If I understood wrong, could you illustrate a little more?

Thanks.

BTW, we needn't take the the case where the old and new scope implementation be used at same time when doing the hot restart. It's bring chaos. We can document it clear to this limitation.

[bpalermo]

Good catch — that trade-off is exactly right for counterFromTaggedName, but counterFromMergedStatName is a different method that avoids it. Concrete example:

Parent creates counterFromStatNameWithTags("custom.requests", {{"source","svc-a"}}) → flat name custom.requests.source.svc-a, tag-extracted name custom.requests, tags [source=svc-a]. It transmits all three over the wire.

On the child, the merge scope is the legacy ScopeImpl (the merge uses store.createScope("") and use_tag_scope defaults to false). If StatMerger called counterFromTaggedName("custom.requests", [source=svc-a], /*tagged_name=*/"custom.requests.source.svc-a"), you're right — the legacy scope drops the tags because tagged_name is set, and we'd get the mangled no-tags counter. That's the bug.

counterFromMergedStatName instead uses the same 7-arg tag-aware joiner that TagScopeImpl::counterFromTaggedName uses, so it keeps the exact flat name as the cache key and the tag metadata:

counterFromMergedStatName("custom.requests.source.svc-a", "custom.requests", [source=svc-a])
  → name             = custom.requests.source.svc-a   (matches parent; == child's own WithTags cache key)
    tagExtractedName = custom.requests
    tags             = [source=svc-a]

We can't just re-derive the flat name from tag_extracted_name + tags, because it has to match the parent's exact name (dynamic components, tag order) and the key the child's own *FromStatNameWithTags will later look up.

Agree on documenting that parent and child must use the same scope implementation — I'll add that.

[wbpcode]

Rather than to extract the tags and compare them, could we add a flag to the metrics directly so we can know the stats has programmatic tags? Like our current used() and so on. We could add two method: markAsNoTagExtracion() and bool NoTagExtraction() to tell whether a stat has tags that generated by the extraction rules/tag producer.

[bpalermo]

That's a nicer design — it also removes the assumption that parent and child share the same tag-extraction config (the re-derivability check runs with the parent's tag producer, but it's the child that re-derives). I'll add a flag bit (there's room in Metric::Flags), set it on the programmatic-tag creation paths — including merged-stat re-creation so a second hot restart still ships the metadata — and drop the TagProducer plumbing from HotRestartingParent.

[bpalermo]

Done in 0af527a — added Metric::noTagExtraction() / markAsNoTagExtraction() backed by a new Flags::NoTagExtraction bit. The stores mark stats at creation when tags are supplied by the caller (including stats re-created by the hot restart merge, so a second hot restart still exports their metadata), and recordTags is now just the flag check — the TagProducer plumbing and Store::tagProducer() accessor are gone. One deviation from the sketch: the two methods have default implementations on Metric (false / no-op) rather than being pure, so metric types without allocator flags (null stats, histograms) and mocks don't need boilerplate — happy to make them pure if you'd rather.

[wbpcode]

@wbpcode any concerns with relation to your recent stat tags changes?

Nope. This is old bug and the fix also be independent with my migration.

[bpalermo]

flaky perhaps? 🤔

[bpalermo]

/retest

[bpalermo]

@wbpcode gentle ping 🙏 ?

[bpalermo]

/retest