stats: migrate rds/scope rds to use new stats API#45914
Conversation
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces tagged stats scopes for both RDS and Scoped RDS (SRDS) by extracting the connection manager prefix and route configuration names as tags instead of embedding them directly into the scope name. The review feedback points out a potential bug in the prefix stripping logic in rds_route_config_subscription.cc when the connection manager prefix is empty (e.g., when stat_prefix is "http.rds."), which would incorrectly resolve the prefix to "rds.". A more robust conditional stripping approach is suggested to resolve this issue.
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.
| absl::string_view conn_manager_prefix = | ||
| absl::StripSuffix(absl::StripPrefix(stat_prefix, "http."), ".rds."); |
There was a problem hiding this comment.
If stat_prefix is "http.rds." (which occurs when the connection manager prefix is empty), absl::StripPrefix(stat_prefix, "http.") returns "rds." . Since "rds." does not end with ".rds." , absl::StripSuffix will not strip anything and incorrectly returns "rds." as the connection manager prefix.
We can make this more robust by conditionally stripping the suffix depending on whether it ends with ".rds." or "rds." :
absl::string_view conn_manager_prefix = absl::StripPrefix(stat_prefix, "http.");
if (absl::EndsWith(conn_manager_prefix, ".rds.")) {
conn_manager_prefix = absl::StripSuffix(conn_manager_prefix, ".rds.");
} else if (absl::EndsWith(conn_manager_prefix, "rds.")) {
conn_manager_prefix = absl::StripSuffix(conn_manager_prefix, "rds.");
}There was a problem hiding this comment.
Pull request overview
This PR migrates RDS and Scoped RDS stats scope creation to Envoy’s newer tags/labels-friendly stats API, while preserving legacy flat stat-name behavior by providing an explicit tagged_name fallback.
Changes:
- Introduce tagged-scope construction helpers for Scoped RDS and RDS route config subscriptions.
- Emit connection-manager prefix and route-config identifiers as scope-level tags for HTTP RDS/SRDS (with defensive parsing/fallbacks).
- Add Bazel deps for
well_known_namesto access canonical tag names.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| source/common/router/scoped_rds.cc | Creates scoped RDS stats scopes using tagged names + well-known tag keys, with legacy fallback. |
| source/common/router/BUILD | Adds well_known_names dependency for scoped RDS library. |
| source/common/rds/rds_route_config_subscription.cc | Creates RDS stats scopes using tagged names for HTTP RDS, with legacy fallback for non-HTTP. |
| source/common/rds/BUILD | Adds well_known_names dependency for RDS library. |
| #include "absl/strings/match.h" | ||
| #include "absl/strings/str_join.h" | ||
| #include "absl/strings/strip.h" |
| // The scoped RDS stats scope is "http.<stat_prefix>.scoped_rds.<name>.". `stat_prefix` is always | ||
| // of the form "http.<stat_prefix>.", so recover the inner connection manager prefix and emit it | ||
| // (along with the scoped route config name) as a tag rather than baking it into the tag-extracted | ||
| // scope name. |
| #include "absl/strings/match.h" | ||
| #include "absl/strings/strip.h" |
Commit Message: stats: migrate rds/scope rds to use new stats API
Additional Description:
Parts of #20289. This migrate the previous stats creation to use new tags-friendly API. But note, before we merge #45846 and enable it explicitly. This won't bring any changes to the final behavior because the legacy mode will ignore the provided tags but only use the flat name.
Risk Level: low.
Testing: unit.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.