admin: order stats in clusters json admin #4306

ramaraochavali · 2018-08-30T15:07:10Z

Signed-off-by: Rama rama.rao@salesforce.com
Description: Currently host level stats in clusters proto uses map so they are outputted in random order. This PR changes it to list so that the order is predictable.
Risk Level: Low
Testing: Added automated tests
Docs Changes: N/A
Release Notes: N/A

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

ramaraochavali · 2018-08-30T16:17:50Z

api/envoy/admin/v2alpha/metrics.proto

@@ -11,9 +11,12 @@ message SimpleMetric {
    GAUGE = 1;
  }

-  // Type of metric represented.
-  Type type = 1;
+  // Name of the metric.


kept this in this order so that it comes in that order

I'm a bit confused here; are you saying the textual ordering in the proto spec dictates the wire format or JSON order? I would have thought the field numbers would dictate this.

Sorry. Some thing wrong. Kept the same order as earlier

ramaraochavali · 2018-08-30T16:32:01Z

@htuch removing map in clusters.proto. PTAL.

htuch

Thanks, looks good, would like @mrice32 to take a pass a well.

htuch · 2018-08-30T17:31:20Z

api/envoy/admin/v2alpha/metrics.proto

@@ -11,9 +11,12 @@ message SimpleMetric {
    GAUGE = 1;
  }

-  // Type of metric represented.
-  Type type = 1;
+  // Name of the metric.


I'm a bit confused here; are you saying the textual ordering in the proto spec dictates the wire format or JSON order? I would have thought the field numbers would dictate this.

mrice32

Thanks for working on this! A few comments and questions.

mrice32 · 2018-08-30T18:04:17Z

source/server/http/admin.cc

        for (const Stats::CounterSharedPtr& counter : host->counters()) {
-          auto& metric = (*host_status.mutable_stats())[counter->name()];
-          metric.set_type(envoy::admin::v2alpha::SimpleMetric::COUNTER);
+          auto& metric = *host_status.add_stats();


IIUC, the order that you iterate through these stats is dictated by their order in this unordered_map. In what situations would you like to have the ordering stay the same? I think your test could break in the future if the unordered_map internals (like its hash function) change. There are a few solutions here:

Change the std::unordered_map in the isolated store to an std::map everywhere or by specifying the data structure as a template parameter so it could be done in the Host object case only.

Sort the entries when exporting.

mrice32 · 2018-08-30T18:04:47Z

api/envoy/admin/v2alpha/clusters.proto

@@ -46,8 +46,8 @@ message HostStatus {
  // Address of this host.
  envoy.api.v2.core.Address address = 1;

-  // Mapping from the name of the statistic to the current value.
-  map<string, SimpleMetric> stats = 2;
+  // list of stats specific to this host.


nit: List (to match the surrounding comments)

mrice32 · 2018-08-30T18:11:17Z

source/server/http/admin.cc

@@ -314,7 +315,7 @@ void AdminImpl::writeClustersAsJson(Buffer::Instance& response) {
      }
    }
  }
-  response.add(MessageUtil::getJsonStringFromMessage(clusters, true)); // pretty-print
+  response.add(MessageUtil::getJsonStringFromMessage(clusters, true, true)); // pretty-print


Can you add a quick comment on this behavior change? I think if we want to do this, we should probably change it in the /config_dump json output, too.

Ok. I think then, I lean to revert this for consistency.

ggreenway · 2018-08-30T19:15:04Z

Why is it important for the stats to be output in a consistent order? It seems like it may be preferable to have the receiver of the stats sort them in whatever order it needs, because the order we make envoy send them in may still not be what the client needs, even if they're now predictable.

htuch · 2018-08-30T21:32:15Z

@ggreenway see #4122 for the earlier discussion. I'm in favor of this, since we transcode to JSON and humans sometimes look at this, I prefer it to be deterministic for a given state.

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

ramaraochavali · 2018-08-31T07:25:30Z

@mrice32 addressed the feedback. PTAL.

test/server/http/admin_test.cc

mrice32

LGTM!

test/server/http/admin_test.cc

htuch

Thanks!

* Fixed the pilot rbac build fail due to envoyproxy/envoy#4250 * Fixed the istioctl test fail due to envoyproxy/envoy#4306

* Fixed the pilot rbac build fail due to envoyproxy/envoy#4250 * Fixed the istioctl test fail due to envoyproxy/envoy#4306 Signed-off-by: Piotr Sikora <piotrsikora@google.com>

* Update Proxy SHA to latest with TCP proxy fixes. Pulling the following changes from github.com/istio/proxy: f498337 Fix a bug in origin authenticator that wrongly treats empty origin methods as pass (#1962) c352de0 Mixer Client: Add support for TCP local attributes (#1967) 2c563c6 remove not used path patcher functions (#1966) 490d26f Update Envoy SHA to latest with TCP proxy fixes. (#1964) 4cc4b7c Mixer Client uses Node metadata to populate Mixer attributes (#1961) cf23357 Fix the peerIsOptional and originIsOptional for authn filter. (#1959) cc6e58e support per-path JWT validation. (#1928) a5dd1aa mixer: clear route cache on header update (#1946) Pulling the following changes from github.com/envoyproxy/envoy: f936fc60f ssl: serialize accesses to SSL socket factory contexts (#4345) e34dcd62a Fix crash in tcp_proxy (#4323) ae6a25222 router: fix matching when all domains have wildcards (#4326) aa06142ff test: Stop fake_upstream methods from accidentally succeeding (#4232) 5d731878f rbac: update the authenticated.user to a StringMatcher. (#4250) c6bfc7d9a time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers (#4257) 752483ea9 Fixing the fix (#4333) 83487f6f3 tls: update BoringSSL to ab36a84b (3497). (#4338) 7bc210e02 test: fixing interactions between waitFor and ignore_spurious_events (#4309) 69474b398 admin: order stats in clusters json admin (#4306) 2d155f901 ppc64le build (#4183) 07efc6dc6 fix static initialization fiasco problem (#4314) 0b7e3b5e0 test: Remove declared but undefined class methods (#4297) 1485a1304 lua: make sure resetting dynamic metadata wrapper when request info is marked dead d243cd62e test: set to zero when start_time exceeds limit (#4328) 0a1e92acc test: fix heap use-after-free in ~IntegrationTestServer. (#4319) cddc732c7 CONTRIBUTING: Document 'kick-ci' trick. (#4335) f13ef2464 docs: remove reference to deprecated value field (#4322) e947a2766 router: minor doc fixes in stream idle timeout (#4329) 0c2e998af tcp-proxy: fixing a TCP proxy bug where we attempted to readDisable a closed connection (#4296) 00ffe44a2 utility: fix strftime overflow handling. (#4321) af1183c28 Re-enable TcpProxySslIntegrationTest and make the tests pass again. (#4318) 35534617b fuzz: fix H2 codec fuzzer post #4262. (#4311) 42f604853 Proto string issue fix (#4320) 9c492a01d Support Envoy to fetch secrets using SDS service. (#4256) a8572192f ratelimit: revert `revert rate limit failure mode config` and add tests (#4303) 1d34172bd dns: fix exception unsafe behavior in c-ares callbacks. (#4307) 121242340 alts: add gRPC TSI socket (#4153) f0363ae63 fuzz: detect client-side resets in H2 codec fuzzer. (#4300) 01aa3f820 test: hopefully deflaking echo integration test (#4304) 1fc0f4ba2 ratelimit: link legacy proto when message is being used (#4308) aa4481e6b fix rare List::remove(&target) segfault (#4244) 89e0f23ba headers: fixing fast fail of size-validation (#4269) 97eba5918 build: bump googletest version. (#4293) 0057e22d9 fuzz: avoid false positives in HCM fuzzer. (#4262) 9d094e590 Revert ac0bd74f6f9716e3a44d1412f795317c30ca770a (#4295) ddb28a4a1 Add validation context provider (#4264) 3b47cbabb added histogram latency information to Hystrix dashboard stream (#3986) cf87d50cd docs: update SNI FAQ. (#4285) f952033a4 config: fix update empty stat for eds (#4276) 329e591d3 router: Add ability of custom headers to rely on per-request data (#4219) 68d20b46c thrift: refactor build files and imports (#4271) 5fa8192a3 access_log: log requested_server_name in tcp proxy (#4144) fa45bb48f fuzz: libc++ clocks don't like nanos. (#4282) 53f8944f7 stats: add symbol table for future stat name encoding (#3927) c987b425b test infra: Remove timeSource() from the ClusterManager api (#4247) cd171d9a9 websocket: tunneling websockets (and upgrades in general) over H2 (#4188) b9dc5d9a0 router: disallow :path/host rewriting in request_headers_to_add. (#4220) 0c9101127 network: skip socket options and source address for UDS client connections (#4252) da1857d59 build: fixing a downstream compile error by noting explicit fallthrough (#4265) 9857cfe2a fuzz: cleanup per-test environment after each fuzz case. (#4253) 52beb067d test: Wrap proto string in std::string before comparison (#4238) f5e219edc extensions/thrift_proxy: Add header matching to thrift router (#4239) c9ce5d2b1 fuzz: track read_disable_count bidirectionally in codec_impl_fuzz_test. (#4260) 35103b353 fuzz: use nanoseconds for SystemTime in RequestInfo. (#4255) ba6ba9883 fuzz: make runtime root hermetic in server_fuzz_test. (#4258) b0a901480 time: Add 'format' test to ensure no one directly instantiates Prod*Time from source. (#4248) 85674603b access_log: support beginning of epoch in START_TIME. (#4254) 28d5f4118 proto: unify envoy_proto_library/api_proto_library. (#4233) f7d3cb638 http: fix allocation bug introduced in #4211. (#4245) Fixes #8310. Signed-off-by: Piotr Sikora <piotrsikora@google.com> * Update go-control-plane API and fix test fails * Fixed the pilot rbac build fail due to envoyproxy/envoy#4250 * Fixed the istioctl test fail due to envoyproxy/envoy#4306 Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Pulling the following changes from github.com/envoyproxy/envoy: f936fc6 ssl: serialize accesses to SSL socket factory contexts (envoyproxy#4345) e34dcd6 Fix crash in tcp_proxy (envoyproxy#4323) ae6a252 router: fix matching when all domains have wildcards (envoyproxy#4326) aa06142 test: Stop fake_upstream methods from accidentally succeeding (envoyproxy#4232) 5d73187 rbac: update the authenticated.user to a StringMatcher. (envoyproxy#4250) c6bfc7d time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers (envoyproxy#4257) 752483e Fixing the fix (envoyproxy#4333) 83487f6 tls: update BoringSSL to ab36a84b (3497). (envoyproxy#4338) 7bc210e test: fixing interactions between waitFor and ignore_spurious_events (envoyproxy#4309) 69474b3 admin: order stats in clusters json admin (envoyproxy#4306) 2d155f9 ppc64le build (envoyproxy#4183) 07efc6d fix static initialization fiasco problem (envoyproxy#4314) 0b7e3b5 test: Remove declared but undefined class methods (envoyproxy#4297) 1485a13 lua: make sure resetting dynamic metadata wrapper when request info is marked dead d243cd6 test: set to zero when start_time exceeds limit (envoyproxy#4328) 0a1e92a test: fix heap use-after-free in ~IntegrationTestServer. (envoyproxy#4319) cddc732 CONTRIBUTING: Document 'kick-ci' trick. (envoyproxy#4335) f13ef24 docs: remove reference to deprecated value field (envoyproxy#4322) e947a27 router: minor doc fixes in stream idle timeout (envoyproxy#4329) 0c2e998 tcp-proxy: fixing a TCP proxy bug where we attempted to readDisable a closed connection (envoyproxy#4296) 00ffe44 utility: fix strftime overflow handling. (envoyproxy#4321) af1183c Re-enable TcpProxySslIntegrationTest and make the tests pass again. (envoyproxy#4318) 3553461 fuzz: fix H2 codec fuzzer post envoyproxy#4262. (envoyproxy#4311) 42f6048 Proto string issue fix (envoyproxy#4320) 9c492a0 Support Envoy to fetch secrets using SDS service. (envoyproxy#4256) a857219 ratelimit: revert `revert rate limit failure mode config` and add tests (envoyproxy#4303) 1d34172 dns: fix exception unsafe behavior in c-ares callbacks. (envoyproxy#4307) 1212423 alts: add gRPC TSI socket (envoyproxy#4153) f0363ae fuzz: detect client-side resets in H2 codec fuzzer. (envoyproxy#4300) 01aa3f8 test: hopefully deflaking echo integration test (envoyproxy#4304) 1fc0f4b ratelimit: link legacy proto when message is being used (envoyproxy#4308) aa4481e fix rare List::remove(&target) segfault (envoyproxy#4244) 89e0f23 headers: fixing fast fail of size-validation (envoyproxy#4269) 97eba59 build: bump googletest version. (envoyproxy#4293) 0057e22 fuzz: avoid false positives in HCM fuzzer. (envoyproxy#4262) 9d094e5 Revert ac0bd74 (envoyproxy#4295) ddb28a4 Add validation context provider (envoyproxy#4264) 3b47cba added histogram latency information to Hystrix dashboard stream (envoyproxy#3986) cf87d50 docs: update SNI FAQ. (envoyproxy#4285) f952033 config: fix update empty stat for eds (envoyproxy#4276) 329e591 router: Add ability of custom headers to rely on per-request data (envoyproxy#4219) 68d20b4 thrift: refactor build files and imports (envoyproxy#4271) 5fa8192 access_log: log requested_server_name in tcp proxy (envoyproxy#4144) fa45bb4 fuzz: libc++ clocks don't like nanos. (envoyproxy#4282) 53f8944 stats: add symbol table for future stat name encoding (envoyproxy#3927) c987b42 test infra: Remove timeSource() from the ClusterManager api (envoyproxy#4247) cd171d9 websocket: tunneling websockets (and upgrades in general) over H2 (envoyproxy#4188) b9dc5d9 router: disallow :path/host rewriting in request_headers_to_add. (envoyproxy#4220) 0c91011 network: skip socket options and source address for UDS client connections (envoyproxy#4252) da1857d build: fixing a downstream compile error by noting explicit fallthrough (envoyproxy#4265) 9857cfe fuzz: cleanup per-test environment after each fuzz case. (envoyproxy#4253) 52beb06 test: Wrap proto string in std::string before comparison (envoyproxy#4238) f5e219e extensions/thrift_proxy: Add header matching to thrift router (envoyproxy#4239) c9ce5d2 fuzz: track read_disable_count bidirectionally in codec_impl_fuzz_test. (envoyproxy#4260) 35103b3 fuzz: use nanoseconds for SystemTime in RequestInfo. (envoyproxy#4255) ba6ba98 fuzz: make runtime root hermetic in server_fuzz_test. (envoyproxy#4258) b0a9014 time: Add 'format' test to ensure no one directly instantiates Prod*Time from source. (envoyproxy#4248) 8567460 access_log: support beginning of epoch in START_TIME. (envoyproxy#4254) 28d5f41 proto: unify envoy_proto_library/api_proto_library. (envoyproxy#4233) f7d3cb6 http: fix allocation bug introduced in envoyproxy#4211. (envoyproxy#4245) Fixes istio/istio#8310 (once pulled into istio/istio). Signed-off-by: Piotr Sikora <piotrsikora@google.com>

ramaraochavali added 2 commits August 30, 2018 20:26

order stats in clusters json admin

9e75142

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

kick ci

2ed1e3d

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

ramaraochavali commented Aug 30, 2018

View reviewed changes

htuch reviewed Aug 30, 2018

View reviewed changes

htuch requested a review from mrice32 August 30, 2018 17:32

htuch assigned htuch and mrice32 Aug 30, 2018

mrice32 reviewed Aug 30, 2018

View reviewed changes

ramaraochavali added 2 commits August 31, 2018 09:29

address review feedback

a84b193

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

fix tests

f12352c

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

mrice32 reviewed Aug 31, 2018

View reviewed changes

test/server/http/admin_test.cc Show resolved Hide resolved

mrice32 approved these changes Sep 4, 2018

View reviewed changes

test/server/http/admin_test.cc Show resolved Hide resolved

htuch approved these changes Sep 4, 2018

View reviewed changes

htuch merged commit 69474b3 into envoyproxy:master Sep 4, 2018

ramaraochavali deleted the fix/cluster_stats_order branch September 4, 2018 22:40

yangminzhu added a commit to yangminzhu/istio that referenced this pull request Sep 13, 2018

Update go-control-plane API and fix test fails

d009ee8

* Fixed the pilot rbac build fail due to envoyproxy/envoy#4250 * Fixed the istioctl test fail due to envoyproxy/envoy#4306

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

admin: order stats in clusters json admin #4306

admin: order stats in clusters json admin #4306

ramaraochavali commented Aug 30, 2018

ramaraochavali Aug 30, 2018

htuch Aug 30, 2018

ramaraochavali Aug 31, 2018

ramaraochavali commented Aug 30, 2018

htuch left a comment

htuch Aug 30, 2018

mrice32 left a comment

mrice32 Aug 30, 2018

mrice32 Aug 30, 2018

mrice32 Aug 30, 2018

ramaraochavali Aug 31, 2018

ggreenway commented Aug 30, 2018

htuch commented Aug 30, 2018

ramaraochavali commented Aug 31, 2018

mrice32 left a comment

htuch left a comment

admin: order stats in clusters json admin #4306

admin: order stats in clusters json admin #4306

Conversation

ramaraochavali commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Aug 30, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreenway commented Aug 30, 2018

htuch commented Aug 30, 2018

ramaraochavali commented Aug 31, 2018

mrice32 left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment