diff --git a/source/common/BUILD b/source/common/BUILD index f576ced60..c77ede286 100644 --- a/source/common/BUILD +++ b/source/common/BUILD @@ -109,7 +109,7 @@ envoy_cc_library( "@envoy//source/common/http:utility_lib_with_external_headers", "@envoy//source/common/network:utility_lib_with_external_headers", "@envoy//source/common/protobuf:utility_lib_with_external_headers", - "@envoy//source/common/stats:stats_lib_with_external_headers", + "@envoy//source/common/stats:histogram_lib_with_external_headers", "@envoy//source/exe:envoy_common_lib_with_external_headers", "@envoy//source/server/config_validation:server_lib_with_external_headers", ], diff --git a/source/common/statistic_impl.cc b/source/common/statistic_impl.cc index bd5750467..b4a697a66 100644 --- a/source/common/statistic_impl.cc +++ b/source/common/statistic_impl.cc @@ -236,4 +236,102 @@ nighthawk::client::Statistic HdrStatistic::toProto(SerializationDomain domain) c return proto; } -} // namespace Nighthawk \ No newline at end of file +CircllhistStatistic::CircllhistStatistic() { + histogram_ = hist_alloc(); + ASSERT(histogram_ != nullptr); +} + +CircllhistStatistic::~CircllhistStatistic() { hist_free(histogram_); } + +void CircllhistStatistic::addValue(uint64_t value) { + hist_insert_intscale(histogram_, value, 0, 1); + StatisticImpl::addValue(value); +} +double CircllhistStatistic::mean() const { return hist_approx_mean(histogram_); } +double CircllhistStatistic::pvariance() const { return pstdev() * pstdev(); } +double CircllhistStatistic::pstdev() const { + return count() == 0 ? std::nan("") : hist_approx_stddev(histogram_); +} + +StatisticPtr CircllhistStatistic::combine(const Statistic& statistic) const { + auto combined = std::make_unique(); + const auto& stat = dynamic_cast(statistic); + hist_accumulate(combined->histogram_, &this->histogram_, /*cnt=*/1); + hist_accumulate(combined->histogram_, &stat.histogram_, /*cnt=*/1); + + combined->min_ = std::min(this->min(), stat.min()); + combined->max_ = std::max(this->max(), stat.max()); + combined->count_ = this->count() + stat.count(); + return combined; +} + +StatisticPtr CircllhistStatistic::createNewInstanceOfSameType() const { + return std::make_unique(); +} + +nighthawk::client::Statistic CircllhistStatistic::toProto(SerializationDomain domain) const { + nighthawk::client::Statistic proto = StatisticImpl::toProto(domain); + if (count() == 0) { + return proto; + } + + // List of quantiles is based on hdr_proto_json.gold. + const std::vector quantiles{0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.55, 0.6, + 0.65, 0.7, 0.75, 0.775, 0.8, 0.825, 0.85, 0.875, + 0.90, 0.925, 0.95, 0.975, 0.99, 0.995, 0.999, 1}; + std::vector computed_quantiles(quantiles.size(), 0.0); + hist_approx_quantile(histogram_, quantiles.data(), quantiles.size(), computed_quantiles.data()); + for (size_t i = 0; i < quantiles.size(); i++) { + nighthawk::client::Percentile* percentile = proto.add_percentiles(); + if (domain == Statistic::SerializationDomain::DURATION) { + setDurationFromNanos(*percentile->mutable_duration(), + static_cast(computed_quantiles[i])); + } else { + percentile->set_raw_value(computed_quantiles[i]); + } + percentile->set_percentile(quantiles[i]); + percentile->set_count(hist_approx_count_below(histogram_, computed_quantiles[i])); + } + + return proto; +} + +SinkableStatistic::SinkableStatistic(Envoy::Stats::Scope& scope, absl::optional worker_id) + : Envoy::Stats::HistogramImplHelper(scope.symbolTable()), scope_(scope), worker_id_(worker_id) { +} + +SinkableStatistic::~SinkableStatistic() { + // We must explicitly free the StatName here in order to supply the + // SymbolTable reference. + MetricImpl::clear(scope_.symbolTable()); +} + +Envoy::Stats::Histogram::Unit SinkableStatistic::unit() const { + return Envoy::Stats::Histogram::Unit::Unspecified; +} + +Envoy::Stats::SymbolTable& SinkableStatistic::symbolTable() { return scope_.symbolTable(); } + +SinkableHdrStatistic::SinkableHdrStatistic(Envoy::Stats::Scope& scope, + absl::optional worker_id) + : SinkableStatistic(scope, worker_id) {} + +void SinkableHdrStatistic::recordValue(uint64_t value) { + addValue(value); + // Currently in Envoy Scope implementation, deliverHistogramToSinks() will flush the histogram + // value directly to stats Sinks. + scope_.deliverHistogramToSinks(*this, value); +} + +SinkableCircllhistStatistic::SinkableCircllhistStatistic(Envoy::Stats::Scope& scope, + absl::optional worker_id) + : SinkableStatistic(scope, worker_id) {} + +void SinkableCircllhistStatistic::recordValue(uint64_t value) { + addValue(value); + // Currently in Envoy Scope implementation, deliverHistogramToSinks() will flush the histogram + // value directly to stats Sinks. + scope_.deliverHistogramToSinks(*this, value); +} + +} // namespace Nighthawk diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index 981133b44..d7e1f240f 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -7,6 +7,7 @@ #include "external/dep_hdrhistogram_c/src/hdr_histogram.h" #include "external/envoy/source/common/common/logger.h" +#include "external/envoy/source/common/stats/histogram_impl.h" #include "common/frequency.h" @@ -150,4 +151,94 @@ class HdrStatistic : public StatisticImpl { struct hdr_histogram* histogram_; }; -} // namespace Nighthawk \ No newline at end of file +/** + * CircllhistStatistic uses Circllhist under the hood to compute statistics. + * Circllhist is used in the implementation of Envoy Histograms, compared to HdrHistogram it trades + * precision for fast performance in merge and insertion. For more info, please see + * https://github.com/circonus-labs/libcircllhist + */ +class CircllhistStatistic : public StatisticImpl { +public: + CircllhistStatistic(); + ~CircllhistStatistic() override; + + void addValue(uint64_t value) override; + double mean() const override; + double pvariance() const override; + double pstdev() const override; + StatisticPtr combine(const Statistic& statistic) const override; + // circllhist has low significant digit precision as a result of base 10 + // algorithm. + uint64_t significantDigits() const override { return 1; } + StatisticPtr createNewInstanceOfSameType() const override; + nighthawk::client::Statistic toProto(SerializationDomain domain) const override; + +private: + histogram_t* histogram_; +}; + +/** + * In order to be able to flush a histogram value to downstream Envoy stats Sinks, abstract class + * SinkableStatistic takes the Scope reference in the constructor and wraps the + * Envoy::Stats::HistogramHelper interface. Implementation of sinkable Nighthawk Statistic class + * will inherit from this class. + */ +class SinkableStatistic : public Envoy::Stats::HistogramImplHelper { +public: + // Calling HistogramImplHelper(SymbolTable& symbol_table) constructor to construct an empty + // MetricImpl. This is to bypass the complicated logic of setting up SymbolTable/StatName in + // Envoy. + SinkableStatistic(Envoy::Stats::Scope& scope, absl::optional worker_id); + ~SinkableStatistic() override; + + // Currently Envoy Histogram Unit supports {Unspecified, Bytes, Microseconds, Milliseconds}. By + // default, Nighthawk::Statistic uses nanosecond as the unit of latency histograms, so Unspecified + // is returned here to isolate Nighthawk Statistic from Envoy Histogram Unit. + Envoy::Stats::Histogram::Unit unit() const override; + Envoy::Stats::SymbolTable& symbolTable() override; + // Return the id of the worker where this statistic is defined. Per worker + // statistic should always set worker_id. Return absl::nullopt when the + // statistic is not defined per worker. + const absl::optional worker_id() { return worker_id_; } + +protected: + // This is used in child class for delivering the histogram data to sinks. + Envoy::Stats::Scope& scope_; + +private: + // worker_id can be used in downstream stats Sinks as the stats tag. + absl::optional worker_id_; +}; + +// Implementation of sinkable Nighthawk Statistic with HdrHistogram. +class SinkableHdrStatistic : public SinkableStatistic, public HdrStatistic { +public: + // The constructor takes the Scope reference which is used to flush a histogram value to + // downstream stats Sinks through deliverHistogramToSinks(). + SinkableHdrStatistic(Envoy::Stats::Scope& scope, absl::optional worker_id = absl::nullopt); + + // Envoy::Stats::Histogram + void recordValue(uint64_t value) override; + bool used() const override { return count() > 0; } + // Overriding name() to return Nighthawk::Statistic::id(). + std::string name() const override { return id(); } + std::string tagExtractedName() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } +}; + +// Implementation of sinkable Nighthawk Statistic with Circllhist Histogram. +class SinkableCircllhistStatistic : public SinkableStatistic, public CircllhistStatistic { +public: + // The constructor takes the Scope reference which is used to flush a histogram value to + // downstream stats Sinks through deliverHistogramToSinks(). + SinkableCircllhistStatistic(Envoy::Stats::Scope& scope, + absl::optional worker_id = absl::nullopt); + + // Envoy::Stats::Histogram + void recordValue(uint64_t value) override; + bool used() const override { return count() > 0; } + // Overriding name() to return Nighthawk::Statistic::id(). + std::string name() const override { return id(); } + std::string tagExtractedName() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } +}; + +} // namespace Nighthawk diff --git a/test/BUILD b/test/BUILD index 868394027..f233f75f8 100644 --- a/test/BUILD +++ b/test/BUILD @@ -195,13 +195,17 @@ envoy_cc_test( envoy_cc_test( name = "statistic_test", srcs = ["statistic_test.cc"], - data = ["test_data/hdr_proto_json.gold"], + data = [ + "test_data/circllhist_proto_json.gold", + "test_data/hdr_proto_json.gold", + ], repository = "@envoy", deps = [ "//source/common:nighthawk_common_lib", "//test/test_common:environment_lib", "@envoy//source/common/protobuf:utility_lib_with_external_headers", "@envoy//source/common/stats:isolated_store_lib_with_external_headers", + "@envoy//test/mocks/stats:stats_mocks", ], ) diff --git a/test/run_nighthawk_bazel_coverage.sh b/test/run_nighthawk_bazel_coverage.sh index 8b48f4eec..0512e3b23 100755 --- a/test/run_nighthawk_bazel_coverage.sh +++ b/test/run_nighthawk_bazel_coverage.sh @@ -43,7 +43,7 @@ COVERAGE_VALUE=${COVERAGE_VALUE%?} if [ "$VALIDATE_COVERAGE" == "true" ] then - COVERAGE_THRESHOLD=98.6 + COVERAGE_THRESHOLD=98.4 COVERAGE_FAILED=$(echo "${COVERAGE_VALUE}<${COVERAGE_THRESHOLD}" | bc) if test ${COVERAGE_FAILED} -eq 1; then echo Code coverage ${COVERAGE_VALUE} is lower than limit of ${COVERAGE_THRESHOLD} @@ -52,4 +52,4 @@ then echo Code coverage ${COVERAGE_VALUE} is good and higher than limit of ${COVERAGE_THRESHOLD} fi fi -echo "HTML coverage report is in ${COVERAGE_DIR}/index.html" \ No newline at end of file +echo "HTML coverage report is in ${COVERAGE_DIR}/index.html" diff --git a/test/statistic_test.cc b/test/statistic_test.cc index c73e1d6b8..b72d76043 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -2,10 +2,12 @@ #include #include +#include #include // std::bad_cast #include "external/envoy/source/common/protobuf/utility.h" #include "external/envoy/source/common/stats/isolated_store_impl.h" +#include "external/envoy/test/mocks/stats/mocks.h" #include "external/envoy/test/test_common/file_system_for_test.h" #include "external/envoy/test/test_common/utility.h" @@ -20,7 +22,8 @@ using namespace testing; namespace Nighthawk { -using MyTypes = Types; +using MyTypes = Types; template class TypedStatisticTest : public Test {}; @@ -116,15 +119,15 @@ TYPED_TEST(TypedStatisticTest, SingleAndDoubleValue) { a.addValue(1); EXPECT_EQ(1, a.count()); - EXPECT_DOUBLE_EQ(1, a.mean()); + Helper::expectNear(1.0, a.mean(), a.significantDigits()); EXPECT_DOUBLE_EQ(0, a.pvariance()); EXPECT_DOUBLE_EQ(0, a.pstdev()); a.addValue(2); EXPECT_EQ(2, a.count()); - EXPECT_DOUBLE_EQ(1.5, a.mean()); - EXPECT_DOUBLE_EQ(0.25, a.pvariance()); - EXPECT_DOUBLE_EQ(0.5, a.pstdev()); + Helper::expectNear(1.5, a.mean(), a.significantDigits()); + Helper::expectNear(0.25, a.pvariance(), a.significantDigits()); + Helper::expectNear(0.5, a.pstdev(), a.significantDigits()); } TYPED_TEST(TypedStatisticTest, CatastrophicalCancellation) { @@ -220,6 +223,15 @@ TYPED_TEST(TypedStatisticTest, StringOutput) { } } +TYPED_TEST(TypedStatisticTest, IdFieldWorks) { + TypeParam statistic; + std::string id = "fooid"; + + EXPECT_EQ("", statistic.id()); + statistic.setId(id); + EXPECT_EQ(id, statistic.id()); +} + class StatisticTest : public Test {}; // Note that we explicitly subject SimpleStatistic to the large @@ -269,6 +281,20 @@ TEST(StatisticTest, StreamingStatProtoOutputLargeValues) { EXPECT_EQ(proto.pstdev().nanos(), 0); } +TEST(StatisticTest, CircllhistStatisticProtoOutputLargeValues) { + CircllhistStatistic statistic; + uint64_t value = 100ul + 0xFFFFFFFF; + statistic.addValue(value); + statistic.addValue(value); + const nighthawk::client::Statistic proto = + statistic.toProto(Statistic::SerializationDomain::DURATION); + + EXPECT_EQ(proto.count(), 2); + Helper::expectNear(Envoy::Protobuf::util::TimeUtil::DurationToNanoseconds(proto.mean()), value, + statistic.significantDigits()); + EXPECT_EQ(Envoy::Protobuf::util::TimeUtil::DurationToNanoseconds(proto.pstdev()), 0); +} + TEST(StatisticTest, HdrStatisticPercentilesProto) { nighthawk::client::Statistic parsed_json_proto; HdrStatistic statistic; @@ -281,33 +307,51 @@ TEST(StatisticTest, HdrStatisticPercentilesProto) { util.loadFromJson(Envoy::Filesystem::fileSystemForTest().fileReadToEnd( TestEnvironment::runfilesPath("test/test_data/hdr_proto_json.gold")), parsed_json_proto, Envoy::ProtobufMessage::getStrictValidationVisitor()); - // Instead of comparing proto's, we perform a string-based comparison, because that emits a - // helpful diff when this fails. const std::string json = util.getJsonStringFromMessage( statistic.toProto(Statistic::SerializationDomain::DURATION), true, true); const std::string golden_json = util.getJsonStringFromMessage(parsed_json_proto, true, true); - EXPECT_EQ(json, golden_json); + EXPECT_THAT(statistic.toProto(Statistic::SerializationDomain::DURATION), + Envoy::ProtoEq(parsed_json_proto)) + << json << "\n" + << "is not equal to golden file:\n" + << golden_json; +} + +TEST(StatisticTest, CircllhistStatisticPercentilesProto) { + nighthawk::client::Statistic parsed_json_proto; + CircllhistStatistic statistic; + + for (int i = 1; i <= 10; i++) { + statistic.addValue(i); + } + + Envoy::MessageUtil util; + util.loadFromJson(Envoy::Filesystem::fileSystemForTest().fileReadToEnd( + TestEnvironment::runfilesPath("test/test_data/circllhist_proto_json.gold")), + parsed_json_proto, Envoy::ProtobufMessage::getStrictValidationVisitor()); + const std::string json = util.getJsonStringFromMessage( + statistic.toProto(Statistic::SerializationDomain::DURATION), true, true); + const std::string golden_json = util.getJsonStringFromMessage(parsed_json_proto, true, true); + EXPECT_THAT(statistic.toProto(Statistic::SerializationDomain::DURATION), + Envoy::ProtoEq(parsed_json_proto)) + << json << "\n" + << "is not equal to golden file:\n" + << golden_json; } TEST(StatisticTest, CombineAcrossTypesFails) { HdrStatistic a; InMemoryStatistic b; StreamingStatistic c; + CircllhistStatistic d; EXPECT_THROW(a.combine(b), std::bad_cast); EXPECT_THROW(a.combine(c), std::bad_cast); EXPECT_THROW(b.combine(a), std::bad_cast); EXPECT_THROW(b.combine(c), std::bad_cast); EXPECT_THROW(c.combine(a), std::bad_cast); EXPECT_THROW(c.combine(b), std::bad_cast); -} - -TEST(StatisticTest, IdFieldWorks) { - StreamingStatistic c; - std::string id = "fooid"; - - EXPECT_EQ("", c.id()); - c.setId(id); - EXPECT_EQ(id, c.id()); + EXPECT_THROW(c.combine(d), std::bad_cast); + EXPECT_THROW(d.combine(a), std::bad_cast); } TEST(StatisticTest, HdrStatisticOutOfRange) { @@ -319,13 +363,73 @@ TEST(StatisticTest, HdrStatisticOutOfRange) { TEST(StatisticTest, NullStatistic) { NullStatistic stat; EXPECT_EQ(0, stat.count()); + std::string id = "fooid"; + stat.setId(id); + EXPECT_EQ(id, stat.id()); stat.addValue(1); + EXPECT_EQ(0, stat.count()); + EXPECT_EQ(0, stat.max()); + EXPECT_EQ(UINT64_MAX, stat.min()); EXPECT_EQ(0, stat.mean()); EXPECT_EQ(0, stat.pvariance()); EXPECT_EQ(0, stat.pstdev()); EXPECT_NE(nullptr, stat.combine(stat)); EXPECT_EQ(0, stat.significantDigits()); EXPECT_NE(nullptr, stat.createNewInstanceOfSameType()); + const nighthawk::client::Statistic proto = stat.toProto(Statistic::SerializationDomain::RAW); + EXPECT_EQ(id, proto.id()); + EXPECT_EQ(0, proto.count()); + EXPECT_EQ(0, proto.raw_mean()); + EXPECT_EQ(0, proto.raw_pstdev()); + EXPECT_EQ(0, proto.raw_max()); + EXPECT_EQ(UINT64_MAX, proto.raw_min()); +} + +using SinkableTypes = Types; + +template class SinkableStatisticTest : public Test {}; + +TYPED_TEST_SUITE(SinkableStatisticTest, SinkableTypes); + +TYPED_TEST(SinkableStatisticTest, EmptySinkableStatistic) { + Envoy::Stats::MockIsolatedStatsStore mock_store; + TypeParam stat(mock_store); + EXPECT_EQ(0, stat.count()); + EXPECT_TRUE(std::isnan(stat.mean())); + EXPECT_TRUE(std::isnan(stat.pvariance())); + EXPECT_TRUE(std::isnan(stat.pstdev())); + EXPECT_EQ(stat.min(), UINT64_MAX); + EXPECT_EQ(stat.max(), 0); + EXPECT_EQ(Envoy::Stats::Histogram::Unit::Unspecified, stat.unit()); + EXPECT_FALSE(stat.used()); + EXPECT_EQ("", stat.name()); + EXPECT_DEATH(stat.tagExtractedName(), ".*"); + EXPECT_EQ(absl::nullopt, stat.worker_id()); +} + +TYPED_TEST(SinkableStatisticTest, SimpleSinkableStatistic) { + Envoy::Stats::MockIsolatedStatsStore mock_store; + const int worker_id = 0; + TypeParam stat(mock_store, worker_id); + const uint64_t sample_value = 123; + const std::string stat_name = "stat_name"; + + EXPECT_CALL(mock_store, deliverHistogramToSinks(_, sample_value)).Times(1); + stat.recordValue(sample_value); + stat.setId(stat_name); + + EXPECT_EQ(1, stat.count()); + Helper::expectNear(123.0, stat.mean(), stat.significantDigits()); + EXPECT_DOUBLE_EQ(0, stat.pvariance()); + EXPECT_DOUBLE_EQ(0, stat.pstdev()); + EXPECT_EQ(123, stat.min()); + EXPECT_EQ(123, stat.max()); + EXPECT_EQ(Envoy::Stats::Histogram::Unit::Unspecified, stat.unit()); + EXPECT_TRUE(stat.used()); + EXPECT_EQ(stat_name, stat.name()); + EXPECT_DEATH(stat.tagExtractedName(), ".*"); + EXPECT_TRUE(stat.worker_id().has_value()); + EXPECT_EQ(worker_id, stat.worker_id().value()); } } // namespace Nighthawk diff --git a/test/test_data/circllhist_proto_json.gold b/test/test_data/circllhist_proto_json.gold new file mode 100644 index 000000000..4262b4fd2 --- /dev/null +++ b/test/test_data/circllhist_proto_json.gold @@ -0,0 +1,130 @@ +{ + "count": "10", + "id": "", + "percentiles": [ + { + "percentile": 0, + "count": "0", + "duration": "0.000000001s" + }, + { + "percentile": 0.1, + "count": "1", + "duration": "0.000000001s" + }, + { + "percentile": 0.2, + "count": "2", + "duration": "0.000000002s" + }, + { + "percentile": 0.3, + "count": "3", + "duration": "0.000000003s" + }, + { + "percentile": 0.4, + "count": "4", + "duration": "0.000000004s" + }, + { + "percentile": 0.5, + "count": "5", + "duration": "0.000000005s" + }, + { + "percentile": 0.55, + "count": "5", + "duration": "0.000000006s" + }, + { + "percentile": 0.6, + "count": "6", + "duration": "0.000000006s" + }, + { + "percentile": 0.65, + "count": "6", + "duration": "0.000000007s" + }, + { + "percentile": 0.7, + "count": "7", + "duration": "0.000000007s" + }, + { + "percentile": 0.75, + "count": "7", + "duration": "0.000000008s" + }, + { + "percentile": 0.775, + "count": "7", + "duration": "0.000000008s" + }, + { + "percentile": 0.8, + "count": "8", + "duration": "0.000000008s" + }, + { + "percentile": 0.825, + "count": "8", + "duration": "0.000000009s" + }, + { + "percentile": 0.85, + "count": "8", + "duration": "0.000000009s" + }, + { + "percentile": 0.875, + "count": "8", + "duration": "0.000000009s" + }, + { + "percentile": 0.9, + "count": "9", + "duration": "0.000000009s" + }, + { + "percentile": 0.925, + "count": "9", + "duration": "0.000000010s" + }, + { + "percentile": 0.95, + "count": "9", + "duration": "0.000000010s" + }, + { + "percentile": 0.975, + "count": "9", + "duration": "0.000000010s" + }, + { + "percentile": 0.99, + "count": "9", + "duration": "0.000000010s" + }, + { + "percentile": 0.995, + "count": "9", + "duration": "0.000000010s" + }, + { + "percentile": 0.999, + "count": "9", + "duration": "0.000000010s" + }, + { + "percentile": 1, + "count": "10", + "duration": "0.000000011s" + } + ], + "mean": "0.000000006s", + "pstdev": "0.000000003s", + "min": "0.000000001s", + "max": "0.000000010s" +}