Skip to content

stats: report sample count as an integer to prevent loss of precision #6274

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

Merged
mattklein123 merged 1 commit into from
Apr 3, 2019
Merged

stats: report sample count as an integer to prevent loss of precision #6274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion include/envoy/stats/histogram.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class HistogramStatistics {
* of the number of samples in the histogram, it is not guaranteed that this will be
* 100% the number of samples observed.
*/
virtual double sampleCount() const PURE;
virtual uint64_t sampleCount() const PURE;

/**
* Returns sum of all values during the period.
Expand Down
4 changes: 2 additions & 2 deletions source/common/stats/histogram_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@ class HistogramStatisticsImpl : public HistogramStatistics, NonCopyable {
const std::vector<double>& computedQuantiles() const override { return computed_quantiles_; }
const std::vector<double>& supportedBuckets() const override;
const std::vector<uint64_t>& computedBuckets() const override { return computed_buckets_; }
double sampleCount() const override { return sample_count_; }
uint64_t sampleCount() const override { return sample_count_; }
double sampleSum() const override { return sample_sum_; }

private:
std::vector<double> computed_quantiles_;
std::vector<uint64_t> computed_buckets_;
double sample_count_;
uint64_t sample_count_;
double sample_sum_;
};

Expand Down
2 changes: 1 addition & 1 deletion source/server/http/admin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -820,7 +820,7 @@ uint64_t PrometheusStatsFormatter::statsAsPrometheus(

response.add(fmt::format("{0}_bucket{{{1}le=\"+Inf\"}} {2}\n", metric_name, hist_tags,
stats.sampleCount()));
response.add(fmt::format("{0}_sum{{{1}}} {2}\n", metric_name, tags, stats.sampleSum()));
response.add(fmt::format("{0}_sum{{{1}}} {2:.32g}\n", metric_name, tags, stats.sampleSum()));
Copy link
Contributor

@dnoe dnoe Mar 13, 2019

Choose a reason for hiding this comment

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

My only concern with this is that it's changing something user visible and might break parsers. But if I understand correctly this output is meant to be parsed specifically by Prometheus scraper, so it's probably not being scraped by someone's custom stuff. WDYT?

Copy link
Member Author

@zuercher zuercher Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I agree. It's also changing it to a purely [0-9]+(\.[0-9]+)? format, so I think that makes it less likely to be misinterpreted. To be fair, I believe prometheus can consume the scientific notation values correctly outside of the tags it uses for histogram bucketing. I changed this so that it matched the other outputs.

response.add(fmt::format("{0}_count{{{1}}} {2}\n", metric_name, tags, stats.sampleCount()));
}

Expand Down
59 changes: 59 additions & 0 deletions test/server/http/admin_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1254,6 +1254,12 @@ class HistogramWrapper {
}
}

void setHistogramValuesWithCounts(const std::vector<std::pair<uint64_t, uint64_t>>& values) {
for (std::pair<uint64_t, uint64_t> cv : values) {
hist_insert_intscale(histogram_, cv.first, 0, cv.second);
}
}

private:
histogram_t* histogram_;
};
Expand Down Expand Up @@ -1399,6 +1405,59 @@ envoy_histogram1_count{} 0
EXPECT_EQ(expected_output, response.toString());
}

TEST_F(PrometheusStatsFormatterTest, HistogramWithHighCounts) {
HistogramWrapper h1_cumulative;

// Force large counts to prove that the +Inf bucket doesn't overflow to scientific notation.
h1_cumulative.setHistogramValuesWithCounts(std::vector<std::pair<uint64_t, uint64_t>>({
{1, 100000},
{100, 1000000},
{1000, 100000000},
}));

Stats::HistogramStatisticsImpl h1_cumulative_statistics(h1_cumulative.getHistogram());

auto histogram = std::make_shared<NiceMock<Stats::MockParentHistogram>>();
histogram->name_ = "histogram1";
histogram->used_ = true;
ON_CALL(*histogram, cumulativeStatistics())
.WillByDefault(testing::ReturnRef(h1_cumulative_statistics));

addHistogram(histogram);

Buffer::OwnedImpl response;
auto size =
PrometheusStatsFormatter::statsAsPrometheus(counters_, gauges_, histograms_, response, false);
EXPECT_EQ(1UL, size);

const std::string expected_output = R"EOF(# TYPE envoy_histogram1 histogram
envoy_histogram1_bucket{le="0.5"} 0
envoy_histogram1_bucket{le="1"} 0
envoy_histogram1_bucket{le="5"} 100000
envoy_histogram1_bucket{le="10"} 100000
envoy_histogram1_bucket{le="25"} 100000
envoy_histogram1_bucket{le="50"} 100000
envoy_histogram1_bucket{le="100"} 100000
envoy_histogram1_bucket{le="250"} 1100000
envoy_histogram1_bucket{le="500"} 1100000
envoy_histogram1_bucket{le="1000"} 1100000
envoy_histogram1_bucket{le="2500"} 101100000
envoy_histogram1_bucket{le="5000"} 101100000
envoy_histogram1_bucket{le="10000"} 101100000
envoy_histogram1_bucket{le="30000"} 101100000
envoy_histogram1_bucket{le="60000"} 101100000
envoy_histogram1_bucket{le="300000"} 101100000
envoy_histogram1_bucket{le="600000"} 101100000
envoy_histogram1_bucket{le="1800000"} 101100000
envoy_histogram1_bucket{le="3600000"} 101100000
envoy_histogram1_bucket{le="+Inf"} 101100000
envoy_histogram1_sum{} 105105105000
envoy_histogram1_count{} 101100000
)EOF";

EXPECT_EQ(expected_output, response.toString());
}

TEST_F(PrometheusStatsFormatterTest, OutputWithAllMetricTypes) {
addCounter("cluster.test_1.upstream_cx_total", {{"a.tag-name", "a.tag-value"}});
addCounter("cluster.test_2.upstream_cx_total", {{"another_tag_name", "another_tag-value"}});
Expand Down