route: add stats prefix for route by ramaraochavali · Pull Request #21302 · envoyproxy/envoy

ramaraochavali · 2022-05-16T07:49:43Z

Commit Message: add stats prefix for route
Additional Description: Adds API for stats generation support per route.
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:

API for #3351

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

repokitteh-read-only · 2022-05-16T07:49:51Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #21302 was opened by ramaraochavali.

see: more, trace.

ramaraochavali · 2022-05-16T07:50:01Z

opening API only change for discussion

mattklein123

Thanks generally makes sense to me with some small comments.

/wait

mattklein123 · 2022-05-16T20:25:48Z

api/envoy/config/route/v3/route_components.proto

    NonForwardingAction non_forwarding_action = 18;
+
+    // [#not-implemented-hide:]
+    // The human readable prefix to use when emitting statistics for this endpoint.


Where are the statistics rooted? Can you clarify?

mattklein123 · 2022-05-16T20:26:07Z

api/envoy/config/route/v3/route_components.proto

+    // This should be set for highly critical
+    // endpoints that you wish to get “perfect” statistics on.


Suggested change

// This should be set for highly critical

// endpoints that you wish to get “perfect” statistics on.

// This can be set for highly critical

// endpoints that one wishes to get “perfect” statistics on.

mattklein123 · 2022-05-16T20:26:34Z

api/envoy/config/route/v3/route_components.proto

+    // This should be set for highly critical
+    // endpoints that you wish to get “perfect” statistics on.
+    //
+    // This emits statistics documentation at :ref:`virtual cluster statistics <config_http_filters_router_vcluster_stats>`.


Suggested change

// This emits statistics documentation at :ref:`virtual cluster statistics <config_http_filters_router_vcluster_stats>`.

// This emitted statistics are the same as those documented at :ref:`virtual cluster statistics <config_http_filters_router_vcluster_stats>`.

mattklein123 · 2022-05-16T20:26:48Z

api/envoy/config/route/v3/route_components.proto

+    //
+    // This emits statistics documentation at :ref:`virtual cluster statistics <config_http_filters_router_vcluster_stats>`.
+    //
+    // .. note::


Suggested change

// .. note::

// .. warning::

mattklein123 · 2022-05-16T20:27:36Z

api/envoy/config/route/v3/route_components.proto

+    //    every application endpoint. This is both not easily maintainable and as well the
+    //    statistics output is not free.


Suggested change

// every application endpoint. This is both not easily maintainable and as well the

// statistics output is not free.

// every application endpoint. This is both not easily maintainable and statistics use a non-trivial amount of memory.

mattklein123 · 2022-05-16T20:28:00Z

api/envoy/config/route/v3/route_components.proto

+    //    We do not recommend setting up a stat prefix for
+    //    every application endpoint. This is both not easily maintainable and as well the
+    //    statistics output is not free.
+    string stat_prefix = 19;


I assume empty means no endpoints stats? If so can you clarify?

yes. I will clarify

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

mattklein123

LGTM with small comments. Let's just do the implementation in this PR.

/wait

mattklein123 · 2022-05-17T20:20:12Z

api/envoy/config/route/v3/route_components.proto


    // [#not-implemented-hide:]
    // The human readable prefix to use when emitting statistics for this endpoint.
+    // The statistics are rooted at vhost.<stat_prefix>.


Should't it be something like: vhost..<stat_prefix> ?

api/envoy/config/route/v3/route_components.proto

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2022-05-25T14:46:56Z

envoy/router/router.h

+  /**
+   * @return PathStatsConfig the config needed to generate path level stats.
+   */
+  virtual const absl::optional<PathStatsConfig>& pathStatsConfig() const PURE;


Should we use Ptr instead of optional reference here?

This is fine.

ramaraochavali · 2022-05-25T14:47:56Z

source/common/router/config_impl.cc

              ? route.route().host_rewrite_path_regex().substitution()
              : ""),
      append_xfh_(route.route().append_x_forwarded_host()), cluster_name_(route.route().cluster()),
+      path_stat_name_storage_(route.stat_prefix(), factory_context.scope().symbolTable()),


Is there a way to initialize this only if stat prefix is there? Does it really matter?

@jmarantz can help review this part. I would have to dig back in and relearn how all of this works at this point. I'm not sure if this is the right way or there is a better way.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2022-05-25T14:54:54Z

@mattklein123 PTAL. Still need to add tests. Want to get your first pass for the implementation. I will have to add this at few more places like timeouts and need to add tests.

ramaraochavali · 2022-05-25T14:56:10Z

This is how stats will come

vhost.backend.path.service1stats.upstream_rq_503: 1
vhost.backend.path.service1stats.upstream_rq_5xx: 1
vhost.backend.path.service1stats.upstream_rq_completed: 1
vhost.backend.path.service2stats.upstream_rq_503: 1
vhost.backend.path.service2stats.upstream_rq_5xx: 1
vhost.backend.path.service2stats.upstream_rq_completed: 1

ggreenway

This is how stats will come

vhost.backend.path.service1stats.upstream_rq_503: 1 vhost.backend.path.service1stats.upstream_rq_5xx: 1 vhost.backend.path.service1stats.upstream_rq_completed: 1 vhost.backend.path.service2stats.upstream_rq_503: 1 vhost.backend.path.service2stats.upstream_rq_5xx: 1 vhost.backend.path.service2stats.upstream_rq_completed: 1

Please add tag-extraction rules for these stats. See source/common/config/well_known_names.cc and add appropriate rules there.

Related: #21397

mattklein123

Looks directionally correct, modulo some comments from a first pass. Please add tests including integration tests. Thank you!

/wait

mattklein123 · 2022-05-25T22:38:20Z

api/envoy/config/route/v3/route_components.proto

  google.protobuf.UInt32Value per_request_buffer_limit_bytes = 16;
+
+  // The human readable prefix to use when emitting statistics for this endpoint.
+  // The statistics are rooted at vhost.<<virtual host name>.<stat_prefix>.


Suggested change

// The statistics are rooted at vhost.<<virtual host name>.<stat_prefix>.

// The statistics are rooted at vhost.<virtual host name>.<stat_prefix>.

mattklein123 · 2022-05-25T22:42:48Z

source/common/router/router.h

  Upstream::ClusterInfoConstSharedPtr cluster_;
  std::unique_ptr<Stats::StatNameDynamicStorage> alt_stat_prefix_;
  const VirtualCluster* request_vcluster_;
+  const PathStatsConfig* path_stats_config_;


I'm not sure it's worth it to keep a member variable for this. I think you can just look it up when you need it in the route.

It is needed similar to request_vcluster_ because some of the code that increments metrics does not have access to route (not passed in)

mattklein123 · 2022-05-25T22:44:28Z

envoy/router/context.h

  virtual const VirtualClusterStatNames& virtualClusterStatNames() const PURE;

+  /**
+   * @return a struct containing StatNames for path level stats.


Instead of "path" I would call this "route" level stats as they might match on a lot of different things. Change this here and elsewhere.

mattklein123 · 2022-05-25T22:44:52Z

envoy/router/router.h

+  /**
+   * @return PathStatsConfig the config needed to generate path level stats.
+   */
+  virtual const absl::optional<PathStatsConfig>& pathStatsConfig() const PURE;


This is fine.

mattklein123 · 2022-05-25T22:46:11Z

source/common/router/config_impl.cc

+    std::cout << "Setting retry policy...." << route_config.retry_policy().num_retries().value()
+              << "\n";


envoy/router/router.h

mattklein123 · 2022-05-25T22:47:36Z

source/common/router/config_impl.cc

              ? route.route().host_rewrite_path_regex().substitution()
              : ""),
      append_xfh_(route.route().append_x_forwarded_host()), cluster_name_(route.route().cluster()),
+      path_stat_name_storage_(route.stat_prefix(), factory_context.scope().symbolTable()),


@jmarantz can help review this part. I would have to dig back in and relearn how all of this works at this point. I'm not sure if this is the right way or there is a better way.

ramaraochavali · 2022-05-26T06:48:04Z

Sure. I will be OOO rest of the week. I will address comments and add tests later next week.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

mattklein123

LGTM with small comment comment, thanks.

/wait

mattklein123 · 2022-06-15T16:04:43Z

api/envoy/config/route/v3/route_components.proto

+  //
+  //    We do not recommend setting up a stat prefix for
+  //    every application endpoint. This is both not easily maintainable and
+  //    statistics use a non-trivial amount of memory(approximately 1k per route).


Suggested change

// statistics use a non-trivial amount of memory(approximately 1k per route).

// statistics use a non-trivial amount of memory (approximately 1KiB per route).

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

comment's addressed

jmarantz · 2022-06-15T22:34:17Z

one more merge-main is needed.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2022-06-16T07:02:21Z

/retest

repokitteh-read-only · 2022-06-16T07:02:24Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #21302 (comment) was created by @ramaraochavali.

see: more, trace.

ramaraochavali · 2022-06-16T07:47:01Z

/retest

repokitteh-read-only · 2022-06-16T07:47:04Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #21302 (comment) was created by @ramaraochavali.

see: more, trace.

Commit Message: add stats prefix for route Additional Description: Adds API for stats generation support per route. Risk Level: Low Testing: Docs Changes: Release Notes: Platform Specific Features: API for envoyproxy#3351 Signed-off-by: Rama Chavali <rama.rao@salesforce.com> Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>

add stats prefix for route

afcc86c

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

repokitteh-read-only bot added the api label May 16, 2022

repokitteh-read-only bot assigned mattklein123 May 16, 2022

mattklein123 requested changes May 16, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label May 16, 2022

review comments

aff9d24

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

repokitteh-read-only bot removed the waiting label May 17, 2022

mattklein123 requested changes May 17, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label May 17, 2022

ramaraochavali added 3 commits May 21, 2022 14:42

wip impl

ae6fb73

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Merge remote-tracking branch 'upstream/main' into fix/route_stats

dc1b306

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

basic implementation

ad705e5

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali requested a review from ggreenway as a code owner May 25, 2022 14:44

repokitteh-read-only bot removed the waiting label May 25, 2022

remove unnecessary

564d11e

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali commented May 25, 2022

View reviewed changes

resolve conflicts

3696144

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ggreenway previously requested changes May 25, 2022

View reviewed changes

mattklein123 requested changes May 25, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label May 25, 2022

fix compile

84c362a

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

repokitteh-read-only bot removed the waiting label May 26, 2022

some review comments

a02822d

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

mattklein123 added the waiting label May 26, 2022

Merge remote-tracking branch 'upstream/main' into fix/route_stats

267975a

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

mattklein123 requested changes Jun 15, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 15, 2022

route comment

14b4d74

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali dismissed jmarantz’s stale review via 14b4d74 June 15, 2022 16:18

repokitteh-read-only bot removed the waiting label Jun 15, 2022

jmarantz previously approved these changes Jun 15, 2022

View reviewed changes

mattklein123 previously approved these changes Jun 15, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Jun 15, 2022

resolve conflicts

83f70eb

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali dismissed stale reviews from mattklein123 and jmarantz via 83f70eb June 16, 2022 03:06

repokitteh-read-only bot added the api label Jun 16, 2022

jmarantz previously approved these changes Jun 16, 2022

View reviewed changes

format

51d5655

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali dismissed jmarantz’s stale review via 51d5655 June 16, 2022 06:56

jmarantz approved these changes Jun 16, 2022

View reviewed changes

jmarantz merged commit 7b937e8 into envoyproxy:main Jun 16, 2022

ramaraochavali deleted the fix/route_stats branch June 16, 2022 13:21

This was referenced Jun 16, 2022

Expose metrics per vhost #3351

Closed

make route stats context as pure class #21765

Merged

ramaraochavali mentioned this pull request Jul 2, 2022

add stat prefix for routes istio/api#2405

Merged

izturn mentioned this pull request Jul 25, 2022

Contour support for Envoy's stats per route projectcontour/contour#4637

Open

		// This should be set for highly critical
		// endpoints that you wish to get “perfect” statistics on.

	// This emits statistics documentation at :ref:`virtual cluster statistics <config_http_filters_router_vcluster_stats>`.
	// This emitted statistics are the same as those documented at :ref:`virtual cluster statistics <config_http_filters_router_vcluster_stats>`.

		// every application endpoint. This is both not easily maintainable and as well the
		// statistics output is not free.

	// The statistics are rooted at vhost.<<virtual host name>.<stat_prefix>.
	// The statistics are rooted at vhost.<virtual host name>.<stat_prefix>.

		std::cout << "Setting retry policy...." << route_config.retry_policy().num_retries().value()
		<< "\n";

	// statistics use a non-trivial amount of memory(approximately 1k per route).
	// statistics use a non-trivial amount of memory (approximately 1KiB per route).

Conversation

ramaraochavali commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented May 16, 2022

Uh oh!

ramaraochavali commented May 16, 2022

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented May 25, 2022

Uh oh!

ramaraochavali commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented May 26, 2022

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Jun 15, 2022

Uh oh!

ramaraochavali commented Jun 16, 2022

Uh oh!

repokitteh-read-only bot commented Jun 16, 2022

Uh oh!

ramaraochavali commented Jun 16, 2022

Uh oh!

ramaraochavali commented May 16, 2022 •

edited

Loading

ramaraochavali commented May 25, 2022 •

edited

Loading