stats: optimize json representation of histograms

[ramaraochavali]

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

[ramaraochavali]

Here is how the new optimized format will look like

{
            "supported_quanlties": [
                0.0,
                25.0,
                50.0,
                75.0,
                90.0,
                95.0,
                99.0,
                99.9,
                100.0
            ],
            "histograms": [
                {
                    "name": "http.egress_http2.downstream_rq_time",
                    "computed_quantiles": [
                        {
                            "interval": 0.0,
                            "cumulative": 0.0
                        },
                        {
                            "interval": 0.0,
                            "cumulative": 0.0
                        },
                        {
                            "interval": 1.043578738857709,
                            "cumulative": 1.043578738857709
                        },
                        {
                            "interval": 1.0941564872895345,
                            "cumulative": 1.0941564872895345
                        },
                        {
                            "interval": 2.086002317497103,
                            "cumulative": 2.086002317497103
                        },
                        {
                            "interval": 3.066523297491039,
                            "cumulative": 3.066523297491039
                        },
                        {
                            "interval": 6.046608695652175,
                            "cumulative": 6.046608695652175
                        },
                        {
                            "interval": 229.57333333333436,
                            "cumulative": 229.57333333333436
                        },
                        {
                            "interval": 260.0,
                            "cumulative": 260.0
                        }
                    ]
                },
            ]
}

[ramaraochavali]

@jmarantz @mattklein123 one of the TODOs in histogram PR.

[mattklein123]

Very nice. Is there a test that at least checks that this is valid JSON? Better would be a test that sanity checks some of the output?

[jmarantz]

nice. Would it also be worthwhile to have computed_quantiles be a simple array of 2-element arrays, to avoid the interleaved repeated keywords? Of course if the json is gzipped it's not going to matter much.

It's also worth explicitly discussing what happens when some of the underlying data is NaN, no?

[ramaraochavali]

@mattklein123 Yes. There is a test that validates whether this a valid json. Do you think any other test is needed?

@jmarantz I think leaving it with interleaved keywords is fine because it would be readable. As you mentioned, it is not going to matter much. When underlying data is Nan(that means that histogram is not used in that interval), we set it to null - I think that is fine.

[jmarantz]

@ramaraochavali It's a pretty big size difference if not gzipped. I'm not sure whether in the flows that consume this, gzip is commonly in the path. If not, then this change adds readability, but makes the output much more verbose, so it's a significant tradeoff. If we are certain gzip is in the flow then I think it's a reasonable idea. Can you confirm that's the case?

I thought any individual value can be NaN, e.g. the summary and not the interval or vice-versa. What happens then?

[ramaraochavali]

@jmarantz agree on the size different if not gzipped. I am not sure about whether gzip filer configured commonly here. @mattklein123 might have details. If it is not there, I am ok, to reduce it to simple arrays excluding the keywords. We can document the pattern similar to how we have done for admin end point.

regarding Nan, the code that outputs to json ensures that it does not set that value if it is Nan so would automatically be null.

[jmarantz]

for (double quantile : empty_statistics.supportedQuantiles()) {
....

[jmarantz]

s/quanlties/quantiles/

[mattklein123]

@jmarantz agree on the size different if not gzipped. I am not sure about whether gzip filer configured commonly here. @mattklein123 might have details. If it is not there, I am ok, to reduce it to simple arrays excluding the keywords. We can document the pattern similar to how we have done for admin end point.

There is no gzip support right now for the admin endpoint (though the same loopback hack that we have done for security in some cases could be used for gzip also, so maybe docs is all that is needed). With that said, given the code @gsagula has written it would be not hard to add. I think we should do this, especially for Prometheus output.

I'm fine either way on this. Will defer to @jmarantz on final approval.

[mrice32]

@ramaraochavali this looks solid! Awesome stuff! Are there any tests that sanity check the output of this method, so we know that it output the specific json we expect? If not do you think it makes sense to add a simple one?

[jmarantz]

OK let's go with the format as described. Tests are the next step I think.

[jmarantz]

from an API perspective, the supported quantiles can in theory be overridden per-histogram, though the code doesn't support that now. Do you want to just put a warning check below as you loop through the histograms, that the quantiles match and at least print a warning if they don't? I think that'll never fire, though, so test-coverage would be annoying.

[ramaraochavali]

I have added a validation in tests that validates supported quantiles and computed quantiles are of the same size. I think that should be fine for now.

[ramaraochavali]

@mrice32 there is a test that validates whether this JSON is valid or not. I have extended and added additional validations to ensure the new histogram structure is what we want. PTAL.

[ramaraochavali]

Here is the new format. I thought this more structured than previous one. The main change is "histograms" come as the top level container that has supported_quantiles, computed_quantlies. The computed_quantiles is an array of histograms with name and values (with interval and cumulative values).

{
            "histograms": {
                "supported_quantiles": [
                    0.0,
                    25.0,
                    50.0,
                    75.0,
                    90.0,
                    95.0,
                    99.0,
                    99.9,
                    100.0
                ],
                "computed_quantiles": [
                    {
                        "name": "cluster.external_auth_cluster.upstream_cx_length_ms",
                        "values": [
                            {
                            "interval": 0.0,
                            "cumulative": 0.0
                        },
                        {
                            "interval": 0.0,
                            "cumulative": 0.0
                        },
                        {
                            "interval": 1.043578738857709,
                            "cumulative": 1.043578738857709
                        },
                        {
                            "interval": 1.0941564872895345,
                            "cumulative": 1.0941564872895345
                        },
                        {
                            "interval": 2.086002317497103,
                            "cumulative": 2.086002317497103
                        },
                        {
                            "interval": 3.066523297491039,
                            "cumulative": 3.066523297491039
                        },
                        {
                            "interval": 6.046608695652175,
                            "cumulative": 6.046608695652175
                        },
                        {
                            "interval": 229.57333333333436,
                            "cumulative": 229.57333333333436
                        },
                        {
                            "interval": 260.0,
                            "cumulative": 260.0
                        }
                        ]
                    },
            },
}

[jmarantz]

This looks good. +1 for having a test with some small amount of data where actually EXPECT_EQ on the stringified json.

More importantly, we should doc the json format, right?

[jmarantz]

I'm curious why you made this a bool as opposed to a count that you increment in the loop. I wouldn't expect this to be flaky between tests.

[mrice32]

+1

[ramaraochavali]

I made it to bool because, we will only have one "histograms" top level object in the JSON which has supported_quantlies (one) and an array of computed_quantiles for each histogram. I may be missing some thing here and not able to follow your suggestion about counting.

[jmarantz]

That means you could successfully pass the test even if the underlying impl matched an extra histogram that it shouldn't have. I'd err on the side of checking more precisely, especially as it's no extra lines of code.

[jmarantz]

It's worth noting in a comment here that an implementation design makes it impossible for the supported quantiles to differ across histograms, although the interface in include/envoy/stats/stats.h, ParentHistogram, makes it look like they can.

[mrice32]

Test looks good to me outside of the issue @jmarantz pointed out. LGTM after addressing that.

[mrice32]

+1

[mattklein123]

Obviously we will hold on this until we sort out the underlying issues in #3235