stats: json optimization of histograms #3280

ramaraochavali · 2018-05-03T16:08:07Z

Signed-off-by: Rama rama.rao@salesforce.com
Description:
optimizes the json representation of histograms by sending all supported_quantile once and computed_quantlie array multiple times.

Risk Level: Low
Testing: Manual
Docs Changes: NA
Release Notes: NA

Fixes #3102
Fixes #3210

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

ramaraochavali · 2018-05-03T16:09:51Z

@mattklein123 @jmarantz @mrice32
I screwed up the original PR #3184 trying to correct DCO. Sorry.
Here is the new PR.
Addressed all the comments in the previous PR and added docs.
PTAL.

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

mrice32

Mostly looks good. Just a few small comments on the docs.

mrice32 · 2018-05-03T20:41:13Z

docs/root/operations/admin.rst

+        },
+        {
+            "histograms": {
+                "supported_quantiles": [...],


Could you add real values here, so the user knows what the array will look like (as in, is it an array of doubles or subobjects)?

mrice32 · 2018-05-03T20:44:02Z

docs/root/operations/admin.rst

+                        "name": "example.histogram",
+                        "values": [
+                            {
+                                "interval": null,


As above, it would be nice to see an example of a histogram with non-null quantiles values since that will be the common case. But it'd also be nice to keep some sort of null example so the user knows what a no-data hist will look like.

Side note: will this ever print histograms that have no samples in both cumulative and internal since that would mean its used() method would return false?

I will make those changes. and regarding used, it currently prints all histograms. And I think it is a good point, it would be good filter the unused ones. I will make that change as well.

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

mattklein123

LGTM if it passes @mrice32 @jmarantz muster.

ramaraochavali · 2018-05-04T08:42:12Z

release test is failing with server_fuzz_test unrelated to this.

mattklein123 · 2018-05-04T08:45:10Z

@ramaraochavali try merging master, @htuch put in a fix that might help.

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

ramaraochavali · 2018-05-04T11:45:48Z

source/common/stats/thread_local_store.cc

  if (!shutting_down_) {
+    ASSERT(!merge_in_progress_);


i just moved this check because, I hit the same issue in CI. Since #3289 fixes it properly we should be good.

It'd be great to get that PR in first to avoid affecting this one :)

jmarantz · 2018-05-04T12:48:26Z

docs/root/operations/admin.rst

+
+  {
+    "histograms": {
+    "supported_quantiles": [


the indentation in this description looks wrong. This line should be indented one more level than the previous one. Do you think it also makes sense, doc purpose, to put these 9 quantiles each on one line?

jmarantz · 2018-05-04T12:51:01Z

docs/root/operations/admin.rst

+        "name": "cluster.external_auth_cluster.upstream_cx_length_ms",
+        "values": [
+          {
+            "interval": 0,


for doc legibility purposes, consider this format:

"values": [ {"interval": 0, "cumulative": 0}, {"interval": 0, "cumulative": 0}, {"interval": 1.0435787, "cumulative": 1.0435787}, ....

I assume the whitespace output is not actually what the json serializer actually emits, right? It would omit all whitespace?

jmarantz · 2018-05-04T12:51:50Z

docs/root/operations/admin.rst

+        ]
+      }
+    ]
+   }


I think the missing indentation earlier forces a few lines of 1-char indent. All this is hard to see because of all the newlines above.

jmarantz · 2018-05-04T12:53:08Z

test/integration/integration_admin_test.cc

+  for (Json::ObjectSharedPtr obj_ptr : statsjson->getObjectArray("stats")) {
+    if (obj_ptr->hasObject("histograms")) {
+      histogram_count++;
+      const Json::ObjectSharedPtr& histograms_ptr = obj_ptr->getObject("histograms");


I'm confused here: you are looking for 'histograms' plural so I was expecting another loop over all the histograms. Can you explain?

There is a toplevel "histograms" element that has supported_quantiles array and computed_quantiels, which is again an array of actual histogram computed values in the form of "name" and "values" (interval and cumulative) - the last array is repeated as many times as there are histograms"

I'm still not entirely clear on the cardinality of variouis things from looking at the code. If we actually add a second histogram and make the test expect 2?

no. it is still 1. The computed_quantiles array size would be equivalent to the number of histograms.
Cardinality of histograms : 1
Cardinality of supported_quantile array : 1
Cardinality if computed_quantiles array = number of histograms (with each element having histogram name and values array which has interval and cumulative values)

Can you please add a second histogram to this test, so computed_quantiles array has length 2?

I am not adding any histograms in the test. I just grab whatever the output is from /stats?format=json and validate that it has histograms, it would definitely some (but non predictable). I am not setting up any thing for this test.

Ah, very cool. Let me rephrase my request. Can you make a new test where you add some specific histogram data with more than one histogram, and check the JSON output?

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

jmarantz · 2018-05-04T13:56:44Z

docs/root/operations/admin.rst

-    ]
-   }
+      "supported_quantiles": [
+        0,25,50,75,90,95,99,99.9,100


nit: can you put a single space after each comma?

jmarantz · 2018-05-04T13:57:21Z

docs/root/operations/admin.rst

+      ],
+      "computed_quantiles": [
+       {
+          "name": "cluster.external_auth_cluster.upstream_cx_length_ms",


make this 2-space-per-nesting indent consistently; here you have 3.

It is two only. I checked by enabling white space. Github is showing like that.

jmarantz · 2018-05-04T13:59:38Z

test/integration/integration_admin_test.cc

+
+      const std::vector<Json::ObjectSharedPtr>& computed_quantiles =
+          histograms_ptr->getObjectArray("computed_quantiles");
+      EXPECT_GT(computed_quantiles.size(), 0);


can we make this EXPECT_EQ something specific here?

this count would be equivalent to number of histograms. I do not know exactly how many will come that is why I have added this GT zero condition.

Isn't the number of histograms predictable in this test?

jmarantz · 2018-05-04T14:10:10Z

test/integration/integration_admin_test.cc

+  for (Json::ObjectSharedPtr obj_ptr : statsjson->getObjectArray("stats")) {
+    if (obj_ptr->hasObject("histograms")) {
+      histogram_count++;
+      const Json::ObjectSharedPtr& histograms_ptr = obj_ptr->getObject("histograms");


I'm still not entirely clear on the cardinality of variouis things from looking at the code. If we actually add a second histogram and make the test expect 2?

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

jmarantz · 2018-05-04T14:35:33Z

docs/root/operations/admin.rst

+      ],
+      "computed_quantiles": [
+       {
+          "name": "cluster.external_auth_cluster.upstream_cx_length_ms",


You have this:

+ { + "name": "cluster.external_auth_cluster.upstream_cx_length_ms",

I think it should be this:

+ { + "name": "cluster.external_auth_cluster.upstream_cx_length_ms",

jmarantz · 2018-05-04T14:35:53Z

source/common/stats/thread_local_store.cc

  if (!shutting_down_) {
+    ASSERT(!merge_in_progress_);


It'd be great to get that PR in first to avoid affecting this one :)

jmarantz · 2018-05-04T14:37:58Z

test/integration/integration_admin_test.cc

+
+      const std::vector<Json::ObjectSharedPtr>& computed_quantiles =
+          histograms_ptr->getObjectArray("computed_quantiles");
+      EXPECT_GT(computed_quantiles.size(), 0);


Isn't the number of histograms predictable in this test?

jmarantz · 2018-05-04T14:38:12Z

test/integration/integration_admin_test.cc

+  for (Json::ObjectSharedPtr obj_ptr : statsjson->getObjectArray("stats")) {
+    if (obj_ptr->hasObject("histograms")) {
+      histogram_count++;
+      const Json::ObjectSharedPtr& histograms_ptr = obj_ptr->getObject("histograms");


Can you please add a second histogram to this test, so computed_quantiles array has length 2?

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

ramaraochavali · 2018-05-04T15:04:06Z

@jmarantz since that is a private method used only at this place, I thought this test is sufficient. I am not sure if we can add tests for private methods in the current test infrastructure. Will have to check. If you think it adds value, I will do it next week

jmarantz · 2018-05-04T15:08:03Z

you can structure the test exactly as it is here, if you like, via the admin interface, if that's easier; I just think it's reasonable to control the data. It's also possible to test private methods by friending the test-class and putting a helper there, or adding a TestingPeer class which is friended, if that helps unit-tests.

mrice32

LGTM once @jmarantz's comments are resolved.

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

ramaraochavali · 2018-05-05T04:29:01Z

@jmarantz i will try adding the test next week. I did not get how I can structure the test with admin interface though. How will clear the existing histograms/add the histograms I want in the admin test - i do not have access to stats_store I guess. I have not looked at the code. Are you suggesting some thing different? May not getting your suggestion...

jmarantz · 2018-05-06T21:15:32Z

RE test for >1 histogram: I would actually prefer you add it as a unit test anyway, which sounds easier. The 'private' issue is easily dealt with by either friending the test-class or a TestingPeer class, either of which can act as in intermediary to access a private method.

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

ramaraochavali · 2018-05-08T13:03:03Z

test/server/http/admin_test.cc

+ * This is a heap test allocator that works similar to how the shared memory allocator works in
+ * terms of reference counting, etc.
+ */
+class TestAllocator : public Stats::RawStatDataAllocator {


@jmarantz added a unit test that verifies the json format. PTAL. I had to copy this TestAllocator class as is from thread_local_store_test.cc as I found that it is the easiest way to generate histograms with stores. I do not know if there is any way to make this class across both the files, with out changing much..

Can you move the TestAllocator class to test/test_common/utility.h so you don't have duplicated code?

jmarantz · 2018-05-10T01:52:01Z

docs/root/operations/admin.rst

+            {"interval": null, "cumulative": 3.0665233},
+            {"interval": null, "cumulative": 6.046609},
+            {"interval": null, "cumulative": 229.57333},
+            {"interval": null, "cumulative": 260}


This looks great now. Much easier to read. Thanks!

jmarantz · 2018-05-10T02:03:54Z

test/server/http/admin_test.cc

+
+  std::string actual_json = statsAsJsonHandler(all_stats, store_->histograms());
+
+  const std::string expected_json = R"EOF({


This reminds me; do we always generate pretty-printed JSON? I think it would be better to deliver it compact in production, though it's nice to have the test be pretty-printed for test.cc legibility.

Yes. Currently we do generate pretty-printed JSON. Now I have added a parameter to control that and defaulted it to false and changed the test to print in pretty form. PTAL.

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

jmarantz

Great, thanks for this! This test adds confidence for me (even though it might not have affected line-coverage).

jmarantz · 2018-05-10T16:20:34Z

test/test_common/utility.h

+      return;
+    }
+
+    for (auto i = stats_.begin(); i != stats_.end(); i++) {


This was pre-existing, but you have a map here, can't we just remove it via the name? I don't really care about perf for the test but I think the code would just be:

if (stats_.erase(std::string(data.name()) == 0) { FAIL(); }

which is fewer lines :)

jmarantz · 2018-05-10T16:32:42Z

source/server/http/admin.cc

+        }
+        quantile_obj.AddMember("interval", interval_value, allocator);
+        Value cumulative_value;
+        if (!std::isnan(histogram->cumulativeStatistics().computedQuantiles()[i])) {


Probably worth commenting here that as we are positionally associating this entry with a quantile, so we need to put in the {null, null} entry to keep other data aligned.

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

ramaraochavali · 2018-05-10T18:30:45Z

@jmarantz addressed the two comments. Should be good now.
@mattklein123 I think we are good here. Can you PTAL ?

json optimization

5c3d11b

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

docs

636e670

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

ggreenway assigned jmarantz, mattklein123 and mrice32 May 3, 2018

mrice32 reviewed May 3, 2018

View reviewed changes

ramaraochavali added 3 commits May 4, 2018 09:21

updated doc, introduced used check

6243853

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

formatted

9c89ab4

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

formatted the doc

776d307

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

mattklein123 previously approved these changes May 4, 2018

View reviewed changes

ramaraochavali added 2 commits May 4, 2018 16:02

Merge branch 'master' into fix/json_optimization

b829018

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

move ASSERT inside

dbe7610

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

ramaraochavali dismissed mattklein123’s stale review via dbe7610 May 4, 2018 11:40

ramaraochavali commented May 4, 2018

View reviewed changes

jmarantz reviewed May 4, 2018

View reviewed changes

corrected indentation

7c68d2a

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

jmarantz reviewed May 4, 2018

View reviewed changes

add space in supported_quantlies

81f8da7

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

jmarantz reviewed May 4, 2018

View reviewed changes

correct indentation

e7ca172

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

mrice32 approved these changes May 4, 2018

View reviewed changes

fix indentation

e86c4c8

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

added test for histogram json

de78873

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

formatted

6ca2748

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

ramaraochavali commented May 8, 2018

View reviewed changes

jmarantz reviewed May 10, 2018

View reviewed changes

ramaraochavali added 3 commits May 10, 2018 06:16

added pretty_print optional

98dd266

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

build formatted

10dc326

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

Merge branch 'master' into fix/json_optimization

3e3bdaa

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

jmarantz approved these changes May 10, 2018

View reviewed changes

fixed minor comments

ae5dfbd

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

jmarantz approved these changes May 10, 2018

View reviewed changes

mattklein123 approved these changes May 10, 2018

View reviewed changes

mattklein123 merged commit 64d50ab into envoyproxy:master May 10, 2018


		std::string actual_json = statsAsJsonHandler(all_stats, store_->histograms());

		const std::string expected_json = R"EOF({

stats: json optimization of histograms #3280

stats: json optimization of histograms #3280

Conversation

ramaraochavali commented May 3, 2018 • edited by mattklein123 Loading

ramaraochavali commented May 3, 2018

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

mattklein123 left a comment

Choose a reason for hiding this comment

ramaraochavali commented May 4, 2018

mattklein123 commented May 4, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali May 4, 2018 • edited Loading

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

jmarantz May 4, 2018 • edited Loading

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

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

Choose a reason for hiding this comment

ramaraochavali commented May 4, 2018

jmarantz commented May 4, 2018

mrice32 left a comment

Choose a reason for hiding this comment

ramaraochavali commented May 5, 2018

jmarantz commented May 6, 2018

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

Choose a reason for hiding this comment

jmarantz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented May 10, 2018

ramaraochavali commented May 3, 2018 •

edited by mattklein123

Loading

ramaraochavali May 4, 2018 •

edited

Loading

jmarantz May 4, 2018 •

edited

Loading