From 4c290b80dc838996078a3c8034fa8a7a7c879c20 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Sat, 6 Feb 2021 19:46:39 +0100 Subject: [PATCH 1/2] Stats: Hdr histogram serialization/deserialization Being able to serialize and deserialize statistics serves several goals: - Allow over-the-wire transportation in horizontally scaled setups. - Facilitate storage of high res data. - Could be used as a means to decouple our workers from the main thread, by avoiding reliance on a shared process namespace to merge statistics. (not a goal right now) This adds an abstraction to Statistic, implements it for HdrHistogram, and adds some tests. Part of horizontal scaling effort, split out from: Split out from https://github.com/oschaaf/nighthawk/tree/horizontal-scaling Signed-off-by: Otto van der Schaaf --- include/nighthawk/common/BUILD | 1 + include/nighthawk/common/statistic.h | 18 ++++++++++++++ source/common/statistic_impl.cc | 35 +++++++++++++++++++++++++++ source/common/statistic_impl.h | 5 ++++ test/statistic_test.cc | 36 ++++++++++++++++++++++++++++ 5 files changed, 95 insertions(+) diff --git a/include/nighthawk/common/BUILD b/include/nighthawk/common/BUILD index 20f89ef3d..1bf10fea4 100644 --- a/include/nighthawk/common/BUILD +++ b/include/nighthawk/common/BUILD @@ -32,6 +32,7 @@ envoy_basic_cc_library( "@envoy//include/envoy/upstream:cluster_manager_interface_with_external_headers", "@envoy//source/common/common:minimal_logger_lib", "@envoy//source/common/common:non_copyable_with_external_headers", + "@envoy//source/common/common:statusor_lib_with_external_headers", "@envoy//source/common/event:dispatcher_lib_with_external_headers", "@envoy//source/common/network:utility_lib_with_external_headers", ], diff --git a/include/nighthawk/common/statistic.h b/include/nighthawk/common/statistic.h index 85b88e082..b7508e15f 100644 --- a/include/nighthawk/common/statistic.h +++ b/include/nighthawk/common/statistic.h @@ -11,6 +11,7 @@ #include "api/client/output.pb.h" +#include "absl/status/statusor.h" #include "absl/strings/string_view.h" namespace Nighthawk { @@ -115,6 +116,23 @@ class Statistic : Envoy::NonCopyable { * @param id The id that should be set for the Statistic instance. */ virtual void setId(absl::string_view id) PURE; + + /** + * Build a string representation of this Statistic instance. + * + * @return absl::StatusOr> Status or a stream that will yield + * a serialized representation of this Statistic instance. + */ + virtual absl::StatusOr> serializeNative() const PURE; + + /** + * Reconstruct this Statistic instance using the serialization delivered by the input stream. + * + * @param input_stream Stream that will deliver a serialized representation. + * @return absl::Status Status indicating success or failure. Upon success the statistic + * instance this was called for will now represent what the stream contained. + */ + virtual absl::Status deserializeNative(std::istream& input_stream) PURE; }; } // namespace Nighthawk \ No newline at end of file diff --git a/source/common/statistic_impl.cc b/source/common/statistic_impl.cc index 0810b99ae..62e27ea52 100644 --- a/source/common/statistic_impl.cc +++ b/source/common/statistic_impl.cc @@ -2,8 +2,10 @@ #include #include +#include #include +#include "external/dep_hdrhistogram_c/src/hdr_histogram_log.h" #include "external/envoy/source/common/common/assert.h" #include "external/envoy/source/common/protobuf/utility.h" @@ -68,6 +70,14 @@ uint64_t StatisticImpl::min() const { return min_; }; uint64_t StatisticImpl::max() const { return max_; }; +absl::StatusOr> StatisticImpl::serializeNative() const { + return absl::Status(absl::StatusCode::kUnimplemented, "serializeNative not implemented."); +} + +absl::Status StatisticImpl::deserializeNative(std::istream&) { + return absl::Status(absl::StatusCode::kUnimplemented, "deserializeNative not implemented."); +} + void SimpleStatistic::addValue(uint64_t value) { StatisticImpl::addValue(value); sum_x_ += value; @@ -236,6 +246,31 @@ nighthawk::client::Statistic HdrStatistic::toProto(SerializationDomain domain) c return proto; } +absl::StatusOr> HdrStatistic::serializeNative() const { + char* data; + if (hdr_log_encode(histogram_, &data) == 0) { + absl::string_view s(data, strlen(data)); + auto ss = std::make_unique(); + *ss << s; + free(data); + return ss; + } + ENVOY_LOG(error, "Failed to write HdrHistogram data."); + return absl::Status(absl::StatusCode::kInternal, "Failed to write HdrHistogram data"); +} + +absl::Status HdrStatistic::deserializeNative(std::istream& stream) { + std::string s(std::istreambuf_iterator(stream), {}); + struct hdr_histogram* new_histogram = nullptr; + if (hdr_log_decode(&new_histogram, const_cast(s.c_str()), s.length()) == 0) { + hdr_close(histogram_); + histogram_ = new_histogram; + return absl::OkStatus(); + } + ENVOY_LOG(error, "Failed to read back HdrHistogram data."); + return absl::Status(absl::StatusCode::kInternal, "Failed to read back HdrHistogram data"); +} + CircllhistStatistic::CircllhistStatistic() { histogram_ = hist_alloc(); ASSERT(histogram_ != nullptr); diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index ea6883750..0dcdab7a8 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -26,6 +26,8 @@ class StatisticImpl : public Statistic, public Envoy::Logger::Loggable> serializeNative() const override; + absl::Status deserializeNative(std::istream&) override; protected: std::string id_; @@ -146,6 +148,9 @@ class HdrStatistic : public StatisticImpl { return std::make_unique(); }; + absl::StatusOr> serializeNative() const override; + absl::Status deserializeNative(std::istream&) override; + private: static const int SignificantDigits; struct hdr_histogram* histogram_; diff --git a/test/statistic_test.cc b/test/statistic_test.cc index 596f04983..f0ad34740 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -209,6 +209,42 @@ TYPED_TEST(TypedStatisticTest, ProtoOutputEmptyStats) { EXPECT_EQ(proto.pstdev().nanos(), 0); } +TYPED_TEST(TypedStatisticTest, NativeRoundtrip) { + TypeParam a; + + a.setId("bar"); + a.addValue(6543456); + a.addValue(342335); + a.addValue(543); + + const absl::StatusOr> status_or_stream = a.serializeNative(); + if (status_or_stream.ok()) { + // If the histogram states it implements native serialization/deserialization, put it through + // a round trip test. + TypeParam b; + absl::Status status = b.deserializeNative(*status_or_stream.value()); + EXPECT_TRUE(status.ok()); + EXPECT_EQ(3, b.count()); + EXPECT_EQ(a.count(), b.count()); + EXPECT_EQ(a.mean(), b.mean()); + EXPECT_EQ(a.pstdev(), b.pstdev()); + } else { + EXPECT_EQ(status_or_stream.status().code(), absl::StatusCode::kUnimplemented); + } +} + +TYPED_TEST(TypedStatisticTest, AttemptsToDeserializeBogusBehaveWell) { + // Deserializing corrupted data should either result in the statistic reporting + // it didn't implement deserialization, or having it report an internal failure. + const std::vector expected_status_list{absl::StatusCode::kInternal, + absl::StatusCode::kUnimplemented}; + TypeParam a; + std::istringstream bogus_input(std::string("BOGUS")); + const absl::Status status = a.deserializeNative(bogus_input); + EXPECT_FALSE(status.ok()); + EXPECT_THAT(expected_status_list, Contains(status.code())); +} + TYPED_TEST(TypedStatisticTest, StringOutput) { TypeParam a; From 79bad4678f3dd628f66573c5a84a2bbf0e12c209 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Tue, 9 Feb 2021 21:51:03 +0100 Subject: [PATCH 2/2] Address review feedback Signed-off-by: Otto van der Schaaf --- source/common/statistic_impl.cc | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/source/common/statistic_impl.cc b/source/common/statistic_impl.cc index 62e27ea52..1a0067066 100644 --- a/source/common/statistic_impl.cc +++ b/source/common/statistic_impl.cc @@ -249,11 +249,11 @@ nighthawk::client::Statistic HdrStatistic::toProto(SerializationDomain domain) c absl::StatusOr> HdrStatistic::serializeNative() const { char* data; if (hdr_log_encode(histogram_, &data) == 0) { - absl::string_view s(data, strlen(data)); - auto ss = std::make_unique(); - *ss << s; + auto write_stream = std::make_unique(); + *write_stream << absl::string_view(data, strlen(data)); + // Free the memory allocated by hrd_log_encode. free(data); - return ss; + return write_stream; } ENVOY_LOG(error, "Failed to write HdrHistogram data."); return absl::Status(absl::StatusCode::kInternal, "Failed to write HdrHistogram data"); @@ -262,8 +262,12 @@ absl::StatusOr> HdrStatistic::serializeNative() co absl::Status HdrStatistic::deserializeNative(std::istream& stream) { std::string s(std::istreambuf_iterator(stream), {}); struct hdr_histogram* new_histogram = nullptr; + // hdr_log_decode allocates memory for the new hdr histogram. if (hdr_log_decode(&new_histogram, const_cast(s.c_str()), s.length()) == 0) { + // Free the memory allocated by our current hdr histogram. hdr_close(histogram_); + // Swap in the new histogram. + // NOTE: Our destructor will eventually call hdr_close on the new one. histogram_ = new_histogram; return absl::OkStatus(); }