xDS: add version gauge by junr03 · Pull Request #1805 · envoyproxy/envoy

junr03 · 2017-10-04T00:05:31Z

Surface the version_info field in the xDS subscriptions as a gauge in the xDS stats trees.

Signed-off-by: Jose Nino jnino@lyft.com

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2017-10-04T00:06:36Z

danielhochman · 2017-10-04T00:08:46Z

source/common/router/rds_subscription.cc

  resources[0].set_name(route_config_name_);
  callbacks_->onConfigUpdate(resources);
  version_info_ = Envoy::Config::Utility::computeHashedVersion(response_body);
+  stats_.version_.set(HashUtil::xxHash64(version_info_));


i would just hash the response body again here

Please avoid computing the hash twice. Just return a tuple or something.

But +1 for using same hash.

mattklein123 · 2017-10-04T00:13:35Z

docs/configuration/http_conn_man/rds.rst

 ----------

-RDS has a statistics tree rooted at *http.<stat_prefix>.rds.* with the following statistics:
+RDS has a statistics tree rooted at *http.<stat_prefix>.rds.<route_config_name>* with the following statistics:


*http.<stat_prefix>.rds.<route_config_name>.*

mattklein123 · 2017-10-04T15:13:46Z

Per #1806 please put the RDS per table stats into a dedicated scope, so that any naming fixes are automatically picked up. cc @hennna

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2017-10-04T17:55:58Z

@danielhochman @htuch @mattklein123 ready for another pass.

danielhochman · 2017-10-04T18:02:49Z

source/common/config/http_subscription_impl.h

  }

  std::string path_;
+  std::string version_info_;


why add this local var?

mattklein123 · 2017-10-04T18:04:06Z

source/common/router/rds_impl.cc

+std::string RdsRouteConfigProviderImpl::rdsStatsName(const std::string& stat_prefix,
+                                                     const std::string& route_config_name) {
+  std::string stats_name = fmt::format("{}rds.{}.", stat_prefix, route_config_name);
+  std::replace(stats_name.begin(), stats_name.end(), ':', '_');


Do you have a test case for this? cc @hennna please update this site also in your other PR if we move this into scope.

I used all the stats expectations as a test that the name is correct. But I will make an explicit test.

I've updated #1806. Let me know if I'm missing anything else.

will remove this in favor of @hennna's change.

htuch · 2017-10-04T18:53:31Z

docs/configuration/http_conn_man/rds.rst


-RDS has a statistics tree rooted at *http.<stat_prefix>.rds.* with the following statistics:
+RDS has a statistics tree rooted at *http.<stat_prefix>.rds.<route_config_name>.*.
+Any ``:`` character in the ``route_config_name`` name gets replaces with ``_`` in the


Nit: s/replaces/replaced/

htuch · 2017-10-04T18:54:13Z

docs/configuration/cluster_manager/cds.rst

  update_attempt, Counter, Total API fetches attempted
  update_success, Counter, Total API fetches completed successfully
  update_failure, Counter, Total API fetches that failed (either network or schema errors)
+  version, Gauge, Hash of the contents from the last successful API fetch


Don't we also want to report the actual version_info as provided by v2 xDS?

I don't think we can provide that via a gauge, given that version_info is a string.

we could attempt atoul on the first 6 chars similar to the server's version info and fallback to hashing. i'm not sure that it's worth it though.

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2017-10-04T20:43:04Z

@danielhochman updated

ccaraman · 2017-10-04T20:49:46Z

include/envoy/config/subscription.h

 */
 // clang-format off
-#define ALL_SUBSCRIPTION_STATS(COUNTER) \
+#define ALL_SUBSCRIPTION_STATS(COUNTER, GAUGE) \


nit: align the spacing of \

hmm I thought fix_format would take care of this.

ccaraman · 2017-10-04T20:50:09Z

include/envoy/upstream/upstream.h

  COUNTER(update_success)                                                                          \
  COUNTER(update_failure)                                                                          \
-  COUNTER(update_empty)
+  COUNTER(update_empty) \


nit: fix spacing of \

Signed-off-by: Jose Nino <jnino@lyft.com>

Description: expose the new per try idle timeout via the engine builder. Risk Level: low Testing: unit test Docs Changes: added Signed-off-by: Jose Nino <jnino@lyft.com> Co-authored-by: Alan Chiu <achiu@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

…ions group (#1805)

xDS: add version gauge

910a531

Signed-off-by: Jose Nino <jnino@lyft.com>

danielhochman reviewed Oct 4, 2017

View reviewed changes

mattklein123 reviewed Oct 4, 2017

View reviewed changes

junr03 mentioned this pull request Oct 4, 2017

Replace ':' with '_' in stats name. #1806

Merged

update

79e2908

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 force-pushed the version-gauge branch from 8377cc9 to 79e2908 Compare October 4, 2017 17:27

comment

9fa33d3

Signed-off-by: Jose Nino <jnino@lyft.com>

danielhochman reviewed Oct 4, 2017

View reviewed changes

mattklein123 reviewed Oct 4, 2017

View reviewed changes

htuch reviewed Oct 4, 2017

View reviewed changes

Jose Nino added 2 commits October 4, 2017 12:21

comments

cb8e2fc

Signed-off-by: Jose Nino <jnino@lyft.com>

Merge branch 'master' into version-gauge

4a4aa75

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 force-pushed the version-gauge branch from 8a07ce2 to 4a4aa75 Compare October 4, 2017 20:42

ccaraman reviewed Oct 4, 2017

View reviewed changes

nits

a34c5e9

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123 approved these changes Oct 4, 2017

View reviewed changes

junr03 merged commit cff1866 into master Oct 4, 2017

junr03 deleted the version-gauge branch October 4, 2017 21:45

mattklein123 mentioned this pull request Oct 4, 2017

Add stronger hash or comparison to xDS APIs #1452

Closed

htuch mentioned this pull request Dec 7, 2017

True xDS version in admin interface #2172

Closed

mathetake pushed a commit that referenced this pull request Mar 3, 2026

chore(deps): bump actions/cache from 5.0.1 to 5.0.2 in the github-act…

1b4e466

…ions group (#1805)

Conversation

junr03 commented Oct 4, 2017

Uh oh!

junr03 commented Oct 4, 2017

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 commented Oct 4, 2017

Uh oh!

junr03 commented Oct 4, 2017

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!

junr03 Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 commented Oct 4, 2017

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

junr03 Oct 4, 2017 •

edited

Loading