-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ecds: use top level stats prefix #20396
Conversation
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
/retest |
Retrying Azure Pipelines: |
@@ -110,7 +110,7 @@ config_source: { ads: {} } | |||
} | |||
|
|||
return filter_config_provider_manager_->createDynamicFilterConfigProvider( | |||
config_source, name, factory_context_, "xds.", last_filter_config, "http"); | |||
config_source, name, factory_context_, "", last_filter_config, "http"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a plan to remove the reloadable feature and use http_filter
as the only stat prefix? I am not sure if it is worth to add a test with envoy.reloadable_features.top_level_ecds_stats
disabled and stat prefix set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md#runtime-guarding, we should maintain both code paths for one release cycle. I'll recover coverage for the original path.
/retest |
Retrying Azure Pipelines: |
/retest |
Retrying Azure Pipelines: |
@@ -80,8 +80,9 @@ for HTTP filters. | |||
|
|||
Extension config discovery service has a :ref:`statistics | |||
<subscription_statistics>` tree rooted at | |||
*<stat_prefix>.extension_config_discovery.<extension_config_name>*. In addition | |||
to the common subscription statistics, it also provides the following: | |||
*<stat_prefix>.extension_config_discovery.<extension_config_name>*. For HTTP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are changing the stat tree anyway, did you consider actually inverting this and making this something like:
extension_config_discovery.<stat_prefix>.<extension_config_name>
?
It seems like this would be more clear as we use ECDS in more places? WDYT?
/wait-any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree this is more consistent with listener manager and cluster manager stats. Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov kuat@google.com
Commit Message: Fixes #19638
Risk Level: low
Testing: updated
Docs Changes: yes
Release Notes: yes
Runtime guard: envoy.reloadable_features.top_level_ecds_stats