Skip to content

stats: migrate rds/scope rds to use new stats API#45914

Open
wbpcode wants to merge 1 commit into
envoyproxy:mainfrom
wbpcode:dev-imigrate-to-new-stats-api-4
Open

stats: migrate rds/scope rds to use new stats API#45914
wbpcode wants to merge 1 commit into
envoyproxy:mainfrom
wbpcode:dev-imigrate-to-new-stats-api-4

Conversation

@wbpcode

@wbpcode wbpcode commented Jul 1, 2026

Copy link
Copy Markdown
Member

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.

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
@wbpcode

wbpcode commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +26 to +27
absl::string_view conn_manager_prefix =
absl::StripSuffix(absl::StripPrefix(stat_prefix, "http."), ".rds.");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.");
  }

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_names to 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.

Comment on lines +26 to +28
#include "absl/strings/match.h"
#include "absl/strings/str_join.h"
#include "absl/strings/strip.h"
Comment on lines +43 to +46
// 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.
Comment on lines +7 to +8
#include "absl/strings/match.h"
#include "absl/strings/strip.h"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants