Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stats: json optimization of histograms #3280

Merged
merged 17 commits into from
May 10, 2018
Merged

stats: json optimization of histograms #3280

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
5c3d11b
json optimization
ramaraochavali May 3, 2018
636e670
docs
ramaraochavali May 3, 2018
6243853
updated doc, introduced used check
ramaraochavali May 4, 2018
9c89ab4
formatted
ramaraochavali May 4, 2018
776d307
formatted the doc
ramaraochavali May 4, 2018
b829018
Merge branch 'master' into fix/json_optimization
ramaraochavali May 4, 2018
dbe7610
move ASSERT inside
ramaraochavali May 4, 2018
7c68d2a
corrected indentation
ramaraochavali May 4, 2018
81f8da7
add space in supported_quantlies
ramaraochavali May 4, 2018
e7ca172
correct indentation
ramaraochavali May 4, 2018
e86c4c8
fix indentation
ramaraochavali May 5, 2018
de78873
added test for histogram json
ramaraochavali May 8, 2018
6ca2748
formatted
ramaraochavali May 8, 2018
98dd266
added pretty_print optional
ramaraochavali May 10, 2018
10dc326
build formatted
ramaraochavali May 10, 2018
3e3bdaa
Merge branch 'master' into fix/json_optimization
ramaraochavali May 10, 2018
ae5dfbd
fixed minor comments
ramaraochavali May 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 111 additions & 2 deletions docs/root/operations/admin.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,118 @@ The fields are:

.. http:get:: /stats?format=json

Outputs /stats in JSON format. This can be used for programmatic access of stats.
Outputs /stats in JSON format. This can be used for programmatic access of stats. Counters and Gauges
will be in the form of a set of (name,value) pairs. Histograms will be under the element "histograms",
that contains "supported_quantiles" which lists the quantiles supported and an array of computed_quantiles
that has the computed quantile for each histogram. Only histograms with recorded values will be exported.

.. http:get:: /stats?format=prometheus
If a histogram is not updated during an interval, the ouput will have null for all the quantiles.

Example histogram output:

.. code-block:: json

{
"histograms": {
"supported_quantiles": [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

0,
25,
50,
75,
90,
95,
99,
99.9,
100
],
"computed_quantiles": [
{
"name": "cluster.external_auth_cluster.upstream_cx_length_ms",
"values": [
{
"interval": 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

"cumulative": 0
},
{
"interval": 0,
"cumulative": 0
},
{
"interval": 1.0435787,
"cumulative": 1.0435787
},
{
"interval": 1.0941565,
"cumulative": 1.0941565
},
{
"interval": 2.0860023,
"cumulative": 2.0860023
},
{
"interval": 3.0665233,
"cumulative": 3.0665233
},
{
"interval": 6.046609,
"cumulative": 6.046609
},
{
"interval": 229.57333,
"cumulative": 229.57333
},
{
"interval": 260,
"cumulative": 260
}
]
},
{
"name": "http.admin.downstream_rq_time",
"values": [
{
"interval": null,
"cumulative": 0
},
{
"interval": null,
"cumulative": 0
},
{
"interval": null,
"cumulative": 1.0435787
},
{
"interval": null,
"cumulative": 1.0941565
},
{
"interval": null,
"cumulative": 2.0860023
},
{
"interval": null,
"cumulative": 3.0665233
},
{
"interval": null,
"cumulative": 6.046609
},
{
"interval": null,
"cumulative": 229.57333
},
{
"interval": null,
"cumulative": 260
}
]
}
]
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

.. http:get:: /stats?format=prometheus

or alternatively,

Expand Down
97 changes: 58 additions & 39 deletions source/server/http/admin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,17 +405,19 @@ Http::Code AdminImpl::handlerStats(absl::string_view url, Http::HeaderMap& respo
// implemented this can be switched back to a normal map.
std::multimap<std::string, std::string> all_histograms;
for (const Stats::ParentHistogramSharedPtr& histogram : server_.stats().histograms()) {
std::vector<std::string> summary;
const std::vector<double>& supported_quantiles_ref =
histogram->intervalStatistics().supportedQuantiles();
summary.reserve(supported_quantiles_ref.size());
for (size_t i = 0; i < supported_quantiles_ref.size(); ++i) {
summary.push_back(fmt::format("P{}({},{})", 100 * supported_quantiles_ref[i],
histogram->intervalStatistics().computedQuantiles()[i],
histogram->cumulativeStatistics().computedQuantiles()[i]));
}
if (histogram->used()) {
std::vector<std::string> summary;
const std::vector<double>& supported_quantiles_ref =
histogram->intervalStatistics().supportedQuantiles();
summary.reserve(supported_quantiles_ref.size());
for (size_t i = 0; i < supported_quantiles_ref.size(); ++i) {
summary.push_back(fmt::format("P{}({},{})", 100 * supported_quantiles_ref[i],
histogram->intervalStatistics().computedQuantiles()[i],
histogram->cumulativeStatistics().computedQuantiles()[i]));
}

all_histograms.emplace(histogram->name(), absl::StrJoin(summary, " "));
all_histograms.emplace(histogram->name(), absl::StrJoin(summary, " "));
}
}

if (params.size() == 0) {
Expand Down Expand Up @@ -520,39 +522,56 @@ AdminImpl::statsAsJson(const std::map<std::string, uint64_t>& all_stats,
stats_array.PushBack(stat_obj, allocator);
}

Value histograms_container_obj;
histograms_container_obj.SetObject();

Value histograms_obj;
histograms_obj.SetObject();

// It is not possible for the supported quantiles to differ across histograms, so it is ok to
// send them once.
Stats::HistogramStatisticsImpl empty_statistics;
rapidjson::Value supported_quantile_array(rapidjson::kArrayType);
for (double quantile : empty_statistics.supportedQuantiles()) {
Value quantile_type;
quantile_type.SetDouble(quantile * 100);
supported_quantile_array.PushBack(quantile_type, allocator);
}
histograms_obj.AddMember("supported_quantiles", supported_quantile_array, allocator);
rapidjson::Value histogram_array(rapidjson::kArrayType);

for (const Stats::ParentHistogramSharedPtr& histogram : all_histograms) {
Value histogram_obj;
histogram_obj.SetObject();
Value histogram_name;
histogram_name.SetString(histogram->name().c_str(), allocator);
histogram_obj.AddMember("name", histogram_name, allocator);

rapidjson::Value quantile_array(rapidjson::kArrayType);

// TODO(ramaraochavali): consider optimizing the model here. Quantiles can be added once,
// followed by two arrays interval and cumulative.
for (size_t i = 0; i < histogram->intervalStatistics().supportedQuantiles().size(); ++i) {
Value quantile_obj;
quantile_obj.SetObject();
Value quantile_type;
quantile_type.SetDouble(histogram->intervalStatistics().supportedQuantiles()[i] * 100);
quantile_obj.AddMember("quantile", quantile_type, allocator);
Value interval_value;
if (!std::isnan(histogram->intervalStatistics().computedQuantiles()[i])) {
interval_value.SetDouble(histogram->intervalStatistics().computedQuantiles()[i]);
}
quantile_obj.AddMember("interval_value", interval_value, allocator);
Value cumulative_value;
if (!std::isnan(histogram->cumulativeStatistics().computedQuantiles()[i])) {
cumulative_value.SetDouble(histogram->cumulativeStatistics().computedQuantiles()[i]);
if (histogram->used()) {
Value histogram_obj;
histogram_obj.SetObject();
Value histogram_name;
histogram_name.SetString(histogram->name().c_str(), allocator);
histogram_obj.AddMember("name", histogram_name, allocator);

rapidjson::Value computed_quantile_array(rapidjson::kArrayType);

for (size_t i = 0; i < histogram->intervalStatistics().supportedQuantiles().size(); ++i) {
Value quantile_obj;
quantile_obj.SetObject();
Value interval_value;
if (!std::isnan(histogram->intervalStatistics().computedQuantiles()[i])) {
interval_value.SetDouble(histogram->intervalStatistics().computedQuantiles()[i]);
}
quantile_obj.AddMember("interval", interval_value, allocator);
Value cumulative_value;
if (!std::isnan(histogram->cumulativeStatistics().computedQuantiles()[i])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cumulative_value.SetDouble(histogram->cumulativeStatistics().computedQuantiles()[i]);
}
quantile_obj.AddMember("cumulative", cumulative_value, allocator);
computed_quantile_array.PushBack(quantile_obj, allocator);
}
quantile_obj.AddMember("cumulative_value", cumulative_value, allocator);
quantile_array.PushBack(quantile_obj, allocator);
histogram_obj.AddMember("values", computed_quantile_array, allocator);
histogram_array.PushBack(histogram_obj, allocator);
}
histogram_obj.AddMember("quantiles", quantile_array, allocator);
stats_array.PushBack(histogram_obj, allocator);
}

histograms_obj.AddMember("computed_quantiles", histogram_array, allocator);
histograms_container_obj.AddMember("histograms", histograms_obj, allocator);
stats_array.PushBack(histograms_container_obj, allocator);
document.AddMember("stats", stats_array, allocator);
rapidjson::StringBuffer strbuf;
rapidjson::PrettyWriter<StringBuffer> writer(strbuf);
Expand Down
29 changes: 28 additions & 1 deletion test/integration/integration_admin_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,37 @@ TEST_P(IntegrationAdminTest, Admin) {
response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/stats?format=json",
"", downstreamProtocol(), version_);
EXPECT_TRUE(response->complete());
EXPECT_STREQ("200", response->headers().Status()->value().c_str());

Json::ObjectSharedPtr statsjson = Json::Factory::loadFromString(response->body());
EXPECT_TRUE(statsjson->hasObject("stats"));
EXPECT_STREQ("application/json", ContentType(response));
EXPECT_STREQ("200", response->headers().Status()->value().c_str());

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@ramaraochavali ramaraochavali May 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// Validate that both supported_quantiles and computed_quantiles are present in JSON.
EXPECT_TRUE(histograms_ptr->hasObject("supported_quantiles"));
EXPECT_TRUE(histograms_ptr->hasObject("computed_quantiles"));

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this EXPECT_EQ something specific here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Validate that each computed_quantile has name and value objects.
EXPECT_TRUE(computed_quantiles[0]->hasObject("name"));
EXPECT_TRUE(computed_quantiles[0]->hasObject("values"));

// Validate that supported and computed quantiles are of the same size.
EXPECT_EQ(histograms_ptr->getObjectArray("supported_quantiles").size(),
computed_quantiles[0]->getObjectArray("values").size());
}
}

// Validate that the stats JSON has exactly one histograms element.
EXPECT_EQ(1, histogram_count);

response = IntegrationUtil::makeSingleRequest(
lookupPort("admin"), "GET", "/stats?format=prometheus", "", downstreamProtocol(), version_);
Expand Down