From a4818e995f58d2e87efcf7c4a1ebfe8a5e11a0bf Mon Sep 17 00:00:00 2001 From: Jakub Sobon Date: Wed, 7 Feb 2024 09:26:47 -0500 Subject: [PATCH] Migrate Nighthawk off the proto DebugString method. (#1078) The `DebugString` method will be deprecated shortly. Migrating to `absl::StrCat` on the proto messages instead. Signed-off-by: Jakub Sobon --- source/adaptive_load/adaptive_load_client_main.cc | 3 ++- .../adaptive_load/adaptive_load_controller_impl.cc | 9 +++++---- source/client/process_impl.cc | 3 ++- source/client/service_impl.cc | 8 +++++--- source/common/request_stream_grpc_client_impl.cc | 6 ++++-- source/common/statistic_impl.cc | 3 ++- .../nighthawk_distributor_client_impl.cc | 6 ++++-- source/distributor/service_impl.cc | 6 ++++-- source/server/configuration.cc | 3 ++- source/sink/service_impl.cc | 14 ++++++++------ .../adaptive_load_client_main_test.cc | 3 ++- .../adaptive_load/adaptive_load_controller_test.cc | 3 ++- test/test_common/proto_matchers.h | 5 +++-- 13 files changed, 45 insertions(+), 27 deletions(-) diff --git a/source/adaptive_load/adaptive_load_client_main.cc b/source/adaptive_load/adaptive_load_client_main.cc index edeedc5af..bfb01d146 100644 --- a/source/adaptive_load/adaptive_load_client_main.cc +++ b/source/adaptive_load/adaptive_load_client_main.cc @@ -20,6 +20,7 @@ #include "source/common/utility.h" #include "source/common/version_info.h" +#include "absl/strings/str_cat.h" #include "fmt/ranges.h" #include "google/rpc/status.pb.h" #include "tclap/CmdLine.h" @@ -125,7 +126,7 @@ uint32_t AdaptiveLoadClientMain::Run() { return 1; } nighthawk::adaptive_load::AdaptiveLoadSessionOutput output = output_or.value(); - WriteFileOrThrow(filesystem_, output_filename_, output.DebugString()); + WriteFileOrThrow(filesystem_, output_filename_, absl::StrCat(output)); return 0; } diff --git a/source/adaptive_load/adaptive_load_controller_impl.cc b/source/adaptive_load/adaptive_load_controller_impl.cc index 07f8fc3a8..077a64ab4 100644 --- a/source/adaptive_load/adaptive_load_controller_impl.cc +++ b/source/adaptive_load/adaptive_load_controller_impl.cc @@ -27,6 +27,7 @@ #include "absl/container/flat_hash_map.h" #include "absl/status/status.h" +#include "absl/strings/str_cat.h" #include "absl/strings/str_format.h" #include "absl/strings/str_join.h" @@ -87,7 +88,7 @@ void LogGlobalResultExcludingStatistics(const nighthawk::client::ExecutionRespon ENVOY_LOG_MISC(info, "Got result (stripped to just the global result excluding " "statistics): {}", - stripped.DebugString()); + absl::StrCat(stripped)); } /** @@ -137,7 +138,7 @@ absl::StatusOr AdaptiveLoadControllerImpl::PerformAndAnalyzeNig // or testing stage. *command_line_options.mutable_duration() = std::move(duration); - ENVOY_LOG_MISC(info, "Sending load: {}", command_line_options.DebugString()); + ENVOY_LOG_MISC(info, "Sending load: {}", absl::StrCat(command_line_options)); Envoy::SystemTime start_time = time_source_.systemTime(); absl::StatusOr nighthawk_response_or = nighthawk_service_client_.PerformNighthawkBenchmark(nighthawk_service_stub, @@ -165,7 +166,7 @@ absl::StatusOr AdaptiveLoadControllerImpl::PerformAndAnalyzeNig Envoy::TimestampUtil::systemClockToTimestamp(end_time, *benchmark_result.mutable_end_time()); for (const MetricEvaluation& evaluation : benchmark_result.metric_evaluations()) { - ENVOY_LOG_MISC(info, "Evaluation: {}", evaluation.DebugString()); + ENVOY_LOG_MISC(info, "Evaluation: {}", absl::StrCat(evaluation)); } step_controller.UpdateAndRecompute(benchmark_result); return benchmark_result; @@ -206,7 +207,7 @@ absl::StatusOr AdaptiveLoadControllerImpl::PerformAda if (spec.has_benchmark_cooldown_duration()) { ENVOY_LOG_MISC(info, "Cooling down before the next benchmark for duration: {}", - spec.benchmark_cooldown_duration().ShortDebugString()); + absl::StrCat(spec.benchmark_cooldown_duration())); uint64_t sleep_time_ms = Envoy::Protobuf::util::TimeUtil::DurationToMilliseconds( spec.benchmark_cooldown_duration()); absl::SleepFor(absl::Milliseconds(sleep_time_ms)); diff --git a/source/client/process_impl.cc b/source/client/process_impl.cc index 1fc73b0db..7222ba8b0 100644 --- a/source/client/process_impl.cc +++ b/source/client/process_impl.cc @@ -37,6 +37,7 @@ #include "source/client/process_bootstrap.h" +#include "absl/strings/str_cat.h" #include "absl/strings/str_replace.h" #include "absl/types/optional.h" @@ -839,7 +840,7 @@ bool ProcessImpl::runInternal(OutputCollector& collector, const UriPtr& tracing_ setupTracingImplementation(bootstrap_, *tracing_uri); addTracingCluster(bootstrap_, *tracing_uri); } - ENVOY_LOG(debug, "Computed configuration: {}", bootstrap_.DebugString()); + ENVOY_LOG(debug, "Computed configuration: {}", absl::StrCat(bootstrap_)); cluster_manager_ = cluster_manager_factory_->clusterManagerFromProto(bootstrap_); maybeCreateTracingDriver(bootstrap_.tracing()); cluster_manager_->setInitializedCb( diff --git a/source/client/service_impl.cc b/source/client/service_impl.cc index 709757d9b..aaff9d38e 100644 --- a/source/client/service_impl.cc +++ b/source/client/service_impl.cc @@ -9,6 +9,8 @@ #include "source/client/output_collector_impl.h" #include "source/common/request_source_impl.h" +#include "absl/strings/str_cat.h" + namespace Nighthawk { namespace Client { @@ -78,7 +80,7 @@ void ServiceImpl::handleExecutionRequest(const nighthawk::client::ExecutionReque } void ServiceImpl::writeResponse(const nighthawk::client::ExecutionResponse& response) { - ENVOY_LOG(debug, "Write response: {}", response.DebugString()); + ENVOY_LOG(debug, "Write response: {}", absl::StrCat(response)); if (!stream_->Write(response)) { ENVOY_LOG(warn, "Failed to write response to the stream"); } @@ -107,7 +109,7 @@ grpc::Status ServiceImpl::ExecutionStream( stream_ = stream; while (stream->Read(&request)) { - ENVOY_LOG(debug, "Read ExecutionRequest data {}", request.DebugString()); + ENVOY_LOG(debug, "Read ExecutionRequest data {}", absl::StrCat(request)); if (request.has_start_request()) { // If busy_lock_ is held we can't start a new benchmark run because one is active already. if (busy_lock_.tryLock()) { @@ -153,7 +155,7 @@ grpc::Status RequestSourceServiceImpl::RequestStream( nighthawk::request_source::RequestStreamRequest request; bool ok = true; while (stream->Read(&request)) { - ENVOY_LOG(trace, "Inbound RequestStreamRequest {}", request.DebugString()); + ENVOY_LOG(trace, "Inbound RequestStreamRequest {}", absl::StrCat(request)); // TODO(oschaaf): this is useful for integration testing purposes, but sending // these nearly empty headers will basically be a near no-op (note that the client will merge diff --git a/source/common/request_stream_grpc_client_impl.cc b/source/common/request_stream_grpc_client_impl.cc index dbde5ac2c..800bbd1be 100644 --- a/source/common/request_stream_grpc_client_impl.cc +++ b/source/common/request_stream_grpc_client_impl.cc @@ -14,6 +14,8 @@ #include "source/common/request_impl.h" +#include "absl/strings/str_cat.h" + namespace Nighthawk { using ::nighthawk::request_source::RequestSpecifier; @@ -41,7 +43,7 @@ void RequestStreamGrpcClientImpl::trySendRequest() { request.set_quantity(header_buffer_length_); stream_->sendMessage(request, false); in_flight_headers_ = header_buffer_length_; - ENVOY_LOG(trace, "send request: {}", request.DebugString()); + ENVOY_LOG(trace, "send request: {}", absl::StrCat(request)); } } @@ -142,7 +144,7 @@ RequestPtr RequestStreamGrpcClientImpl::maybeDequeue() { void RequestStreamGrpcClientImpl::emplaceMessage( std::unique_ptr&& message) { - ENVOY_LOG(trace, "message received: {}", message->DebugString()); + ENVOY_LOG(trace, "message received: {}", absl::StrCat(*message)); messages_.emplace(std::move(message)); } diff --git a/source/common/statistic_impl.cc b/source/common/statistic_impl.cc index 3fa99f10d..f7b7f26f8 100644 --- a/source/common/statistic_impl.cc +++ b/source/common/statistic_impl.cc @@ -9,6 +9,7 @@ #include "external/envoy/source/common/common/assert.h" #include "external/envoy/source/common/protobuf/utility.h" +#include "absl/strings/str_cat.h" #include "internal_proto/statistic/statistic.pb.h" namespace Nighthawk { @@ -29,7 +30,7 @@ static void setDurationFromNanos(Envoy::ProtobufWkt::Duration& mutable_duration, } // namespace std::string StatisticImpl::toString() const { - return toProto(SerializationDomain::RAW).DebugString(); + return absl::StrCat(toProto(SerializationDomain::RAW)); } nighthawk::client::Statistic StatisticImpl::toProto(SerializationDomain domain) const { diff --git a/source/distributor/nighthawk_distributor_client_impl.cc b/source/distributor/nighthawk_distributor_client_impl.cc index 5e43b6cb5..6a20559ea 100644 --- a/source/distributor/nighthawk_distributor_client_impl.cc +++ b/source/distributor/nighthawk_distributor_client_impl.cc @@ -2,6 +2,8 @@ #include "external/envoy/source/common/common/assert.h" +#include "absl/strings/str_cat.h" + namespace Nighthawk { absl::StatusOr NighthawkDistributorClientImpl::DistributedRequest( @@ -11,7 +13,7 @@ absl::StatusOr NighthawkDistributorClientImpl::D std::shared_ptr<::grpc::ClientReaderWriterInterface> stream(nighthawk_distributor_stub.DistributedRequestStream(&context)); - ENVOY_LOG_MISC(trace, "Write {}", distributed_request.DebugString()); + ENVOY_LOG_MISC(trace, "Write {}", absl::StrCat(distributed_request)); if (!stream->Write(distributed_request)) { return absl::UnavailableError( "Failed to write request to the Nighthawk Distributor gRPC channel."); @@ -25,7 +27,7 @@ absl::StatusOr NighthawkDistributorClientImpl::D RELEASE_ASSERT(!got_response, "Distributor Service has started responding with more than one message."); got_response = true; - ENVOY_LOG_MISC(trace, "Read {}", response.DebugString()); + ENVOY_LOG_MISC(trace, "Read {}", absl::StrCat(response)); } if (!got_response) { return absl::InternalError("Distributor Service did not send a gRPC response."); diff --git a/source/distributor/service_impl.cc b/source/distributor/service_impl.cc index f71224556..250dc8322 100644 --- a/source/distributor/service_impl.cc +++ b/source/distributor/service_impl.cc @@ -10,6 +10,8 @@ #include "api/distributor/distributor.pb.validate.h" +#include "absl/strings/str_cat.h" + namespace Nighthawk { namespace { @@ -87,7 +89,7 @@ grpc::Status NighthawkDistributorServiceImpl::DistributedRequestStream( nighthawk::DistributedRequest request; grpc::Status status = grpc::Status::OK; while (status.ok() && stream->Read(&request)) { - ENVOY_LOG(trace, "Inbound DistributedRequest {}", request.DebugString()); + ENVOY_LOG(trace, "Inbound DistributedRequest {}", absl::StrCat(request)); status = validateRequest(request); if (status.ok()) { std::tuple status_and_response = @@ -99,7 +101,7 @@ grpc::Status NighthawkDistributorServiceImpl::DistributedRequestStream( status = grpc::Status(grpc::StatusCode::INTERNAL, std::string("Failed to write DistributedResponse.")); } else { - ENVOY_LOG(trace, "Wrote DistributedResponse {}", response.DebugString()); + ENVOY_LOG(trace, "Wrote DistributedResponse {}", absl::StrCat(response)); } } else { ENVOY_LOG(error, "DistributedRequest invalid: ({}) '{}'", status.error_code(), diff --git a/source/server/configuration.cc b/source/server/configuration.cc index e9a06828d..798889695 100644 --- a/source/server/configuration.cc +++ b/source/server/configuration.cc @@ -11,6 +11,7 @@ #include "api/server/response_options.pb.validate.h" #include "absl/strings/numbers.h" +#include "absl/strings/str_cat.h" namespace Nighthawk { namespace Server { @@ -75,7 +76,7 @@ void validateResponseOptions(const nighthawk::server::ResponseOptions& response_ throw Envoy::EnvoyException( absl::StrCat("invalid configuration in nighthawk::server::ResponseOptions ", "cannot specify both response_headers and v3_response_headers ", - "configuration was: ", response_options.ShortDebugString())); + "configuration was: ", absl::StrCat(response_options))); } } diff --git a/source/sink/service_impl.cc b/source/sink/service_impl.cc index 785703eb8..820f260de 100644 --- a/source/sink/service_impl.cc +++ b/source/sink/service_impl.cc @@ -9,6 +9,8 @@ #include "source/sink/nighthawk_sink_client_impl.h" #include "source/sink/sink_impl.h" +#include "absl/strings/str_cat.h" + namespace Nighthawk { using ::Envoy::Protobuf::util::MessageDifferencer; @@ -21,7 +23,7 @@ grpc::Status SinkServiceImpl::StoreExecutionResponseStream( RELEASE_ASSERT(request_reader != nullptr, "stream == nullptr"); nighthawk::StoreExecutionRequest request; while (request_reader->Read(&request)) { - ENVOY_LOG(trace, "StoreExecutionResponseStream request {}", request.DebugString()); + ENVOY_LOG(trace, "StoreExecutionResponseStream request {}", absl::StrCat(request)); const absl::Status status = sink_->StoreExecutionResultPiece(request.execution_response()); if (!status.ok()) { return abslStatusToGrpcStatus(status); @@ -44,7 +46,7 @@ grpc::Status SinkServiceImpl::SinkRequestStream( nighthawk::SinkRequest request; RELEASE_ASSERT(stream != nullptr, "stream == nullptr"); while (stream->Read(&request)) { - ENVOY_LOG(trace, "Inbound SinkRequest {}", request.DebugString()); + ENVOY_LOG(trace, "Inbound SinkRequest {}", absl::StrCat(request)); absl::StatusOr> execution_responses = sink_->LoadExecutionResult(request.execution_id()); if (!execution_responses.status().ok()) { @@ -80,14 +82,14 @@ absl::Status mergeOutput(const nighthawk::client::Output& input_to_merge, if (!MessageDifferencer::Equivalent(input_to_merge.options(), merge_target.options())) { return absl::Status{absl::StatusCode::kInternal, fmt::format("Options divergence detected: {} vs {}.", - merge_target.options().DebugString(), - input_to_merge.options().DebugString())}; + absl::StrCat(merge_target.options()), + absl::StrCat(input_to_merge.options()))}; } if (!MessageDifferencer::Equivalent(input_to_merge.version(), merge_target.version())) { return absl::Status{absl::StatusCode::kInternal, fmt::format("Version divergence detected: {} vs {}.", - merge_target.version().DebugString(), - input_to_merge.version().DebugString())}; + absl::StrCat(merge_target.version()), + absl::StrCat(input_to_merge.version()))}; } } // Append all input results into our own results. diff --git a/test/adaptive_load/adaptive_load_client_main_test.cc b/test/adaptive_load/adaptive_load_client_main_test.cc index 49077f485..8d4ed64b1 100644 --- a/test/adaptive_load/adaptive_load_client_main_test.cc +++ b/test/adaptive_load/adaptive_load_client_main_test.cc @@ -18,6 +18,7 @@ #include "test/mocks/adaptive_load/mock_adaptive_load_controller.h" #include "test/test_common/environment.h" +#include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -327,7 +328,7 @@ TEST(AdaptiveLoadClientMainTest, WritesOutputProtoToFile) { .value(); nighthawk::adaptive_load::AdaptiveLoadSessionOutput golden_proto; Envoy::Protobuf::TextFormat::ParseFromString(golden_text, &golden_proto); - EXPECT_EQ(actual_outfile_contents, golden_proto.DebugString()); + EXPECT_EQ(actual_outfile_contents, absl::StrCat(golden_proto)); } TEST(AdaptiveLoadClientMainTest, DefaultsToInsecureConnection) { diff --git a/test/adaptive_load/adaptive_load_controller_test.cc b/test/adaptive_load/adaptive_load_controller_test.cc index c0c33ee97..922498d1c 100644 --- a/test/adaptive_load/adaptive_load_controller_test.cc +++ b/test/adaptive_load/adaptive_load_controller_test.cc @@ -42,6 +42,7 @@ #include "absl/container/flat_hash_map.h" #include "absl/status/status.h" +#include "absl/strings/str_cat.h" #include "absl/strings/str_join.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -171,7 +172,7 @@ TEST_F(AdaptiveLoadControllerImplFixture, CopiesThresholdSpecsIntoOutput) { MetricSpecWithThreshold actual_spec_with_threshold = output_or.value().metric_thresholds(0); EXPECT_TRUE( MessageDifferencer::Equivalent(actual_spec_with_threshold, spec.metric_thresholds(0))); - EXPECT_EQ(actual_spec_with_threshold.DebugString(), spec.metric_thresholds(0).DebugString()); + EXPECT_EQ(absl::StrCat(actual_spec_with_threshold), absl::StrCat(spec.metric_thresholds(0))); } TEST_F(AdaptiveLoadControllerImplFixture, TimesOutIfNeverConverged) { diff --git a/test/test_common/proto_matchers.h b/test/test_common/proto_matchers.h index 8809dea48..79602c6d1 100644 --- a/test/test_common/proto_matchers.h +++ b/test/test_common/proto_matchers.h @@ -4,6 +4,7 @@ #include "external/envoy/source/common/protobuf/protobuf.h" +#include "absl/strings/str_cat.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -26,9 +27,9 @@ MATCHER_P(EqualsProto, expected_proto, "is equal to the expected_proto") { if (!equal) { *result_listener << "\n" << "=======================Expected proto:===========================\n" - << expected_proto.DebugString() + << absl::StrCat(expected_proto) << "------------------is not equal to actual proto:------------------\n" - << arg.DebugString() + << absl::StrCat(arg) << "------------------------the diff is:-----------------------------\n" << diff << "=================================================================\n";