From d23552a6ad55576978dcd5cd187ddb4c32601c93 Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Thu, 24 Dec 2020 16:21:20 +0200 Subject: [PATCH 01/19] Add support for proto ListVale formatting Signed-off-by: Itamar Kaminski --- .../formatter/substitution_formatter.cc | 25 +++++++++------ .../common/formatter/substitution_formatter.h | 11 +++++++ .../formatter/substitution_formatter_test.cc | 32 +++++++++++++++++++ 3 files changed, 58 insertions(+), 10 deletions(-) diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 2f4737bf3fb4..4ff19b706934 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -149,23 +149,23 @@ StructFormatter::StructFormatter(const ProtobufWkt::Struct& format_mapping, bool bool omit_empty_values) : omit_empty_values_(omit_empty_values), preserve_types_(preserve_types), empty_value_(omit_empty_values_ ? EMPTY_STRING : DefaultUnspecifiedValueString), - struct_output_format_(toFormatMapValue(format_mapping)) {} + struct_output_format_(toFormatValue(format_mapping)) {} StructFormatter::StructFormatMapWrapper -StructFormatter::toFormatMapValue(const ProtobufWkt::Struct& struct_format) const { +StructFormatter::toFormatValue(const ProtobufWkt::Struct& struct_format) const { auto output = std::make_unique(); for (const auto& pair : struct_format.fields()) { switch (pair.second.kind_case()) { case ProtobufWkt::Value::kStringValue: - output->emplace(pair.first, toFormatStringValue(pair.second.string_value())); + output->emplace(pair.first, toFormatValue(pair.second.string_value())); break; case ProtobufWkt::Value::kStructValue: - output->emplace(pair.first, toFormatMapValue(pair.second.struct_value())); + output->emplace(pair.first, toFormatValue(pair.second.struct_value())); break; case ProtobufWkt::Value::kListValue: - output->emplace(pair.first, toFormatListValue(pair.second.list_value())); + output->emplace(pair.first, toFormatValue(pair.second.list_value())); break; default: @@ -174,23 +174,28 @@ StructFormatter::toFormatMapValue(const ProtobufWkt::Struct& struct_format) cons } } return {std::move(output)}; +} + +std::vector +StructFormatter::toFormatStringValue(const std::string& string_format) const { + return SubstitutionFormatParser::parse(string_format); }; StructFormatter::StructFormatListWrapper -StructFormatter::toFormatListValue(const ProtobufWkt::ListValue& list_value_format) const { +StructFormatter::toFormatValue(const ProtobufWkt::ListValue& list_value_format) const { auto output = std::make_unique(); for (const auto& value : list_value_format.values()) { switch (value.kind_case()) { case ProtobufWkt::Value::kStringValue: - output->emplace_back(toFormatStringValue(value.string_value())); + output->emplace_back(toFormatValue(value.string_value())); break; case ProtobufWkt::Value::kStructValue: - output->emplace_back(toFormatMapValue(value.struct_value())); + output->emplace_back(toFormatValue(value.struct_value())); break; case ProtobufWkt::Value::kListValue: - output->emplace_back(toFormatListValue(value.list_value())); + output->emplace_back(toFormatValue(value.list_value())); break; default: throw EnvoyException("Only string values, nested structs and list values are " @@ -201,7 +206,7 @@ StructFormatter::toFormatListValue(const ProtobufWkt::ListValue& list_value_form } std::vector -StructFormatter::toFormatStringValue(const std::string& string_format) const { +StructFormatter::toFormatValue(const std::string& string_format) const { return SubstitutionFormatParser::parse(string_format); }; diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index 19d6b68f19fb..63f990e10db1 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -162,15 +162,26 @@ class StructFormatter { StructFormatListPtr value_; }; +<<<<<<< HEAD +======= + template struct StructFormatMapVisitorHelper : Ts... { using Ts::operator()...; }; + template StructFormatMapVisitorHelper(Ts...) -> StructFormatMapVisitorHelper; +>>>>>>> 57c2668ab (Add support for proto ListVale formatting) using StructFormatMapVisitor = StructFormatMapVisitorHelper< const std::function&)>, const std::function, const std::function>; // Methods for building the format map. +<<<<<<< HEAD std::vector toFormatStringValue(const std::string& string_format) const; StructFormatMapWrapper toFormatMapValue(const ProtobufWkt::Struct& struct_format) const; StructFormatListWrapper toFormatListValue(const ProtobufWkt::ListValue& list_value_format) const; +======= + std::vector toFormatValue(const std::string& string_format) const; + StructFormatMapWrapper toFormatValue(const ProtobufWkt::Struct& struct_format) const; + StructFormatListWrapper toFormatValue(const ProtobufWkt::ListValue& list_value_format) const; +>>>>>>> 57c2668ab (Add support for proto ListVale formatting) // Methods for doing the actual formatting. ProtobufWkt::Value providersCallback(const std::vector& providers, diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc index 73b7ce9ecd99..da85ece8fbc0 100644 --- a/test/common/formatter/substitution_formatter_test.cc +++ b/test/common/formatter/substitution_formatter_test.cc @@ -1825,6 +1825,38 @@ TEST(SubstitutionFormatterTest, StructFormatterNestedObjectsTest) { EXPECT_TRUE(TestUtility::protoEqual(out_struct, expected)); } +TEST(SubstitutionFormatterTest, StructFormatterList) { + StreamInfo::MockStreamInfo stream_info; + Http::TestRequestHeaderMapImpl request_header; + Http::TestResponseHeaderMapImpl response_header; + Http::TestResponseTrailerMapImpl response_trailer; + std::string body; + + envoy::config::core::v3::Metadata metadata; + populateMetadataTestData(metadata); + absl::optional protocol = Http::Protocol::Http11; + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); + + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + list: + - plain_string: plain_string_value + - protocol: '%PROTOCOL%' + )EOF", + key_mapping); + StructFormatter formatter(key_mapping, false, false); + + const ProtobufWkt::Struct expected = TestUtility::jsonToStruct(R"EOF({ + "list": [ + { "plain_string": "plain_string_value" }, + { "protocol": "HTTP/1.1" }, + ] + })EOF"); + const ProtobufWkt::Struct out_struct = + formatter.format(request_header, response_header, response_trailer, stream_info, body); + EXPECT_TRUE(TestUtility::protoEqual(out_struct, expected)); +} + TEST(SubstitutionFormatterTest, StructFormatterSingleOperatorTest) { StreamInfo::MockStreamInfo stream_info; Http::TestRequestHeaderMapImpl request_header; From 6bafc9ea1c2feba67a1b27f1f9aa00df907bb75d Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Thu, 24 Dec 2020 16:46:15 +0200 Subject: [PATCH 02/19] Fix some method names Signed-off-by: Itamar Kaminski --- .../formatter/substitution_formatter.cc | 20 +++++++++---------- .../common/formatter/substitution_formatter.h | 9 --------- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 4ff19b706934..cf12239f8030 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -149,23 +149,23 @@ StructFormatter::StructFormatter(const ProtobufWkt::Struct& format_mapping, bool bool omit_empty_values) : omit_empty_values_(omit_empty_values), preserve_types_(preserve_types), empty_value_(omit_empty_values_ ? EMPTY_STRING : DefaultUnspecifiedValueString), - struct_output_format_(toFormatValue(format_mapping)) {} + struct_output_format_(toFormatMapValue(format_mapping)) {} StructFormatter::StructFormatMapWrapper -StructFormatter::toFormatValue(const ProtobufWkt::Struct& struct_format) const { +StructFormatter::toFormatMapValue(const ProtobufWkt::Struct& struct_format) const { auto output = std::make_unique(); for (const auto& pair : struct_format.fields()) { switch (pair.second.kind_case()) { case ProtobufWkt::Value::kStringValue: - output->emplace(pair.first, toFormatValue(pair.second.string_value())); + output->emplace(pair.first, toFormatStringValue(pair.second.string_value())); break; case ProtobufWkt::Value::kStructValue: - output->emplace(pair.first, toFormatValue(pair.second.struct_value())); + output->emplace(pair.first, toFormatMapValue(pair.second.struct_value())); break; case ProtobufWkt::Value::kListValue: - output->emplace(pair.first, toFormatValue(pair.second.list_value())); + output->emplace(pair.first, toFormatListValue(pair.second.list_value())); break; default: @@ -182,20 +182,20 @@ StructFormatter::toFormatStringValue(const std::string& string_format) const { }; StructFormatter::StructFormatListWrapper -StructFormatter::toFormatValue(const ProtobufWkt::ListValue& list_value_format) const { +StructFormatter::toFormatListValue(const ProtobufWkt::ListValue& list_value_format) const { auto output = std::make_unique(); for (const auto& value : list_value_format.values()) { switch (value.kind_case()) { case ProtobufWkt::Value::kStringValue: - output->emplace_back(toFormatValue(value.string_value())); + output->emplace_back(toFormatStringValue(value.string_value())); break; case ProtobufWkt::Value::kStructValue: - output->emplace_back(toFormatValue(value.struct_value())); + output->emplace_back(toFormatMapValue(value.struct_value())); break; case ProtobufWkt::Value::kListValue: - output->emplace_back(toFormatValue(value.list_value())); + output->emplace_back(toFormatListValue(value.list_value())); break; default: throw EnvoyException("Only string values, nested structs and list values are " @@ -206,7 +206,7 @@ StructFormatter::toFormatValue(const ProtobufWkt::ListValue& list_value_format) } std::vector -StructFormatter::toFormatValue(const std::string& string_format) const { +StructFormatter::toFormatStringValue(const std::string& string_format) const { return SubstitutionFormatParser::parse(string_format); }; diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index 63f990e10db1..05ea9140266c 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -162,26 +162,17 @@ class StructFormatter { StructFormatListPtr value_; }; -<<<<<<< HEAD -======= template struct StructFormatMapVisitorHelper : Ts... { using Ts::operator()...; }; template StructFormatMapVisitorHelper(Ts...) -> StructFormatMapVisitorHelper; ->>>>>>> 57c2668ab (Add support for proto ListVale formatting) using StructFormatMapVisitor = StructFormatMapVisitorHelper< const std::function&)>, const std::function, const std::function>; // Methods for building the format map. -<<<<<<< HEAD - std::vector toFormatStringValue(const std::string& string_format) const; - StructFormatMapWrapper toFormatMapValue(const ProtobufWkt::Struct& struct_format) const; - StructFormatListWrapper toFormatListValue(const ProtobufWkt::ListValue& list_value_format) const; -======= std::vector toFormatValue(const std::string& string_format) const; StructFormatMapWrapper toFormatValue(const ProtobufWkt::Struct& struct_format) const; StructFormatListWrapper toFormatValue(const ProtobufWkt::ListValue& list_value_format) const; ->>>>>>> 57c2668ab (Add support for proto ListVale formatting) // Methods for doing the actual formatting. ProtobufWkt::Value providersCallback(const std::vector& providers, From dfbe7081ab0b5411b5493c5e2f73c7030ccfb444 Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Tue, 5 Jan 2021 21:44:45 +0200 Subject: [PATCH 03/19] Restructure nested formatting tests Signed-off-by: Itamar Kaminski --- .../formatter/substitution_formatter_test.cc | 34 +------------------ 1 file changed, 1 insertion(+), 33 deletions(-) diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc index da85ece8fbc0..100a1218da02 100644 --- a/test/common/formatter/substitution_formatter_test.cc +++ b/test/common/formatter/substitution_formatter_test.cc @@ -1825,38 +1825,6 @@ TEST(SubstitutionFormatterTest, StructFormatterNestedObjectsTest) { EXPECT_TRUE(TestUtility::protoEqual(out_struct, expected)); } -TEST(SubstitutionFormatterTest, StructFormatterList) { - StreamInfo::MockStreamInfo stream_info; - Http::TestRequestHeaderMapImpl request_header; - Http::TestResponseHeaderMapImpl response_header; - Http::TestResponseTrailerMapImpl response_trailer; - std::string body; - - envoy::config::core::v3::Metadata metadata; - populateMetadataTestData(metadata); - absl::optional protocol = Http::Protocol::Http11; - EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); - - ProtobufWkt::Struct key_mapping; - TestUtility::loadFromYaml(R"EOF( - list: - - plain_string: plain_string_value - - protocol: '%PROTOCOL%' - )EOF", - key_mapping); - StructFormatter formatter(key_mapping, false, false); - - const ProtobufWkt::Struct expected = TestUtility::jsonToStruct(R"EOF({ - "list": [ - { "plain_string": "plain_string_value" }, - { "protocol": "HTTP/1.1" }, - ] - })EOF"); - const ProtobufWkt::Struct out_struct = - formatter.format(request_header, response_header, response_trailer, stream_info, body); - EXPECT_TRUE(TestUtility::protoEqual(out_struct, expected)); -} - TEST(SubstitutionFormatterTest, StructFormatterSingleOperatorTest) { StreamInfo::MockStreamInfo stream_info; Http::TestRequestHeaderMapImpl request_header; @@ -2099,7 +2067,7 @@ TEST(SubstitutionFormatterTest, StructFormatterTypedFilterStateTest) { fields.at("test_obj").struct_value().fields().at("inner_key").string_value()); } -// Test new specifier (PLAIN/TYPED) of FilterState. Ensure that after adding additional specifier, +// Test new specifier (PLAIN/TYPED) of FilterState. Ensure that after adding additiona specifier, // the FilterState can call the serializeAsProto or serializeAsString methods correctly. TEST(SubstitutionFormatterTest, FilterStateSpeciferTest) { Http::TestRequestHeaderMapImpl request_headers; From 8f57a556a35fe9d15740657e9a796c13ee8501e6 Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Tue, 5 Jan 2021 21:48:24 +0200 Subject: [PATCH 04/19] Fix typo Signed-off-by: Itamar Kaminski --- test/common/formatter/substitution_formatter_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc index 100a1218da02..73b7ce9ecd99 100644 --- a/test/common/formatter/substitution_formatter_test.cc +++ b/test/common/formatter/substitution_formatter_test.cc @@ -2067,7 +2067,7 @@ TEST(SubstitutionFormatterTest, StructFormatterTypedFilterStateTest) { fields.at("test_obj").struct_value().fields().at("inner_key").string_value()); } -// Test new specifier (PLAIN/TYPED) of FilterState. Ensure that after adding additiona specifier, +// Test new specifier (PLAIN/TYPED) of FilterState. Ensure that after adding additional specifier, // the FilterState can call the serializeAsProto or serializeAsString methods correctly. TEST(SubstitutionFormatterTest, FilterStateSpeciferTest) { Http::TestRequestHeaderMapImpl request_headers; From e6dd2201c26f1208944fc553a3beb2f893018c51 Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Wed, 13 Jan 2021 21:18:43 +0200 Subject: [PATCH 05/19] Fix GCC comnpilation Signed-off-by: Itamar Kaminski --- source/common/formatter/substitution_formatter.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index 05ea9140266c..4d23d9ad5b0d 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -162,8 +162,6 @@ class StructFormatter { StructFormatListPtr value_; }; - template struct StructFormatMapVisitorHelper : Ts... { using Ts::operator()...; }; - template StructFormatMapVisitorHelper(Ts...) -> StructFormatMapVisitorHelper; using StructFormatMapVisitor = StructFormatMapVisitorHelper< const std::function&)>, const std::function, From 7aebba8719316ffcbef2a1154beaab8e898f4590 Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Mon, 7 Dec 2020 19:28:48 +0200 Subject: [PATCH 06/19] generalizing gRPC access logger base class (GrpcAccessLoggel) Temporary commit Small fixes Signed-off-by: Itamar Kaminski Testing skeleton Signed-off-by: Itamar Kaminski Test skeleton Signed-off-by: Itamar Kaminski Tests and Access logger cache generalization Signed-off-by: Itamar Kaminski Tests done Signed-off-by: Itamar Kaminski Tests done after horrible merge Signed-off-by: Itamar Kaminski Uncomment something Signed-off-by: Itamar Kaminski Cleanups and dependency fixes Signed-off-by: Itamar Kaminski Format fix Signed-off-by: Itamar Kaminski --- source/extensions/access_loggers/common/BUILD | 1 + .../common/grpc_access_logger.h | 11 ++++++ .../common/grpc_access_logger_test.cc | 36 +++++++++++++++++++ 3 files changed, 48 insertions(+) diff --git a/source/extensions/access_loggers/common/BUILD b/source/extensions/access_loggers/common/BUILD index e7851e9da725..d7e449111640 100644 --- a/source/extensions/access_loggers/common/BUILD +++ b/source/extensions/access_loggers/common/BUILD @@ -34,6 +34,7 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/grpc:typed_async_client_lib", "//source/common/protobuf:utility_lib", + "//source/common/runtime:runtime_features_lib", "@com_google_absl//absl/types:optional", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], diff --git a/source/extensions/access_loggers/common/grpc_access_logger.h b/source/extensions/access_loggers/common/grpc_access_logger.h index 8598bfbaeb57..23e7d09796fb 100644 --- a/source/extensions/access_loggers/common/grpc_access_logger.h +++ b/source/extensions/access_loggers/common/grpc_access_logger.h @@ -12,6 +12,7 @@ #include "common/common/assert.h" #include "common/grpc/typed_async_client.h" #include "common/protobuf/utility.h" +#include "common/runtime/runtime_features.h" #include "absl/container/flat_hash_map.h" #include "absl/types/optional.h" @@ -247,8 +248,18 @@ class GrpcAccessLogger : public Detail::GrpcAccessLogger>>>>>> a9cbacc16 (generalizing gRPC access logger base class (GrpcAccessLoggel)) } const std::chrono::milliseconds buffer_flush_interval_msec_; diff --git a/test/extensions/access_loggers/common/grpc_access_logger_test.cc b/test/extensions/access_loggers/common/grpc_access_logger_test.cc index fc6a37e871df..1c2ce6b36249 100644 --- a/test/extensions/access_loggers/common/grpc_access_logger_test.cc +++ b/test/extensions/access_loggers/common/grpc_access_logger_test.cc @@ -230,6 +230,42 @@ TEST_F(GrpcAccessLogTest, WatermarksOverrun) { TestUtility::findCounter(stats_store_, "mock_access_log_prefix.logs_dropped")->value()); } +// Test legacy behavior of unbounded access logs. +TEST_F(GrpcAccessLogTest, WatermarksLegacy) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.disallow_unbounded_access_logs", "false"}}); + + initLogger(FlushInterval, 1); + + // Start a stream for the first log. + MockAccessLogStream stream; + AccessLogCallbacks* callbacks; + expectStreamStart(stream, &callbacks); + + EXPECT_CALL(stream, isAboveWriteBufferHighWatermark()) + .Times(AnyNumber()) + .WillRepeatedly(Return(true)); + + // Fail to flush, so the log stays buffered up. + EXPECT_CALL(stream, sendMessageRaw_(_, false)).Times(0); + logger_->log(mockHttpEntry()); + // Message should still be initialized. + EXPECT_EQ(1, logger_->numInits()); + EXPECT_EQ(1, + TestUtility::findCounter(stats_store_, "mock_access_log_prefix.logs_written")->value()); + EXPECT_EQ(0, + TestUtility::findCounter(stats_store_, "mock_access_log_prefix.logs_dropped")->value()); + + // As with the above test, try to log more. The log will not be dropped. + EXPECT_CALL(stream, sendMessageRaw_(_, _)).Times(0); + logger_->log(mockHttpEntry()); + EXPECT_EQ(2, + TestUtility::findCounter(stats_store_, "mock_access_log_prefix.logs_written")->value()); + EXPECT_EQ(0, + TestUtility::findCounter(stats_store_, "mock_access_log_prefix.logs_dropped")->value()); +} + // Test that stream failure is handled correctly. TEST_F(GrpcAccessLogTest, StreamFailure) { initLogger(FlushInterval, 0); From e73281dc992113ea5ae27324600e81bcb7e8ab14 Mon Sep 17 00:00:00 2001 From: antonio Date: Thu, 17 Dec 2020 08:59:38 -0800 Subject: [PATCH 07/19] event: Extract DispatcherBase interface (#14446) Signed-off-by: Antonio Vicente --- include/envoy/event/dispatcher.h | 412 +++++++++++++++---------------- 1 file changed, 200 insertions(+), 212 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 36e34f78bd1a..b9dd1ade0e5d 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -73,223 +73,211 @@ class DispatcherBase { uint32_t events) PURE; /** - * Allocates a timer. @see Timer for docs on how to use the timer. - * @param cb supplies the callback to invoke when the timer fires. + * Abstract event dispatching loop. */ - virtual Event::TimerPtr createTimer(TimerCb cb) PURE; - - /** - * Allocates a scaled timer. @see Timer for docs on how to use the timer. - * @param timer_type the type of timer to create. - * @param cb supplies the callback to invoke when the timer fires. - */ - virtual Event::TimerPtr createScaledTimer(Event::ScaledTimerType timer_type, TimerCb cb) PURE; - - /** - * Allocates a scaled timer. @see Timer for docs on how to use the timer. - * @param minimum the rule for computing the minimum value of the timer. - * @param cb supplies the callback to invoke when the timer fires. - */ - virtual Event::TimerPtr createScaledTimer(Event::ScaledTimerMinimum minimum, TimerCb cb) PURE; - - /** - * Allocates a schedulable callback. @see SchedulableCallback for docs on how to use the wrapped - * callback. - * @param cb supplies the callback to invoke when the SchedulableCallback is triggered on the - * event loop. - */ - virtual Event::SchedulableCallbackPtr createSchedulableCallback(std::function cb) PURE; - - /** - * Appends a tracked object to the current stack of tracked objects operating - * in the dispatcher. - * - * It's recommended to use ScopeTrackerScopeState to manage the object's tracking. If directly - * invoking, there needs to be a subsequent call to popTrackedObject(). - */ - virtual void pushTrackedObject(const ScopeTrackedObject* object) PURE; - - /** - * Removes the top of the stack of tracked object and asserts that it was expected. - */ - virtual void popTrackedObject(const ScopeTrackedObject* expected_object) PURE; - - /** - * Validates that an operation is thread-safe with respect to this dispatcher; i.e. that the - * current thread of execution is on the same thread upon which the dispatcher loop is running. - */ - virtual bool isThreadSafe() const PURE; - - /** - * Returns a recently cached MonotonicTime value. - */ - virtual MonotonicTime approximateMonotonicTime() const PURE; -}; - -/** - * Abstract event dispatching loop. - */ -class Dispatcher : public DispatcherBase { -public: - /** - * Returns the name that identifies this dispatcher, such as "worker_2" or "main_thread". - * @return const std::string& the name that identifies this dispatcher. - */ - virtual const std::string& name() PURE; - - /** - * Register a watchdog for this dispatcher. The dispatcher is responsible for touching the - * watchdog at least once per touch interval. Dispatcher implementations may choose to touch more - * often to avoid spurious miss events when processing long callback queues. - * @param min_touch_interval Touch interval for the watchdog. - */ - virtual void registerWatchdog(const Server::WatchDogSharedPtr& watchdog, - std::chrono::milliseconds min_touch_interval) PURE; - - /** - * Returns a time-source to use with this dispatcher. - */ - virtual TimeSource& timeSource() PURE; - - /** - * Initializes stats for this dispatcher. Note that this can't generally be done at construction - * time, since the main and worker thread dispatchers are constructed before - * ThreadLocalStoreImpl::initializeThreading. - * @param scope the scope to contain the new per-dispatcher stats created here. - * @param prefix the stats prefix to identify this dispatcher. If empty, the dispatcher will be - * identified by its name. - */ - virtual void initializeStats(Stats::Scope& scope, - const absl::optional& prefix = absl::nullopt) PURE; - - /** - * Clears any items in the deferred deletion queue. - */ - virtual void clearDeferredDeleteList() PURE; - - /** - * Wraps an already-accepted socket in an instance of Envoy's server Network::Connection. - * @param socket supplies an open file descriptor and connection metadata to use for the - * connection. Takes ownership of the socket. - * @param transport_socket supplies a transport socket to be used by the connection. - * @param stream_info info object for the server connection - * @return Network::ConnectionPtr a server connection that is owned by the caller. - */ - virtual Network::ServerConnectionPtr - createServerConnection(Network::ConnectionSocketPtr&& socket, - Network::TransportSocketPtr&& transport_socket, - StreamInfo::StreamInfo& stream_info) PURE; - - /** - * Creates an instance of Envoy's Network::ClientConnection. Does NOT initiate the connection; - * the caller must then call connect() on the returned Network::ClientConnection. - * @param address supplies the address to connect to. - * @param source_address supplies an address to bind to or nullptr if no bind is necessary. - * @param transport_socket supplies a transport socket to be used by the connection. - * @param options the socket options to be set on the underlying socket before anything is sent - * on the socket. - * @return Network::ClientConnectionPtr a client connection that is owned by the caller. - */ - virtual Network::ClientConnectionPtr - createClientConnection(Network::Address::InstanceConstSharedPtr address, - Network::Address::InstanceConstSharedPtr source_address, - Network::TransportSocketPtr&& transport_socket, - const Network::ConnectionSocket::OptionsSharedPtr& options) PURE; - - /** - * Creates an async DNS resolver. The resolver should only be used on the thread that runs this - * dispatcher. - * @param resolvers supplies the addresses of DNS resolvers that this resolver should use. If left - * empty, it will not use any specific resolvers, but use defaults (/etc/resolv.conf) - * @param use_tcp_for_dns_lookups if set to true, tcp will be used to perform dns lookups. - * Otherwise, udp is used. - * @return Network::DnsResolverSharedPtr that is owned by the caller. - */ - virtual Network::DnsResolverSharedPtr - createDnsResolver(const std::vector& resolvers, - bool use_tcp_for_dns_lookups) PURE; - - /** - * @return Filesystem::WatcherPtr a filesystem watcher owned by the caller. - */ - virtual Filesystem::WatcherPtr createFilesystemWatcher() PURE; - - /** - * Creates a listener on a specific port. - * @param socket supplies the socket to listen on. - * @param cb supplies the callbacks to invoke for listener events. - * @param bind_to_port controls whether the listener binds to a transport port or not. - * @param backlog_size controls listener pending connections backlog - * @return Network::ListenerPtr a new listener that is owned by the caller. - */ - virtual Network::ListenerPtr createListener(Network::SocketSharedPtr&& socket, - Network::TcpListenerCallbacks& cb, bool bind_to_port, - uint32_t backlog_size) PURE; - - /** - * Creates a logical udp listener on a specific port. - * @param socket supplies the socket to listen on. - * @param cb supplies the udp listener callbacks to invoke for listener events. - * @return Network::ListenerPtr a new listener that is owned by the caller. - */ - virtual Network::UdpListenerPtr createUdpListener(Network::SocketSharedPtr socket, - Network::UdpListenerCallbacks& cb) PURE; - /** - * Submits an item for deferred delete. @see DeferredDeletable. - */ - virtual void deferredDelete(DeferredDeletablePtr&& to_delete) PURE; - - /** - * Exits the event loop. - */ - virtual void exit() PURE; - - /** - * Listens for a signal event. Only a single dispatcher in the process can listen for signals. - * If more than one dispatcher calls this routine in the process the behavior is undefined. - * - * @param signal_num supplies the signal to listen on. - * @param cb supplies the callback to invoke when the signal fires. - * @return SignalEventPtr a signal event that is owned by the caller. - */ - virtual SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) PURE; - - /** - * Posts a functor to the dispatcher. This is safe cross thread. The functor runs in the context - * of the dispatcher event loop which may be on a different thread than the caller. - */ - virtual void post(PostCb callback) PURE; - - /** - * Runs the event loop. This will not return until exit() is called either from within a callback - * or from a different thread. - * @param type specifies whether to run in blocking mode (run() will not return until exit() is - * called) or non-blocking mode where only active events will be executed and then - * run() will return. - */ - enum class RunType { - Block, // Runs the event-loop until there are no pending events. - NonBlock, // Checks for any pending events to activate, executes them, - // then exits. Exits immediately if there are no pending or - // active events. - RunUntilExit // Runs the event-loop until loopExit() is called, blocking - // until there are pending or active events. + class Dispatcher : public DispatcherBase { + public: + /** + * Allocates a timer. @see Timer for docs on how to use the timer. + * @param cb supplies the callback to invoke when the timer fires. + */ + virtual Event::TimerPtr createTimer(TimerCb cb) PURE; + + /** + * Allocates a schedulable callback. @see SchedulableCallback for docs on how to use the wrapped + * callback. + * @param cb supplies the callback to invoke when the SchedulableCallback is triggered on the + * event loop. + */ + virtual Event::SchedulableCallbackPtr createSchedulableCallback(std::function cb) PURE; + + /** + * Sets a tracked object, which is currently operating in this Dispatcher. + * This should be cleared with another call to setTrackedObject() when the object is done doing + * work. Calling setTrackedObject(nullptr) results in no object being tracked. + * + * This is optimized for performance, to avoid allocation where we do scoped object tracking. + * + * @return The previously tracked object or nullptr if there was none. + */ + virtual const ScopeTrackedObject* setTrackedObject(const ScopeTrackedObject* object) PURE; + + /** + * Validates that an operation is thread-safe with respect to this dispatcher; i.e. that the + * current thread of execution is on the same thread upon which the dispatcher loop is running. + */ + virtual bool isThreadSafe() const PURE; + + /** + * Returns a recently cached MonotonicTime value. + */ + virtual MonotonicTime approximateMonotonicTime() const PURE; }; - virtual void run(RunType type) PURE; /** - * Returns a factory which connections may use for watermark buffer creation. - * @return the watermark buffer factory for this dispatcher. + * Abstract event dispatching loop. */ - virtual Buffer::WatermarkFactory& getWatermarkFactory() PURE; - - /** - * Updates approximate monotonic time to current value. - */ - virtual void updateApproximateMonotonicTime() PURE; -}; + class Dispatcher : public DispatcherBase { + public: + /** + * Returns the name that identifies this dispatcher, such as "worker_2" or "main_thread". + * @return const std::string& the name that identifies this dispatcher. + */ + virtual const std::string& name() PURE; + + /** + * Register a watchdog for this dispatcher. The dispatcher is responsible for touching the + * watchdog at least once per touch interval. Dispatcher implementations may choose to touch + * more often to avoid spurious miss events when processing long callback queues. + * @param min_touch_interval Touch interval for the watchdog. + */ + virtual void registerWatchdog(const Server::WatchDogSharedPtr& watchdog, + std::chrono::milliseconds min_touch_interval) PURE; + + /** + * Returns a time-source to use with this dispatcher. + */ + virtual TimeSource& timeSource() PURE; + + /** + * Initializes stats for this dispatcher. Note that this can't generally be done at construction + * time, since the main and worker thread dispatchers are constructed before + * ThreadLocalStoreImpl::initializeThreading. + * @param scope the scope to contain the new per-dispatcher stats created here. + * @param prefix the stats prefix to identify this dispatcher. If empty, the dispatcher will be + * identified by its name. + */ + virtual void initializeStats(Stats::Scope& scope, + const absl::optional& prefix = absl::nullopt) PURE; + + /** + * Clears any items in the deferred deletion queue. + */ + virtual void clearDeferredDeleteList() PURE; + + /** + * Wraps an already-accepted socket in an instance of Envoy's server Network::Connection. + * @param socket supplies an open file descriptor and connection metadata to use for the + * connection. Takes ownership of the socket. + * @param transport_socket supplies a transport socket to be used by the connection. + * @param stream_info info object for the server connection + * @return Network::ConnectionPtr a server connection that is owned by the caller. + */ + virtual Network::ServerConnectionPtr + createServerConnection(Network::ConnectionSocketPtr&& socket, + Network::TransportSocketPtr&& transport_socket, + StreamInfo::StreamInfo& stream_info) PURE; + + /** + * Creates an instance of Envoy's Network::ClientConnection. Does NOT initiate the connection; + * the caller must then call connect() on the returned Network::ClientConnection. + * @param address supplies the address to connect to. + * @param source_address supplies an address to bind to or nullptr if no bind is necessary. + * @param transport_socket supplies a transport socket to be used by the connection. + * @param options the socket options to be set on the underlying socket before anything is sent + * on the socket. + * @return Network::ClientConnectionPtr a client connection that is owned by the caller. + */ + virtual Network::ClientConnectionPtr + createClientConnection(Network::Address::InstanceConstSharedPtr address, + Network::Address::InstanceConstSharedPtr source_address, + Network::TransportSocketPtr&& transport_socket, + const Network::ConnectionSocket::OptionsSharedPtr& options) PURE; + + /** + * Creates an async DNS resolver. The resolver should only be used on the thread that runs this + * dispatcher. + * @param resolvers supplies the addresses of DNS resolvers that this resolver should use. If + * left empty, it will not use any specific resolvers, but use defaults (/etc/resolv.conf) + * @param use_tcp_for_dns_lookups if set to true, tcp will be used to perform dns lookups. + * Otherwise, udp is used. + * @return Network::DnsResolverSharedPtr that is owned by the caller. + */ + virtual Network::DnsResolverSharedPtr + createDnsResolver(const std::vector& resolvers, + bool use_tcp_for_dns_lookups) PURE; + + /** + * @return Filesystem::WatcherPtr a filesystem watcher owned by the caller. + */ + virtual Filesystem::WatcherPtr createFilesystemWatcher() PURE; + + /** + * Creates a listener on a specific port. + * @param socket supplies the socket to listen on. + * @param cb supplies the callbacks to invoke for listener events. + * @param bind_to_port controls whether the listener binds to a transport port or not. + * @param backlog_size controls listener pending connections backlog + * @return Network::ListenerPtr a new listener that is owned by the caller. + */ + virtual Network::ListenerPtr createListener(Network::SocketSharedPtr&& socket, + Network::TcpListenerCallbacks& cb, + bool bind_to_port, uint32_t backlog_size) PURE; + + /** + * Creates a logical udp listener on a specific port. + * @param socket supplies the socket to listen on. + * @param cb supplies the udp listener callbacks to invoke for listener events. + * @return Network::ListenerPtr a new listener that is owned by the caller. + */ + virtual Network::UdpListenerPtr createUdpListener(Network::SocketSharedPtr socket, + Network::UdpListenerCallbacks& cb) PURE; + /** + * Submits an item for deferred delete. @see DeferredDeletable. + */ + virtual void deferredDelete(DeferredDeletablePtr&& to_delete) PURE; + + /** + * Exits the event loop. + */ + virtual void exit() PURE; + + /** + * Listens for a signal event. Only a single dispatcher in the process can listen for signals. + * If more than one dispatcher calls this routine in the process the behavior is undefined. + * + * @param signal_num supplies the signal to listen on. + * @param cb supplies the callback to invoke when the signal fires. + * @return SignalEventPtr a signal event that is owned by the caller. + */ + virtual SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) PURE; + + /** + * Posts a functor to the dispatcher. This is safe cross thread. The functor runs in the context + * of the dispatcher event loop which may be on a different thread than the caller. + */ + virtual void post(PostCb callback) PURE; + + /** + * Runs the event loop. This will not return until exit() is called either from within a + * callback or from a different thread. + * @param type specifies whether to run in blocking mode (run() will not return until exit() is + * called) or non-blocking mode where only active events will be executed and then + * run() will return. + */ + enum class RunType { + Block, // Runs the event-loop until there are no pending events. + NonBlock, // Checks for any pending events to activate, executes them, + // then exits. Exits immediately if there are no pending or + // active events. + RunUntilExit // Runs the event-loop until loopExit() is called, blocking + // until there are pending or active events. + }; + virtual void run(RunType type) PURE; + + /** + * Returns a factory which connections may use for watermark buffer creation. + * @return the watermark buffer factory for this dispatcher. + */ + virtual Buffer::WatermarkFactory& getWatermarkFactory() PURE; + + /** + * Updates approximate monotonic time to current value. + */ + virtual void updateApproximateMonotonicTime() PURE; + }; -using DispatcherPtr = std::unique_ptr; + using DispatcherPtr = std::unique_ptr; } // namespace Event -} // namespace Envoy +} // namespace Event From d5289ff868292922b278fe19f26a3a6444364708 Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Thu, 17 Dec 2020 20:45:11 +0200 Subject: [PATCH 08/19] Add OTLP proto dependencies Signed-off-by: Itamar Kaminski --- api/bazel/repository_locations.bzl | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/api/bazel/repository_locations.bzl b/api/bazel/repository_locations.bzl index d15978e7dd6c..903dd8a85ba8 100644 --- a/api/bazel/repository_locations.bzl +++ b/api/bazel/repository_locations.bzl @@ -107,4 +107,15 @@ REPOSITORY_LOCATIONS_SPEC = dict( release_date = "2020-07-29", use_category = ["api"], ), + opentelemetry_proto = dict( + project_name = "OpenTelemetry Proto", + project_desc = "Language Independent Interface Types For OpenTelemetry", + project_url = "https://github.com/open-telemetry/opentelemetry-proto", + version = "0.6.0", + sha256 = "08f090570e0a112bfae276ba37e9c45bf724b64d902a7a001db33123b840ebd6", + strip_prefix = "opentelemetry-proto-{version}", + urls = ["https://github.com/open-telemetry/opentelemetry-proto/archive/v{version}.tar.gz"], + release_date = "2020-10-29", + use_category = ["api"], + ), ) From 090a926ccc47b7e2462c145c236a1dce6e4834ad Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Mon, 21 Dec 2020 19:21:35 +0200 Subject: [PATCH 09/19] OTLP gRPC access logger (skeleton) Signed-off-by: Itamar Kaminski --- api/bazel/repositories.bzl | 26 +++ generated_api_shadow/bazel/repositories.bzl | 26 +++ .../bazel/repository_locations.bzl | 11 ++ source/extensions/access_loggers/grpc/BUILD | 19 ++ .../grpc/grpc_ot_access_log_impl.cc | 75 +++++++ .../grpc/grpc_ot_access_log_impl.h | 84 ++++++++ test/extensions/access_loggers/grpc/BUILD | 18 ++ .../grpc/grpc_ot_access_log_impl_test.cc | 187 ++++++++++++++++++ 8 files changed, 446 insertions(+) create mode 100644 source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.cc create mode 100644 source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h create mode 100644 test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc diff --git a/api/bazel/repositories.bzl b/api/bazel/repositories.bzl index 983f15967b28..1155b1d0cce2 100644 --- a/api/bazel/repositories.bzl +++ b/api/bazel/repositories.bzl @@ -44,6 +44,10 @@ def api_dependencies(): name = "com_github_apache_skywalking_data_collect_protocol", build_file_content = SKYWALKING_DATA_COLLECT_PROTOCOL_BUILD_CONTENT, ) + external_http_archive( + name = "opentelemetry_proto", + build_file_content = OPENTELEMETRY_LOGS_BUILD_CONTENT, + ) PROMETHEUSMETRICS_BUILD_CONTENT = """ load("@envoy_api//bazel:api_build_system.bzl", "api_cc_py_proto_library") @@ -132,3 +136,25 @@ go_proto_library( visibility = ["//visibility:public"], ) """ + +OPENTELEMETRY_LOGS_BUILD_CONTENT = """ +load("@rules_proto//proto:defs.bzl", "proto_library") +load("@rules_cc//cc:defs.bzl", "cc_proto_library") + +proto_library( + name = "logs", + srcs = [ + "opentelemetry/proto/collector/logs/v1/logs_service.proto", + "opentelemetry/proto/common/v1/common.proto", + "opentelemetry/proto/logs/v1/logs.proto", + "opentelemetry/proto/resource/v1/resource.proto", + ], + visibility = ["//visibility:public"], +) + +cc_proto_library( + name = "logs_cc_proto", + deps = [":logs"], + visibility = ["//visibility:public"], +) +""" diff --git a/generated_api_shadow/bazel/repositories.bzl b/generated_api_shadow/bazel/repositories.bzl index 983f15967b28..1155b1d0cce2 100644 --- a/generated_api_shadow/bazel/repositories.bzl +++ b/generated_api_shadow/bazel/repositories.bzl @@ -44,6 +44,10 @@ def api_dependencies(): name = "com_github_apache_skywalking_data_collect_protocol", build_file_content = SKYWALKING_DATA_COLLECT_PROTOCOL_BUILD_CONTENT, ) + external_http_archive( + name = "opentelemetry_proto", + build_file_content = OPENTELEMETRY_LOGS_BUILD_CONTENT, + ) PROMETHEUSMETRICS_BUILD_CONTENT = """ load("@envoy_api//bazel:api_build_system.bzl", "api_cc_py_proto_library") @@ -132,3 +136,25 @@ go_proto_library( visibility = ["//visibility:public"], ) """ + +OPENTELEMETRY_LOGS_BUILD_CONTENT = """ +load("@rules_proto//proto:defs.bzl", "proto_library") +load("@rules_cc//cc:defs.bzl", "cc_proto_library") + +proto_library( + name = "logs", + srcs = [ + "opentelemetry/proto/collector/logs/v1/logs_service.proto", + "opentelemetry/proto/common/v1/common.proto", + "opentelemetry/proto/logs/v1/logs.proto", + "opentelemetry/proto/resource/v1/resource.proto", + ], + visibility = ["//visibility:public"], +) + +cc_proto_library( + name = "logs_cc_proto", + deps = [":logs"], + visibility = ["//visibility:public"], +) +""" diff --git a/generated_api_shadow/bazel/repository_locations.bzl b/generated_api_shadow/bazel/repository_locations.bzl index d15978e7dd6c..903dd8a85ba8 100644 --- a/generated_api_shadow/bazel/repository_locations.bzl +++ b/generated_api_shadow/bazel/repository_locations.bzl @@ -107,4 +107,15 @@ REPOSITORY_LOCATIONS_SPEC = dict( release_date = "2020-07-29", use_category = ["api"], ), + opentelemetry_proto = dict( + project_name = "OpenTelemetry Proto", + project_desc = "Language Independent Interface Types For OpenTelemetry", + project_url = "https://github.com/open-telemetry/opentelemetry-proto", + version = "0.6.0", + sha256 = "08f090570e0a112bfae276ba37e9c45bf724b64d902a7a001db33123b840ebd6", + strip_prefix = "opentelemetry-proto-{version}", + urls = ["https://github.com/open-telemetry/opentelemetry-proto/archive/v{version}.tar.gz"], + release_date = "2020-10-29", + use_category = ["api"], + ), ) diff --git a/source/extensions/access_loggers/grpc/BUILD b/source/extensions/access_loggers/grpc/BUILD index b958ccba7995..09fdd1e22958 100644 --- a/source/extensions/access_loggers/grpc/BUILD +++ b/source/extensions/access_loggers/grpc/BUILD @@ -24,6 +24,25 @@ envoy_cc_library( ], ) + + +envoy_cc_library( + name = "grpc_ot_access_log_lib", + srcs = ["grpc_ot_access_log_impl.cc"], + hdrs = ["grpc_ot_access_log_impl.h"], + deps = [ + "//include/envoy/event:dispatcher_interface", + "//include/envoy/grpc:async_client_manager_interface", + "//include/envoy/thread_local:thread_local_interface", + "//source/common/config:utility_lib", + "//source/common/grpc:typed_async_client_lib", + "//source/extensions/access_loggers/common:grpc_access_logger", + "@envoy_api//envoy/extensions/access_loggers/grpc/v3:pkg_cc_proto", + "@opentelemetry_proto//:logs_cc_proto", + ], +) + + envoy_cc_library( name = "grpc_access_log_lib", srcs = ["grpc_access_log_impl.cc"], diff --git a/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.cc b/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.cc new file mode 100644 index 000000000000..8ab753641615 --- /dev/null +++ b/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.cc @@ -0,0 +1,75 @@ +#include "extensions/access_loggers/grpc/grpc_ot_access_log_impl.h" + +#include "opentelemetry/proto/collector/logs/v1/logs_service.pb.h" +#include "opentelemetry/proto/common/v1/common.pb.h" +#include "opentelemetry/proto/logs/v1/logs.pb.h" +#include "opentelemetry/proto/resource/v1/resource.pb.h" + +#include "envoy/extensions/access_loggers/grpc/v3/als.pb.h" +#include "envoy/grpc/async_client_manager.h" + +#include "common/config/utility.h" +#include "common/grpc/typed_async_client.h" + +const char GRPC_LOG_STATS_PREFIX[] = "access_logs.grpc_ot_access_log."; + +namespace Envoy { +namespace Extensions { +namespace AccessLoggers { +namespace GrpcCommon { + +GrpcOpenTelemetryAccessLoggerImpl::GrpcOpenTelemetryAccessLoggerImpl( + Grpc::RawAsyncClientPtr&& client, std::chrono::milliseconds buffer_flush_interval_msec, + uint64_t max_buffer_size_bytes, Event::Dispatcher& dispatcher, Stats::Scope& scope, + envoy::config::core::v3::ApiVersion transport_api_version) + : GrpcAccessLogger( + std::move(client), buffer_flush_interval_msec, max_buffer_size_bytes, dispatcher, scope, + GRPC_LOG_STATS_PREFIX, + Grpc::VersionedMethods("opentelemetry.proto.collector.logs.v1.LogsService.Export", + "opentelemetry.proto.collector.logs.v1.LogsService.Export") + .getMethodDescriptorForVersion(transport_api_version), + transport_api_version), + root_(initRoot()) {} + +InstrumentationLibraryLogs GrpcOpenTelemetryAccessLoggerImpl::initRoot() { + InstrumentationLibraryLogs root; + root.mutable_instrumentation_library()->set_name("envoy"); + root.mutable_instrumentation_library()->set_version("v3"); // TODO: Get real API version. + return root; +} + +void GrpcOpenTelemetryAccessLoggerImpl::addEntry( + opentelemetry::proto::logs::v1::LogRecord&& entry) { + root_->mutable_logs()->Add(std::move(entry)); +} + +void GrpcOpenTelemetryAccessLoggerImpl::addEntry( + opentelemetry::proto::logs::v1::ResourceLogs&& entry) { + (void)entry; +} + +bool GrpcOpenTelemetryAccessLoggerImpl::isEmpty() { return root_->logs().empty(); } + +void GrpcOpenTelemetryAccessLoggerImpl::initMessage() { + fmt::print("iitamark init before\n"); + // See comment about structure of repeated fields. + message_.add_resource_logs()->mutable_instrumentation_library_logs()->Add( + opentelemetry::proto::logs::v1::InstrumentationLibraryLogs(root_)); + // root_->mutable_instrumentation_library()->set_name("envoy"); + // root_->mutable_instrumentation_library()->set_version("v3"); // GetAPI. + fmt::print("iitamark init after\n"); +} + +GrpcOpenTelemetryAccessLoggerImpl::SharedPtr GrpcOpenTelemetryAccessLoggerCacheImpl::createLogger( + const envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig& config, + Grpc::RawAsyncClientPtr&& client, std::chrono::milliseconds buffer_flush_interval_msec, + uint64_t max_buffer_size_bytes, Event::Dispatcher& dispatcher, Stats::Scope& scope) { + return std::make_shared( + std::move(client), buffer_flush_interval_msec, max_buffer_size_bytes, dispatcher, scope, + Config::Utility::getAndCheckTransportVersion(config)); +} + +} // namespace GrpcCommon +} // namespace AccessLoggers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h b/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h new file mode 100644 index 000000000000..f019731309fd --- /dev/null +++ b/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h @@ -0,0 +1,84 @@ +#pragma once + +#include + +#include "opentelemetry/proto/collector/logs/v1/logs_service.pb.h" +#include "opentelemetry/proto/common/v1/common.pb.h" +#include "opentelemetry/proto/logs/v1/logs.pb.h" +#include "opentelemetry/proto/resource/v1/resource.pb.h" + +#include "envoy/event/dispatcher.h" +#include "envoy/extensions/access_loggers/grpc/v3/als.pb.h" +#include "envoy/grpc/async_client_manager.h" +#include "envoy/thread_local/thread_local.h" + +#include "extensions/access_loggers/common/grpc_access_logger.h" + +namespace Envoy { +namespace Extensions { +namespace AccessLoggers { +namespace GrpcCommon { + +// Note: OpenTelemetry protos are extra flexible and used also in the OT collector for batching and +// so forth. As a result, some fields are repeated, but for our use case we assume the following +// structure: +// ExportLogsServiceRequest -> (single) ResourceLogs -> (single) InstrumentationLibrary -> +// (repeated) LogRecord. +class GrpcOpenTelemetryAccessLoggerImpl + : public Common::GrpcAccessLogger< + opentelemetry::proto::logs::v1::LogRecord, + opentelemetry::proto::logs::v1::ResourceLogs /*TCP*/, + opentelemetry::proto::collector::logs::v1::ExportLogsServiceRequest, + opentelemetry::proto::collector::logs::v1::ExportLogsServiceResponse> { +public: + GrpcOpenTelemetryAccessLoggerImpl(Grpc::RawAsyncClientPtr&& client, + std::chrono::milliseconds buffer_flush_interval_msec, + uint64_t max_buffer_size_bytes, Event::Dispatcher& dispatcher, + Stats::Scope& scope, + envoy::config::core::v3::ApiVersion transport_api_version); + +private: + // Explanation. + static InstrumentationLibraryLogs initRoot() const; + // Extensions::AccessLoggers::GrpcCommon::GrpcAccessLogger + void addEntry(opentelemetry::proto::logs::v1::LogRecord&& entry) override; + void addEntry(opentelemetry::proto::logs::v1::ResourceLogs&& entry) override; + bool isEmpty() override; + void initMessage() override; + + // Root for adding new log entries (see comment above about structure). + const opentelemetry::proto::logs::v1::InstrumentationLibraryLogs root_; +}; + +class GrpcOpenTelemetryAccessLoggerCacheImpl + : public Common::GrpcAccessLoggerCache< + GrpcOpenTelemetryAccessLoggerImpl, + envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig> { +public: + // Inherited constructor. + using Common::GrpcAccessLoggerCache::GrpcAccessLoggerCache; + +private: + // Common::GrpcAccessLoggerCache + GrpcOpenTelemetryAccessLoggerImpl::SharedPtr + createLogger(const envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig& config, + Grpc::RawAsyncClientPtr&& client, + std::chrono::milliseconds buffer_flush_interval_msec, uint64_t max_buffer_size_bytes, + Event::Dispatcher& dispatcher, Stats::Scope& scope) override; +}; + +/** + * Aliases for class interfaces for mock definitions. + */ +using GrpcAccessLogger = GrpcOpenTelemetryAccessLoggerImpl::Interface; +using GrpcAccessLoggerSharedPtr = GrpcAccessLogger::SharedPtr; + +using GrpcAccessLoggerCache = GrpcOpenTelemetryAccessLoggerCacheImpl::Interface; +using GrpcAccessLoggerCacheSharedPtr = GrpcAccessLoggerCache::SharedPtr; + +} // namespace GrpcCommon +} // namespace AccessLoggers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/access_loggers/grpc/BUILD b/test/extensions/access_loggers/grpc/BUILD index 535fb3408600..7f190a356368 100644 --- a/test/extensions/access_loggers/grpc/BUILD +++ b/test/extensions/access_loggers/grpc/BUILD @@ -112,3 +112,21 @@ envoy_extension_cc_test( "@envoy_api//envoy/service/accesslog/v3:pkg_cc_proto", ], ) + + +envoy_extension_cc_test( + name = "grpc_ot_access_log_impl_test", + srcs = ["grpc_ot_access_log_impl_test.cc"], + extension_name = "envoy.access_loggers.http_grpc", + deps = [ + "//source/common/buffer:zero_copy_input_stream_lib", + "//source/extensions/access_loggers/grpc:grpc_ot_access_log_lib", + "//test/mocks/grpc:grpc_mocks", + "//test/mocks/stats:stats_mocks", + "//test/mocks/thread_local:thread_local_mocks", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/access_loggers/grpc/v3:pkg_cc_proto", + "@opentelemetry_proto//:logs_cc_proto", + ], +) + diff --git a/test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc b/test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc new file mode 100644 index 000000000000..016b4a90dc0b --- /dev/null +++ b/test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc @@ -0,0 +1,187 @@ +#include + +#include "opentelemetry/proto/collector/logs/v1/logs_service.pb.h" +#include "opentelemetry/proto/common/v1/common.pb.h" +#include "opentelemetry/proto/logs/v1/logs.pb.h" +#include "opentelemetry/proto/resource/v1/resource.pb.h" + +#include "envoy/config/core/v3/grpc_service.pb.h" +#include "envoy/extensions/access_loggers/grpc/v3/als.pb.h" + +#include "common/buffer/zero_copy_input_stream_impl.h" + +#include "extensions/access_loggers/grpc/grpc_ot_access_log_impl.h" + +#include "test/mocks/grpc/mocks.h" +#include "test/mocks/stats/mocks.h" +#include "test/mocks/thread_local/mocks.h" + +using testing::_; +using testing::Invoke; +using testing::NiceMock; +using testing::Return; + +// opentelemetry::proto::logs::v1::LogRecord, +// opentelemetry::proto::logs::v1::ResourceLogs /*TCP*/, +// opentelemetry::proto::collector::logs::v1::ExportLogsServiceRequest, +// opentelemetry::proto::collector::logs::v1::ExportLogsServiceResponse> + +namespace Envoy { +namespace Extensions { +namespace AccessLoggers { +namespace GrpcCommon { +namespace { + +constexpr std::chrono::milliseconds FlushInterval(10); +constexpr int BUFFER_SIZE_BYTES = 0; + +// A helper test class to mock and intercept GrpcOpenTelemetryAccessLoggerImpl streams. +class GrpcOpenTelemetryAccessLoggerImplTestHelper { +public: + using MockAccessLogStream = Grpc::MockAsyncStream; + using AccessLogCallbacks = Grpc::AsyncStreamCallbacks< + opentelemetry::proto::collector::logs::v1::ExportLogsServiceResponse>; + + GrpcOpenTelemetryAccessLoggerImplTestHelper(Grpc::MockAsyncClient* async_client) { + EXPECT_CALL(*async_client, startRaw(_, _, _, _)) + .WillOnce( + Invoke([this](absl::string_view, absl::string_view, Grpc::RawAsyncStreamCallbacks& cbs, + const Http::AsyncClient::StreamOptions&) { + this->callbacks_ = dynamic_cast(&cbs); + return &this->stream_; + })); + } + + void expectStreamMessage(const std::string& expected_message_yaml) { + opentelemetry::proto::collector::logs::v1::ExportLogsServiceRequest expected_message; + TestUtility::loadFromYaml(expected_message_yaml, expected_message); + EXPECT_CALL(stream_, isAboveWriteBufferHighWatermark()).WillOnce(Return(false)); + EXPECT_CALL(stream_, sendMessageRaw_(_, false)) + .WillOnce(Invoke([expected_message](Buffer::InstancePtr& request, bool) { + opentelemetry::proto::collector::logs::v1::ExportLogsServiceRequest message; + Buffer::ZeroCopyInputStreamImpl request_stream(std::move(request)); + EXPECT_TRUE(message.ParseFromZeroCopyStream(&request_stream)); + EXPECT_EQ(message.DebugString(), expected_message.DebugString()); + })); + } + +private: + MockAccessLogStream stream_; + AccessLogCallbacks* callbacks_; +}; + +class GrpcOpenTelemetryAccessLoggerImplTest : public testing::Test { +public: + GrpcOpenTelemetryAccessLoggerImplTest() + : async_client_(new Grpc::MockAsyncClient), timer_(new Event::MockTimer(&dispatcher_)), + grpc_access_logger_impl_test_helper_(async_client_) { + EXPECT_CALL(*timer_, enableTimer(_, _)); + fmt::print("before"); + logger_ = std::make_unique( + Grpc::RawAsyncClientPtr{async_client_}, FlushInterval, BUFFER_SIZE_BYTES, dispatcher_, + stats_store_, envoy::config::core::v3::ApiVersion::V3); + fmt::print("after"); + } + + Grpc::MockAsyncClient* async_client_; + Stats::IsolatedStoreImpl stats_store_; + Event::MockDispatcher dispatcher_; + Event::MockTimer* timer_; + std::unique_ptr logger_; + GrpcOpenTelemetryAccessLoggerImplTestHelper grpc_access_logger_impl_test_helper_; +}; + +TEST_F(GrpcOpenTelemetryAccessLoggerImplTest, LogHttp) { + grpc_access_logger_impl_test_helper_.expectStreamMessage(R"EOF( + resource_logs: + - instrumentation_library_logs: + - instrumentation_library: + name: "envoy" + version: "v3" + logs: + - severity_text: "iitamark" + )EOF"); + opentelemetry::proto::logs::v1::LogRecord entry; + entry.set_severity_text("iitamark"); + logger_->log(opentelemetry::proto::logs::v1::LogRecord(entry)); +} + +// TEST_F(GrpcOpenTelemetryAccessLoggerImplTest, LogTcp) { +// grpc_access_logger_impl_test_helper_.expectStreamMessage(R"EOF( +// identifier: +// node: +// id: node_name +// cluster: cluster_name +// locality: +// zone: zone_name +// log_name: test_log_name +// tcp_logs: +// log_entry: +// common_properties: +// sample_rate: 1.0 +// )EOF"); +// envoy::data::accesslog::v3::TCPAccessLogEntry tcp_entry; +// tcp_entry.mutable_common_properties()->set_sample_rate(1); +// logger_->log(envoy::data::accesslog::v3::TCPAccessLogEntry(tcp_entry)); +// } + +// class GrpcOpenTelemetryAccessLoggerCacheImplTest : public testing::Test { +// public: +// GrpcOpenTelemetryAccessLoggerCacheImplTest() +// : async_client_(new Grpc::MockAsyncClient), factory_(new Grpc::MockAsyncClientFactory), +// logger_cache_(async_client_manager_, scope_, tls_), +// grpc_access_logger_impl_test_helper_(async_client_) { +// EXPECT_CALL(async_client_manager_, factoryForGrpcService(_, _, false)) +// .WillOnce(Invoke([this](const envoy::config::core::v3::GrpcService&, Stats::Scope&, bool) +// { +// EXPECT_CALL(*factory_, create()).WillOnce(Invoke([this] { +// return Grpc::RawAsyncClientPtr{async_client_}; +// })); +// return Grpc::AsyncClientFactoryPtr{factory_}; +// })); +// } + +// Grpc::MockAsyncClient* async_client_; +// Grpc::MockAsyncClientFactory* factory_; +// Grpc::MockAsyncClientManager async_client_manager_; +// NiceMock scope_; +// NiceMock tls_; +// GrpcOpenTelemetryAccessLoggerCacheImpl logger_cache_; +// GrpcOpenTelemetryAccessLoggerImplTestHelper grpc_access_logger_impl_test_helper_; +// }; + +// // Test that the logger is created according to the config (by inspecting the generated log). +// TEST_F(GrpcOpenTelemetryAccessLoggerCacheImplTest, LoggerCreation) { +// envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig config; +// config.set_log_name("test-log"); +// config.set_transport_api_version(envoy::config::core::v3::ApiVersion::V3); +// // Force a flush for every log entry. +// config.mutable_buffer_size_bytes()->set_value(BUFFER_SIZE_BYTES); + +// GrpcAccessLoggerSharedPtr logger = +// logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP, scope_); +// (void)logger; +// // Note that the local info node() method is mocked, so the node is not really configurable. +// // grpc_access_logger_impl_test_helper_.expectStreamMessage(R"EOF( +// // identifier: +// // node: +// // id: node_name +// // cluster: cluster_name +// // locality: +// // zone: zone_name +// // log_name: test-log +// // http_logs: +// // log_entry: +// // request: +// // path: /test/path1 +// // )EOF"); +// // envoy::data::accesslog::v3::HTTPAccessLogEntry entry; +// // entry.mutable_request()->set_path("/test/path1"); +// // logger->log(envoy::data::accesslog::v3::HTTPAccessLogEntry(entry)); +// } + +} // namespace +} // namespace GrpcCommon +} // namespace AccessLoggers +} // namespace Extensions +} // namespace Envoy From 2562b5fb74674db58ea3cd60df26393bea126cea Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Mon, 21 Dec 2020 22:38:59 +0200 Subject: [PATCH 10/19] OTLP logger (no resource attributes) Signed-off-by: Itamar Kaminski --- .../common/grpc_access_logger.h | 3 +- source/extensions/access_loggers/grpc/BUILD | 3 +- .../grpc/grpc_ot_access_log_impl.cc | 43 +++-- .../grpc/grpc_ot_access_log_impl.h | 25 ++- .../common/grpc_access_logger_test.cc | 25 +++ test/extensions/access_loggers/grpc/BUILD | 1 + .../grpc/grpc_ot_access_log_impl_test.cc | 157 +++++++++--------- 7 files changed, 146 insertions(+), 111 deletions(-) diff --git a/source/extensions/access_loggers/common/grpc_access_logger.h b/source/extensions/access_loggers/common/grpc_access_logger.h index 23e7d09796fb..0f6f794dcdbd 100644 --- a/source/extensions/access_loggers/common/grpc_access_logger.h +++ b/source/extensions/access_loggers/common/grpc_access_logger.h @@ -220,6 +220,7 @@ class GrpcAccessLogger : public Detail::GrpcAccessLoggerset_name("envoy"); - root.mutable_instrumentation_library()->set_version("v3"); // TODO: Get real API version. - return root; +// See comment about the structure of repeated fields in the header file. +void GrpcOpenTelemetryAccessLoggerImpl::initRoot() { + root_ = message_.add_resource_logs()->add_instrumentation_library_logs(); + // No reason to clear and refill these every time. + root_->mutable_instrumentation_library()->set_name("envoy"); + root_->mutable_instrumentation_library()->set_version("v1"); } void GrpcOpenTelemetryAccessLoggerImpl::addEntry( @@ -51,22 +56,26 @@ void GrpcOpenTelemetryAccessLoggerImpl::addEntry( bool GrpcOpenTelemetryAccessLoggerImpl::isEmpty() { return root_->logs().empty(); } void GrpcOpenTelemetryAccessLoggerImpl::initMessage() { - fmt::print("iitamark init before\n"); - // See comment about structure of repeated fields. - message_.add_resource_logs()->mutable_instrumentation_library_logs()->Add( - opentelemetry::proto::logs::v1::InstrumentationLibraryLogs(root_)); - // root_->mutable_instrumentation_library()->set_name("envoy"); - // root_->mutable_instrumentation_library()->set_version("v3"); // GetAPI. - fmt::print("iitamark init after\n"); + // auto* resource = message_.mutable_resource_logs(0)->mutable_resource(); + // resource->add_attributes() .. + (void)log_name_; + (void)local_info_; } +void GrpcOpenTelemetryAccessLoggerImpl::clearMessage() { root_->clear_logs(); } + +GrpcOpenTelemetryAccessLoggerCacheImpl::GrpcOpenTelemetryAccessLoggerCacheImpl( + Grpc::AsyncClientManager& async_client_manager, Stats::Scope& scope, + ThreadLocal::SlotAllocator& tls, const LocalInfo::LocalInfo& local_info) + : GrpcAccessLoggerCache(async_client_manager, scope, tls), local_info_(local_info) {} + GrpcOpenTelemetryAccessLoggerImpl::SharedPtr GrpcOpenTelemetryAccessLoggerCacheImpl::createLogger( const envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig& config, Grpc::RawAsyncClientPtr&& client, std::chrono::milliseconds buffer_flush_interval_msec, uint64_t max_buffer_size_bytes, Event::Dispatcher& dispatcher, Stats::Scope& scope) { return std::make_shared( - std::move(client), buffer_flush_interval_msec, max_buffer_size_bytes, dispatcher, scope, - Config::Utility::getAndCheckTransportVersion(config)); + std::move(client), config.log_name(), buffer_flush_interval_msec, max_buffer_size_bytes, + dispatcher, local_info_, scope, Config::Utility::getAndCheckTransportVersion(config)); } } // namespace GrpcCommon diff --git a/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h b/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h index f019731309fd..5cb1b3563d18 100644 --- a/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h +++ b/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h @@ -10,6 +10,7 @@ #include "envoy/event/dispatcher.h" #include "envoy/extensions/access_loggers/grpc/v3/als.pb.h" #include "envoy/grpc/async_client_manager.h" +#include "envoy/local_info/local_info.h" #include "envoy/thread_local/thread_local.h" #include "extensions/access_loggers/common/grpc_access_logger.h" @@ -31,23 +32,24 @@ class GrpcOpenTelemetryAccessLoggerImpl opentelemetry::proto::collector::logs::v1::ExportLogsServiceRequest, opentelemetry::proto::collector::logs::v1::ExportLogsServiceResponse> { public: - GrpcOpenTelemetryAccessLoggerImpl(Grpc::RawAsyncClientPtr&& client, + GrpcOpenTelemetryAccessLoggerImpl(Grpc::RawAsyncClientPtr&& client, std::string log_name, std::chrono::milliseconds buffer_flush_interval_msec, uint64_t max_buffer_size_bytes, Event::Dispatcher& dispatcher, - Stats::Scope& scope, + const LocalInfo::LocalInfo& local_info, Stats::Scope& scope, envoy::config::core::v3::ApiVersion transport_api_version); private: - // Explanation. - static InstrumentationLibraryLogs initRoot() const; + void initRoot(); // Extensions::AccessLoggers::GrpcCommon::GrpcAccessLogger void addEntry(opentelemetry::proto::logs::v1::LogRecord&& entry) override; void addEntry(opentelemetry::proto::logs::v1::ResourceLogs&& entry) override; bool isEmpty() override; void initMessage() override; + void clearMessage() override; - // Root for adding new log entries (see comment above about structure). - const opentelemetry::proto::logs::v1::InstrumentationLibraryLogs root_; + const std::string log_name_; + const LocalInfo::LocalInfo& local_info_; + opentelemetry::proto::logs::v1::InstrumentationLibraryLogs* root_; }; class GrpcOpenTelemetryAccessLoggerCacheImpl @@ -56,9 +58,12 @@ class GrpcOpenTelemetryAccessLoggerCacheImpl envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig> { public: // Inherited constructor. - using Common::GrpcAccessLoggerCache::GrpcAccessLoggerCache; + // using Common::GrpcAccessLoggerCache::GrpcAccessLoggerCache; + GrpcOpenTelemetryAccessLoggerCacheImpl(Grpc::AsyncClientManager& async_client_manager, + Stats::Scope& scope, ThreadLocal::SlotAllocator& tls, + const LocalInfo::LocalInfo& local_info); private: // Common::GrpcAccessLoggerCache @@ -67,6 +72,8 @@ class GrpcOpenTelemetryAccessLoggerCacheImpl Grpc::RawAsyncClientPtr&& client, std::chrono::milliseconds buffer_flush_interval_msec, uint64_t max_buffer_size_bytes, Event::Dispatcher& dispatcher, Stats::Scope& scope) override; + + const LocalInfo::LocalInfo& local_info_; }; /** diff --git a/test/extensions/access_loggers/common/grpc_access_logger_test.cc b/test/extensions/access_loggers/common/grpc_access_logger_test.cc index 1c2ce6b36249..d93be366601e 100644 --- a/test/extensions/access_loggers/common/grpc_access_logger_test.cc +++ b/test/extensions/access_loggers/common/grpc_access_logger_test.cc @@ -64,6 +64,8 @@ class MockGrpcAccessLoggerImpl int numInits() const { return num_inits_; } + int numClears() const { return num_clears_; } + private: void mockAddEntry(const std::string& key) { if (!message_.fields().contains(key)) { @@ -94,7 +96,13 @@ class MockGrpcAccessLoggerImpl void initMessage() override { ++num_inits_; } + void clearMessage() override { + message_.Clear(); + num_clears_++; + } + int num_inits_ = 0; + int num_clears_ = 0; }; class GrpcAccessLogTest : public testing::Test { @@ -161,12 +169,15 @@ TEST_F(GrpcAccessLogTest, BasicFlow) { expectFlushedLogEntriesCount(stream, MOCK_HTTP_LOG_FIELD_NAME, 1); logger_->log(mockHttpEntry()); EXPECT_EQ(1, logger_->numInits()); + // Messages should be cleared after each flush. + EXPECT_EQ(1, logger_->numClears()); EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "mock_access_log_prefix.logs_written")->value()); // Log a TCP entry. expectFlushedLogEntriesCount(stream, MOCK_TCP_LOG_FIELD_NAME, 1); logger_->log(ProtobufWkt::Empty()); + EXPECT_EQ(2, logger_->numClears()); // TCP logging doesn't change the logs_written counter. EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "mock_access_log_prefix.logs_written")->value()); @@ -183,6 +194,7 @@ TEST_F(GrpcAccessLogTest, BasicFlow) { logger_->log(mockHttpEntry()); // Message should be initialized again. EXPECT_EQ(2, logger_->numInits()); + EXPECT_EQ(3, logger_->numClears()); EXPECT_EQ(0, TestUtility::findCounter(stats_store_, "mock_access_log_prefix.logs_dropped")->value()); EXPECT_EQ(2, @@ -202,6 +214,8 @@ TEST_F(GrpcAccessLogTest, WatermarksOverrun) { EXPECT_CALL(stream, isAboveWriteBufferHighWatermark()).WillOnce(Return(true)); EXPECT_CALL(stream, sendMessageRaw_(_, false)).Times(0); logger_->log(mockHttpEntry()); + // No entry was logged so no clear expected. + EXPECT_EQ(0, logger_->numClears()); EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "mock_access_log_prefix.logs_written")->value()); EXPECT_EQ(0, @@ -212,6 +226,8 @@ TEST_F(GrpcAccessLogTest, WatermarksOverrun) { EXPECT_CALL(stream, sendMessageRaw_(_, _)).Times(0); logger_->log(mockHttpEntry()); EXPECT_EQ(1, logger_->numInits()); + // Still no entry was logged so no clear expected. + EXPECT_EQ(0, logger_->numClears()); EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "mock_access_log_prefix.logs_written")->value()); EXPECT_EQ(1, @@ -224,6 +240,8 @@ TEST_F(GrpcAccessLogTest, WatermarksOverrun) { EXPECT_CALL(stream, isAboveWriteBufferHighWatermark()).WillOnce(Return(false)); EXPECT_CALL(stream, sendMessageRaw_(_, _)); logger_->log(mockHttpEntry()); + // Now both entries were logged separately so we expect 2 clears. + EXPECT_EQ(2, logger_->numClears()); EXPECT_EQ(2, TestUtility::findCounter(stats_store_, "mock_access_log_prefix.logs_written")->value()); EXPECT_EQ(1, @@ -252,6 +270,8 @@ TEST_F(GrpcAccessLogTest, WatermarksLegacy) { logger_->log(mockHttpEntry()); // Message should still be initialized. EXPECT_EQ(1, logger_->numInits()); + // No entry was logged so no clear expected. + EXPECT_EQ(0, logger_->numClears()); EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "mock_access_log_prefix.logs_written")->value()); EXPECT_EQ(0, @@ -260,6 +280,8 @@ TEST_F(GrpcAccessLogTest, WatermarksLegacy) { // As with the above test, try to log more. The log will not be dropped. EXPECT_CALL(stream, sendMessageRaw_(_, _)).Times(0); logger_->log(mockHttpEntry()); + // Still no entries were logged so no clears expected. + EXPECT_EQ(0, logger_->numClears()); EXPECT_EQ(2, TestUtility::findCounter(stats_store_, "mock_access_log_prefix.logs_written")->value()); EXPECT_EQ(0, @@ -296,6 +318,8 @@ TEST_F(GrpcAccessLogTest, Batching) { logger_->log(mockHttpEntry()); logger_->log(mockHttpEntry()); EXPECT_EQ(1, logger_->numInits()); + // The entries were batched and logged together so we expect a single clear. + EXPECT_EQ(1, logger_->numClears()); // Logging an entry that's bigger than the buffer size should trigger another flush. expectFlushedLogEntriesCount(stream, MOCK_HTTP_LOG_FIELD_NAME, 1); @@ -303,6 +327,7 @@ TEST_F(GrpcAccessLogTest, Batching) { const std::string big_key(max_buffer_size, 'a'); big_entry.mutable_fields()->insert({big_key, ProtobufWkt::Value()}); logger_->log(std::move(big_entry)); + EXPECT_EQ(2, logger_->numClears()); } // Test that log entries are flushed periodically. diff --git a/test/extensions/access_loggers/grpc/BUILD b/test/extensions/access_loggers/grpc/BUILD index 7f190a356368..903a161afe81 100644 --- a/test/extensions/access_loggers/grpc/BUILD +++ b/test/extensions/access_loggers/grpc/BUILD @@ -122,6 +122,7 @@ envoy_extension_cc_test( "//source/common/buffer:zero_copy_input_stream_lib", "//source/extensions/access_loggers/grpc:grpc_ot_access_log_lib", "//test/mocks/grpc:grpc_mocks", + "//test/mocks/local_info:local_info_mocks", "//test/mocks/stats:stats_mocks", "//test/mocks/thread_local:thread_local_mocks", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", diff --git a/test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc b/test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc index 016b4a90dc0b..2c29f0556669 100644 --- a/test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc +++ b/test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc @@ -13,6 +13,7 @@ #include "extensions/access_loggers/grpc/grpc_ot_access_log_impl.h" #include "test/mocks/grpc/mocks.h" +#include "test/mocks/local_info/mocks.h" #include "test/mocks/stats/mocks.h" #include "test/mocks/thread_local/mocks.h" @@ -42,7 +43,10 @@ class GrpcOpenTelemetryAccessLoggerImplTestHelper { using AccessLogCallbacks = Grpc::AsyncStreamCallbacks< opentelemetry::proto::collector::logs::v1::ExportLogsServiceResponse>; - GrpcOpenTelemetryAccessLoggerImplTestHelper(Grpc::MockAsyncClient* async_client) { + GrpcOpenTelemetryAccessLoggerImplTestHelper(LocalInfo::MockLocalInfo& local_info, + Grpc::MockAsyncClient* async_client) { + // EXPECT_CALL(local_info, node()); + (void)local_info; EXPECT_CALL(*async_client, startRaw(_, _, _, _)) .WillOnce( Invoke([this](absl::string_view, absl::string_view, Grpc::RawAsyncStreamCallbacks& cbs, @@ -74,17 +78,16 @@ class GrpcOpenTelemetryAccessLoggerImplTest : public testing::Test { public: GrpcOpenTelemetryAccessLoggerImplTest() : async_client_(new Grpc::MockAsyncClient), timer_(new Event::MockTimer(&dispatcher_)), - grpc_access_logger_impl_test_helper_(async_client_) { + grpc_access_logger_impl_test_helper_(local_info_, async_client_) { EXPECT_CALL(*timer_, enableTimer(_, _)); - fmt::print("before"); logger_ = std::make_unique( - Grpc::RawAsyncClientPtr{async_client_}, FlushInterval, BUFFER_SIZE_BYTES, dispatcher_, - stats_store_, envoy::config::core::v3::ApiVersion::V3); - fmt::print("after"); + Grpc::RawAsyncClientPtr{async_client_}, "test_log_name", FlushInterval, BUFFER_SIZE_BYTES, + dispatcher_, local_info_, stats_store_, envoy::config::core::v3::ApiVersion::V3); } Grpc::MockAsyncClient* async_client_; Stats::IsolatedStoreImpl stats_store_; + LocalInfo::MockLocalInfo local_info_; Event::MockDispatcher dispatcher_; Event::MockTimer* timer_; std::unique_ptr logger_; @@ -97,88 +100,78 @@ TEST_F(GrpcOpenTelemetryAccessLoggerImplTest, LogHttp) { - instrumentation_library_logs: - instrumentation_library: name: "envoy" - version: "v3" + version: "v1" logs: - - severity_text: "iitamark" + - severity_text: "test-severity-text" )EOF"); opentelemetry::proto::logs::v1::LogRecord entry; - entry.set_severity_text("iitamark"); + entry.set_severity_text("test-severity-text"); logger_->log(opentelemetry::proto::logs::v1::LogRecord(entry)); } -// TEST_F(GrpcOpenTelemetryAccessLoggerImplTest, LogTcp) { -// grpc_access_logger_impl_test_helper_.expectStreamMessage(R"EOF( -// identifier: -// node: -// id: node_name -// cluster: cluster_name -// locality: -// zone: zone_name -// log_name: test_log_name -// tcp_logs: -// log_entry: -// common_properties: -// sample_rate: 1.0 -// )EOF"); -// envoy::data::accesslog::v3::TCPAccessLogEntry tcp_entry; -// tcp_entry.mutable_common_properties()->set_sample_rate(1); -// logger_->log(envoy::data::accesslog::v3::TCPAccessLogEntry(tcp_entry)); -// } - -// class GrpcOpenTelemetryAccessLoggerCacheImplTest : public testing::Test { -// public: -// GrpcOpenTelemetryAccessLoggerCacheImplTest() -// : async_client_(new Grpc::MockAsyncClient), factory_(new Grpc::MockAsyncClientFactory), -// logger_cache_(async_client_manager_, scope_, tls_), -// grpc_access_logger_impl_test_helper_(async_client_) { -// EXPECT_CALL(async_client_manager_, factoryForGrpcService(_, _, false)) -// .WillOnce(Invoke([this](const envoy::config::core::v3::GrpcService&, Stats::Scope&, bool) -// { -// EXPECT_CALL(*factory_, create()).WillOnce(Invoke([this] { -// return Grpc::RawAsyncClientPtr{async_client_}; -// })); -// return Grpc::AsyncClientFactoryPtr{factory_}; -// })); -// } - -// Grpc::MockAsyncClient* async_client_; -// Grpc::MockAsyncClientFactory* factory_; -// Grpc::MockAsyncClientManager async_client_manager_; -// NiceMock scope_; -// NiceMock tls_; -// GrpcOpenTelemetryAccessLoggerCacheImpl logger_cache_; -// GrpcOpenTelemetryAccessLoggerImplTestHelper grpc_access_logger_impl_test_helper_; -// }; - -// // Test that the logger is created according to the config (by inspecting the generated log). -// TEST_F(GrpcOpenTelemetryAccessLoggerCacheImplTest, LoggerCreation) { -// envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig config; -// config.set_log_name("test-log"); -// config.set_transport_api_version(envoy::config::core::v3::ApiVersion::V3); -// // Force a flush for every log entry. -// config.mutable_buffer_size_bytes()->set_value(BUFFER_SIZE_BYTES); - -// GrpcAccessLoggerSharedPtr logger = -// logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP, scope_); -// (void)logger; -// // Note that the local info node() method is mocked, so the node is not really configurable. -// // grpc_access_logger_impl_test_helper_.expectStreamMessage(R"EOF( -// // identifier: -// // node: -// // id: node_name -// // cluster: cluster_name -// // locality: -// // zone: zone_name -// // log_name: test-log -// // http_logs: -// // log_entry: -// // request: -// // path: /test/path1 -// // )EOF"); -// // envoy::data::accesslog::v3::HTTPAccessLogEntry entry; -// // entry.mutable_request()->set_path("/test/path1"); -// // logger->log(envoy::data::accesslog::v3::HTTPAccessLogEntry(entry)); -// } +TEST_F(GrpcOpenTelemetryAccessLoggerImplTest, LogTcp) { + grpc_access_logger_impl_test_helper_.expectStreamMessage(R"EOF( + resource_logs: + - instrumentation_library_logs: + - instrumentation_library: + name: "envoy" + version: "v1" + logs: + - severity_text: "test-severity-text" + )EOF"); + opentelemetry::proto::logs::v1::LogRecord entry; + entry.set_severity_text("test-severity-text"); + logger_->log(opentelemetry::proto::logs::v1::LogRecord(entry)); +} + +class GrpcOpenTelemetryAccessLoggerCacheImplTest : public testing::Test { +public: + GrpcOpenTelemetryAccessLoggerCacheImplTest() + : async_client_(new Grpc::MockAsyncClient), factory_(new Grpc::MockAsyncClientFactory), + logger_cache_(async_client_manager_, scope_, tls_, local_info_), + grpc_access_logger_impl_test_helper_(local_info_, async_client_) { + EXPECT_CALL(async_client_manager_, factoryForGrpcService(_, _, false)) + .WillOnce(Invoke([this](const envoy::config::core::v3::GrpcService&, Stats::Scope&, bool) { + EXPECT_CALL(*factory_, create()).WillOnce(Invoke([this] { + return Grpc::RawAsyncClientPtr{async_client_}; + })); + return Grpc::AsyncClientFactoryPtr{factory_}; + })); + } + + Grpc::MockAsyncClient* async_client_; + Grpc::MockAsyncClientFactory* factory_; + Grpc::MockAsyncClientManager async_client_manager_; + LocalInfo::MockLocalInfo local_info_; + NiceMock scope_; + NiceMock tls_; + GrpcOpenTelemetryAccessLoggerCacheImpl logger_cache_; + GrpcOpenTelemetryAccessLoggerImplTestHelper grpc_access_logger_impl_test_helper_; +}; + +// Test that the logger is created according to the config (by inspecting the generated log). +TEST_F(GrpcOpenTelemetryAccessLoggerCacheImplTest, LoggerCreation) { + envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig config; + config.set_log_name("test-log"); + config.set_transport_api_version(envoy::config::core::v3::ApiVersion::V3); + // Force a flush for every log entry. + config.mutable_buffer_size_bytes()->set_value(BUFFER_SIZE_BYTES); + + GrpcAccessLoggerSharedPtr logger = + logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP, scope_); + grpc_access_logger_impl_test_helper_.expectStreamMessage(R"EOF( + resource_logs: + - instrumentation_library_logs: + - instrumentation_library: + name: "envoy" + version: "v1" + logs: + - severity_text: "test-severity-text" + )EOF"); + opentelemetry::proto::logs::v1::LogRecord entry; + entry.set_severity_text("test-severity-text"); + logger->log(opentelemetry::proto::logs::v1::LogRecord(entry)); +} } // namespace } // namespace GrpcCommon From 04a3628c771768eaef0967d7b15034c50da30691 Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Fri, 15 Jan 2021 18:49:13 +0200 Subject: [PATCH 11/19] Move resource data into OTLP 'Resource' Signed-off-by: Itamar Kaminski --- source/extensions/access_loggers/common/BUILD | 1 - .../common/grpc_access_logger.h | 1 - .../grpc/grpc_ot_access_log_impl.cc | 40 ++++++---- .../grpc/grpc_ot_access_log_impl.h | 8 +- .../grpc/grpc_ot_access_log_impl_test.cc | 77 +++++++++++++------ 5 files changed, 82 insertions(+), 45 deletions(-) diff --git a/source/extensions/access_loggers/common/BUILD b/source/extensions/access_loggers/common/BUILD index d7e449111640..e7851e9da725 100644 --- a/source/extensions/access_loggers/common/BUILD +++ b/source/extensions/access_loggers/common/BUILD @@ -34,7 +34,6 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/grpc:typed_async_client_lib", "//source/common/protobuf:utility_lib", - "//source/common/runtime:runtime_features_lib", "@com_google_absl//absl/types:optional", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], diff --git a/source/extensions/access_loggers/common/grpc_access_logger.h b/source/extensions/access_loggers/common/grpc_access_logger.h index 0f6f794dcdbd..f28b50f786be 100644 --- a/source/extensions/access_loggers/common/grpc_access_logger.h +++ b/source/extensions/access_loggers/common/grpc_access_logger.h @@ -12,7 +12,6 @@ #include "common/common/assert.h" #include "common/grpc/typed_async_client.h" #include "common/protobuf/utility.h" -#include "common/runtime/runtime_features.h" #include "absl/container/flat_hash_map.h" #include "absl/types/optional.h" diff --git a/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.cc b/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.cc index 0936f5896099..8132733393d1 100644 --- a/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.cc +++ b/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.cc @@ -30,17 +30,33 @@ GrpcOpenTelemetryAccessLoggerImpl::GrpcOpenTelemetryAccessLoggerImpl( Grpc::VersionedMethods("opentelemetry.proto.collector.logs.v1.LogsService.Export", "opentelemetry.proto.collector.logs.v1.LogsService.Export") .getMethodDescriptorForVersion(transport_api_version), - transport_api_version), - log_name_(log_name), local_info_(local_info) { - initRoot(); + transport_api_version) { + initMessageRoot(log_name, local_info); } +namespace { + +opentelemetry::proto::common::v1::KeyValue getStringKeyValue(const std::string& key, + const std::string& value) { + opentelemetry::proto::common::v1::KeyValue keyValue; + keyValue.set_key(key); + keyValue.mutable_value()->set_string_value(value); + return keyValue; +} + +} // namespace + // See comment about the structure of repeated fields in the header file. -void GrpcOpenTelemetryAccessLoggerImpl::initRoot() { - root_ = message_.add_resource_logs()->add_instrumentation_library_logs(); - // No reason to clear and refill these every time. - root_->mutable_instrumentation_library()->set_name("envoy"); - root_->mutable_instrumentation_library()->set_version("v1"); +// TODO(itamarkam): allow user configurable attributes. +void GrpcOpenTelemetryAccessLoggerImpl::initMessageRoot(const std::string& log_name, + const LocalInfo::LocalInfo& local_info) { + auto* resource_logs = message_.add_resource_logs(); + root_ = resource_logs->add_instrumentation_library_logs(); + auto* resource = resource_logs->mutable_resource(); + *resource->add_attributes() = getStringKeyValue("log_name", log_name); + *resource->add_attributes() = getStringKeyValue("zone_name", local_info.zoneName()); + *resource->add_attributes() = getStringKeyValue("cluster_name", local_info.clusterName()); + *resource->add_attributes() = getStringKeyValue("node_name", local_info.nodeName()); } void GrpcOpenTelemetryAccessLoggerImpl::addEntry( @@ -55,12 +71,8 @@ void GrpcOpenTelemetryAccessLoggerImpl::addEntry( bool GrpcOpenTelemetryAccessLoggerImpl::isEmpty() { return root_->logs().empty(); } -void GrpcOpenTelemetryAccessLoggerImpl::initMessage() { - // auto* resource = message_.mutable_resource_logs(0)->mutable_resource(); - // resource->add_attributes() .. - (void)log_name_; - (void)local_info_; -} +// The message is already initialized in the c'tor, and only the logs are cleared. +void GrpcOpenTelemetryAccessLoggerImpl::initMessage() {} void GrpcOpenTelemetryAccessLoggerImpl::clearMessage() { root_->clear_logs(); } diff --git a/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h b/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h index 5cb1b3563d18..4255b94fc963 100644 --- a/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h +++ b/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h @@ -39,7 +39,7 @@ class GrpcOpenTelemetryAccessLoggerImpl envoy::config::core::v3::ApiVersion transport_api_version); private: - void initRoot(); + void initMessageRoot(const std::string& log_name, const LocalInfo::LocalInfo& local_info); // Extensions::AccessLoggers::GrpcCommon::GrpcAccessLogger void addEntry(opentelemetry::proto::logs::v1::LogRecord&& entry) override; void addEntry(opentelemetry::proto::logs::v1::ResourceLogs&& entry) override; @@ -47,8 +47,6 @@ class GrpcOpenTelemetryAccessLoggerImpl void initMessage() override; void clearMessage() override; - const std::string log_name_; - const LocalInfo::LocalInfo& local_info_; opentelemetry::proto::logs::v1::InstrumentationLibraryLogs* root_; }; @@ -57,10 +55,6 @@ class GrpcOpenTelemetryAccessLoggerCacheImpl GrpcOpenTelemetryAccessLoggerImpl, envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig> { public: - // Inherited constructor. - // using Common::GrpcAccessLoggerCache::GrpcAccessLoggerCache; GrpcOpenTelemetryAccessLoggerCacheImpl(Grpc::AsyncClientManager& async_client_manager, Stats::Scope& scope, ThreadLocal::SlotAllocator& tls, const LocalInfo::LocalInfo& local_info); diff --git a/test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc b/test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc index 2c29f0556669..be8fb5f12b56 100644 --- a/test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc +++ b/test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc @@ -21,11 +21,7 @@ using testing::_; using testing::Invoke; using testing::NiceMock; using testing::Return; - -// opentelemetry::proto::logs::v1::LogRecord, -// opentelemetry::proto::logs::v1::ResourceLogs /*TCP*/, -// opentelemetry::proto::collector::logs::v1::ExportLogsServiceRequest, -// opentelemetry::proto::collector::logs::v1::ExportLogsServiceResponse> +using testing::ReturnRef; namespace Envoy { namespace Extensions { @@ -35,6 +31,9 @@ namespace { constexpr std::chrono::milliseconds FlushInterval(10); constexpr int BUFFER_SIZE_BYTES = 0; +const std::string ZONE_NAME = "zone_name"; +const std::string CLUSTER_NAME = "cluster_name"; +const std::string NODE_NAME = "node_name"; // A helper test class to mock and intercept GrpcOpenTelemetryAccessLoggerImpl streams. class GrpcOpenTelemetryAccessLoggerImplTestHelper { @@ -45,8 +44,9 @@ class GrpcOpenTelemetryAccessLoggerImplTestHelper { GrpcOpenTelemetryAccessLoggerImplTestHelper(LocalInfo::MockLocalInfo& local_info, Grpc::MockAsyncClient* async_client) { - // EXPECT_CALL(local_info, node()); - (void)local_info; + EXPECT_CALL(local_info, zoneName()).WillOnce(ReturnRef(ZONE_NAME)); + EXPECT_CALL(local_info, clusterName()).WillOnce(ReturnRef(CLUSTER_NAME)); + EXPECT_CALL(local_info, nodeName()).WillOnce(ReturnRef(NODE_NAME)); EXPECT_CALL(*async_client, startRaw(_, _, _, _)) .WillOnce( Invoke([this](absl::string_view, absl::string_view, Grpc::RawAsyncStreamCallbacks& cbs, @@ -97,11 +97,22 @@ class GrpcOpenTelemetryAccessLoggerImplTest : public testing::Test { TEST_F(GrpcOpenTelemetryAccessLoggerImplTest, LogHttp) { grpc_access_logger_impl_test_helper_.expectStreamMessage(R"EOF( resource_logs: - - instrumentation_library_logs: - - instrumentation_library: - name: "envoy" - version: "v1" - logs: + resource: + attributes: + - key: "log_name" + value: + string_value: "test_log_name" + - key: "zone_name" + value: + string_value: "zone_name" + - key: "cluster_name" + value: + string_value: "cluster_name" + - key: "node_name" + value: + string_value: "node_name" + instrumentation_library_logs: + - logs: - severity_text: "test-severity-text" )EOF"); opentelemetry::proto::logs::v1::LogRecord entry; @@ -112,11 +123,22 @@ TEST_F(GrpcOpenTelemetryAccessLoggerImplTest, LogHttp) { TEST_F(GrpcOpenTelemetryAccessLoggerImplTest, LogTcp) { grpc_access_logger_impl_test_helper_.expectStreamMessage(R"EOF( resource_logs: - - instrumentation_library_logs: - - instrumentation_library: - name: "envoy" - version: "v1" - logs: + resource: + attributes: + - key: "log_name" + value: + string_value: "test_log_name" + - key: "zone_name" + value: + string_value: "zone_name" + - key: "cluster_name" + value: + string_value: "cluster_name" + - key: "node_name" + value: + string_value: "node_name" + instrumentation_library_logs: + - logs: - severity_text: "test-severity-text" )EOF"); opentelemetry::proto::logs::v1::LogRecord entry; @@ -161,11 +183,22 @@ TEST_F(GrpcOpenTelemetryAccessLoggerCacheImplTest, LoggerCreation) { logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP, scope_); grpc_access_logger_impl_test_helper_.expectStreamMessage(R"EOF( resource_logs: - - instrumentation_library_logs: - - instrumentation_library: - name: "envoy" - version: "v1" - logs: + resource: + attributes: + - key: "log_name" + value: + string_value: "test-log" + - key: "zone_name" + value: + string_value: "zone_name" + - key: "cluster_name" + value: + string_value: "cluster_name" + - key: "node_name" + value: + string_value: "node_name" + instrumentation_library_logs: + - logs: - severity_text: "test-severity-text" )EOF"); opentelemetry::proto::logs::v1::LogRecord entry; From 9a5c5455d3b94bd3c6ab9b91e4419267c24ee902 Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Mon, 25 Jan 2021 18:46:11 +0200 Subject: [PATCH 12/19] Fix history of bad merge Signed-off-by: Itamar Kaminski --- include/envoy/event/dispatcher.h | 412 ++++++++++++++++--------------- 1 file changed, 212 insertions(+), 200 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index b9dd1ade0e5d..36e34f78bd1a 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -73,211 +73,223 @@ class DispatcherBase { uint32_t events) PURE; /** - * Abstract event dispatching loop. + * Allocates a timer. @see Timer for docs on how to use the timer. + * @param cb supplies the callback to invoke when the timer fires. */ - class Dispatcher : public DispatcherBase { - public: - /** - * Allocates a timer. @see Timer for docs on how to use the timer. - * @param cb supplies the callback to invoke when the timer fires. - */ - virtual Event::TimerPtr createTimer(TimerCb cb) PURE; - - /** - * Allocates a schedulable callback. @see SchedulableCallback for docs on how to use the wrapped - * callback. - * @param cb supplies the callback to invoke when the SchedulableCallback is triggered on the - * event loop. - */ - virtual Event::SchedulableCallbackPtr createSchedulableCallback(std::function cb) PURE; - - /** - * Sets a tracked object, which is currently operating in this Dispatcher. - * This should be cleared with another call to setTrackedObject() when the object is done doing - * work. Calling setTrackedObject(nullptr) results in no object being tracked. - * - * This is optimized for performance, to avoid allocation where we do scoped object tracking. - * - * @return The previously tracked object or nullptr if there was none. - */ - virtual const ScopeTrackedObject* setTrackedObject(const ScopeTrackedObject* object) PURE; - - /** - * Validates that an operation is thread-safe with respect to this dispatcher; i.e. that the - * current thread of execution is on the same thread upon which the dispatcher loop is running. - */ - virtual bool isThreadSafe() const PURE; - - /** - * Returns a recently cached MonotonicTime value. - */ - virtual MonotonicTime approximateMonotonicTime() const PURE; - }; + virtual Event::TimerPtr createTimer(TimerCb cb) PURE; + + /** + * Allocates a scaled timer. @see Timer for docs on how to use the timer. + * @param timer_type the type of timer to create. + * @param cb supplies the callback to invoke when the timer fires. + */ + virtual Event::TimerPtr createScaledTimer(Event::ScaledTimerType timer_type, TimerCb cb) PURE; + + /** + * Allocates a scaled timer. @see Timer for docs on how to use the timer. + * @param minimum the rule for computing the minimum value of the timer. + * @param cb supplies the callback to invoke when the timer fires. + */ + virtual Event::TimerPtr createScaledTimer(Event::ScaledTimerMinimum minimum, TimerCb cb) PURE; + + /** + * Allocates a schedulable callback. @see SchedulableCallback for docs on how to use the wrapped + * callback. + * @param cb supplies the callback to invoke when the SchedulableCallback is triggered on the + * event loop. + */ + virtual Event::SchedulableCallbackPtr createSchedulableCallback(std::function cb) PURE; + + /** + * Appends a tracked object to the current stack of tracked objects operating + * in the dispatcher. + * + * It's recommended to use ScopeTrackerScopeState to manage the object's tracking. If directly + * invoking, there needs to be a subsequent call to popTrackedObject(). + */ + virtual void pushTrackedObject(const ScopeTrackedObject* object) PURE; + + /** + * Removes the top of the stack of tracked object and asserts that it was expected. + */ + virtual void popTrackedObject(const ScopeTrackedObject* expected_object) PURE; + + /** + * Validates that an operation is thread-safe with respect to this dispatcher; i.e. that the + * current thread of execution is on the same thread upon which the dispatcher loop is running. + */ + virtual bool isThreadSafe() const PURE; + + /** + * Returns a recently cached MonotonicTime value. + */ + virtual MonotonicTime approximateMonotonicTime() const PURE; +}; + +/** + * Abstract event dispatching loop. + */ +class Dispatcher : public DispatcherBase { +public: + /** + * Returns the name that identifies this dispatcher, such as "worker_2" or "main_thread". + * @return const std::string& the name that identifies this dispatcher. + */ + virtual const std::string& name() PURE; + + /** + * Register a watchdog for this dispatcher. The dispatcher is responsible for touching the + * watchdog at least once per touch interval. Dispatcher implementations may choose to touch more + * often to avoid spurious miss events when processing long callback queues. + * @param min_touch_interval Touch interval for the watchdog. + */ + virtual void registerWatchdog(const Server::WatchDogSharedPtr& watchdog, + std::chrono::milliseconds min_touch_interval) PURE; + + /** + * Returns a time-source to use with this dispatcher. + */ + virtual TimeSource& timeSource() PURE; + + /** + * Initializes stats for this dispatcher. Note that this can't generally be done at construction + * time, since the main and worker thread dispatchers are constructed before + * ThreadLocalStoreImpl::initializeThreading. + * @param scope the scope to contain the new per-dispatcher stats created here. + * @param prefix the stats prefix to identify this dispatcher. If empty, the dispatcher will be + * identified by its name. + */ + virtual void initializeStats(Stats::Scope& scope, + const absl::optional& prefix = absl::nullopt) PURE; + + /** + * Clears any items in the deferred deletion queue. + */ + virtual void clearDeferredDeleteList() PURE; + + /** + * Wraps an already-accepted socket in an instance of Envoy's server Network::Connection. + * @param socket supplies an open file descriptor and connection metadata to use for the + * connection. Takes ownership of the socket. + * @param transport_socket supplies a transport socket to be used by the connection. + * @param stream_info info object for the server connection + * @return Network::ConnectionPtr a server connection that is owned by the caller. + */ + virtual Network::ServerConnectionPtr + createServerConnection(Network::ConnectionSocketPtr&& socket, + Network::TransportSocketPtr&& transport_socket, + StreamInfo::StreamInfo& stream_info) PURE; + + /** + * Creates an instance of Envoy's Network::ClientConnection. Does NOT initiate the connection; + * the caller must then call connect() on the returned Network::ClientConnection. + * @param address supplies the address to connect to. + * @param source_address supplies an address to bind to or nullptr if no bind is necessary. + * @param transport_socket supplies a transport socket to be used by the connection. + * @param options the socket options to be set on the underlying socket before anything is sent + * on the socket. + * @return Network::ClientConnectionPtr a client connection that is owned by the caller. + */ + virtual Network::ClientConnectionPtr + createClientConnection(Network::Address::InstanceConstSharedPtr address, + Network::Address::InstanceConstSharedPtr source_address, + Network::TransportSocketPtr&& transport_socket, + const Network::ConnectionSocket::OptionsSharedPtr& options) PURE; + + /** + * Creates an async DNS resolver. The resolver should only be used on the thread that runs this + * dispatcher. + * @param resolvers supplies the addresses of DNS resolvers that this resolver should use. If left + * empty, it will not use any specific resolvers, but use defaults (/etc/resolv.conf) + * @param use_tcp_for_dns_lookups if set to true, tcp will be used to perform dns lookups. + * Otherwise, udp is used. + * @return Network::DnsResolverSharedPtr that is owned by the caller. + */ + virtual Network::DnsResolverSharedPtr + createDnsResolver(const std::vector& resolvers, + bool use_tcp_for_dns_lookups) PURE; + + /** + * @return Filesystem::WatcherPtr a filesystem watcher owned by the caller. + */ + virtual Filesystem::WatcherPtr createFilesystemWatcher() PURE; + + /** + * Creates a listener on a specific port. + * @param socket supplies the socket to listen on. + * @param cb supplies the callbacks to invoke for listener events. + * @param bind_to_port controls whether the listener binds to a transport port or not. + * @param backlog_size controls listener pending connections backlog + * @return Network::ListenerPtr a new listener that is owned by the caller. + */ + virtual Network::ListenerPtr createListener(Network::SocketSharedPtr&& socket, + Network::TcpListenerCallbacks& cb, bool bind_to_port, + uint32_t backlog_size) PURE; /** - * Abstract event dispatching loop. + * Creates a logical udp listener on a specific port. + * @param socket supplies the socket to listen on. + * @param cb supplies the udp listener callbacks to invoke for listener events. + * @return Network::ListenerPtr a new listener that is owned by the caller. */ - class Dispatcher : public DispatcherBase { - public: - /** - * Returns the name that identifies this dispatcher, such as "worker_2" or "main_thread". - * @return const std::string& the name that identifies this dispatcher. - */ - virtual const std::string& name() PURE; - - /** - * Register a watchdog for this dispatcher. The dispatcher is responsible for touching the - * watchdog at least once per touch interval. Dispatcher implementations may choose to touch - * more often to avoid spurious miss events when processing long callback queues. - * @param min_touch_interval Touch interval for the watchdog. - */ - virtual void registerWatchdog(const Server::WatchDogSharedPtr& watchdog, - std::chrono::milliseconds min_touch_interval) PURE; - - /** - * Returns a time-source to use with this dispatcher. - */ - virtual TimeSource& timeSource() PURE; - - /** - * Initializes stats for this dispatcher. Note that this can't generally be done at construction - * time, since the main and worker thread dispatchers are constructed before - * ThreadLocalStoreImpl::initializeThreading. - * @param scope the scope to contain the new per-dispatcher stats created here. - * @param prefix the stats prefix to identify this dispatcher. If empty, the dispatcher will be - * identified by its name. - */ - virtual void initializeStats(Stats::Scope& scope, - const absl::optional& prefix = absl::nullopt) PURE; - - /** - * Clears any items in the deferred deletion queue. - */ - virtual void clearDeferredDeleteList() PURE; - - /** - * Wraps an already-accepted socket in an instance of Envoy's server Network::Connection. - * @param socket supplies an open file descriptor and connection metadata to use for the - * connection. Takes ownership of the socket. - * @param transport_socket supplies a transport socket to be used by the connection. - * @param stream_info info object for the server connection - * @return Network::ConnectionPtr a server connection that is owned by the caller. - */ - virtual Network::ServerConnectionPtr - createServerConnection(Network::ConnectionSocketPtr&& socket, - Network::TransportSocketPtr&& transport_socket, - StreamInfo::StreamInfo& stream_info) PURE; - - /** - * Creates an instance of Envoy's Network::ClientConnection. Does NOT initiate the connection; - * the caller must then call connect() on the returned Network::ClientConnection. - * @param address supplies the address to connect to. - * @param source_address supplies an address to bind to or nullptr if no bind is necessary. - * @param transport_socket supplies a transport socket to be used by the connection. - * @param options the socket options to be set on the underlying socket before anything is sent - * on the socket. - * @return Network::ClientConnectionPtr a client connection that is owned by the caller. - */ - virtual Network::ClientConnectionPtr - createClientConnection(Network::Address::InstanceConstSharedPtr address, - Network::Address::InstanceConstSharedPtr source_address, - Network::TransportSocketPtr&& transport_socket, - const Network::ConnectionSocket::OptionsSharedPtr& options) PURE; - - /** - * Creates an async DNS resolver. The resolver should only be used on the thread that runs this - * dispatcher. - * @param resolvers supplies the addresses of DNS resolvers that this resolver should use. If - * left empty, it will not use any specific resolvers, but use defaults (/etc/resolv.conf) - * @param use_tcp_for_dns_lookups if set to true, tcp will be used to perform dns lookups. - * Otherwise, udp is used. - * @return Network::DnsResolverSharedPtr that is owned by the caller. - */ - virtual Network::DnsResolverSharedPtr - createDnsResolver(const std::vector& resolvers, - bool use_tcp_for_dns_lookups) PURE; - - /** - * @return Filesystem::WatcherPtr a filesystem watcher owned by the caller. - */ - virtual Filesystem::WatcherPtr createFilesystemWatcher() PURE; - - /** - * Creates a listener on a specific port. - * @param socket supplies the socket to listen on. - * @param cb supplies the callbacks to invoke for listener events. - * @param bind_to_port controls whether the listener binds to a transport port or not. - * @param backlog_size controls listener pending connections backlog - * @return Network::ListenerPtr a new listener that is owned by the caller. - */ - virtual Network::ListenerPtr createListener(Network::SocketSharedPtr&& socket, - Network::TcpListenerCallbacks& cb, - bool bind_to_port, uint32_t backlog_size) PURE; - - /** - * Creates a logical udp listener on a specific port. - * @param socket supplies the socket to listen on. - * @param cb supplies the udp listener callbacks to invoke for listener events. - * @return Network::ListenerPtr a new listener that is owned by the caller. - */ - virtual Network::UdpListenerPtr createUdpListener(Network::SocketSharedPtr socket, - Network::UdpListenerCallbacks& cb) PURE; - /** - * Submits an item for deferred delete. @see DeferredDeletable. - */ - virtual void deferredDelete(DeferredDeletablePtr&& to_delete) PURE; - - /** - * Exits the event loop. - */ - virtual void exit() PURE; - - /** - * Listens for a signal event. Only a single dispatcher in the process can listen for signals. - * If more than one dispatcher calls this routine in the process the behavior is undefined. - * - * @param signal_num supplies the signal to listen on. - * @param cb supplies the callback to invoke when the signal fires. - * @return SignalEventPtr a signal event that is owned by the caller. - */ - virtual SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) PURE; - - /** - * Posts a functor to the dispatcher. This is safe cross thread. The functor runs in the context - * of the dispatcher event loop which may be on a different thread than the caller. - */ - virtual void post(PostCb callback) PURE; - - /** - * Runs the event loop. This will not return until exit() is called either from within a - * callback or from a different thread. - * @param type specifies whether to run in blocking mode (run() will not return until exit() is - * called) or non-blocking mode where only active events will be executed and then - * run() will return. - */ - enum class RunType { - Block, // Runs the event-loop until there are no pending events. - NonBlock, // Checks for any pending events to activate, executes them, - // then exits. Exits immediately if there are no pending or - // active events. - RunUntilExit // Runs the event-loop until loopExit() is called, blocking - // until there are pending or active events. - }; - virtual void run(RunType type) PURE; - - /** - * Returns a factory which connections may use for watermark buffer creation. - * @return the watermark buffer factory for this dispatcher. - */ - virtual Buffer::WatermarkFactory& getWatermarkFactory() PURE; - - /** - * Updates approximate monotonic time to current value. - */ - virtual void updateApproximateMonotonicTime() PURE; + virtual Network::UdpListenerPtr createUdpListener(Network::SocketSharedPtr socket, + Network::UdpListenerCallbacks& cb) PURE; + /** + * Submits an item for deferred delete. @see DeferredDeletable. + */ + virtual void deferredDelete(DeferredDeletablePtr&& to_delete) PURE; + + /** + * Exits the event loop. + */ + virtual void exit() PURE; + + /** + * Listens for a signal event. Only a single dispatcher in the process can listen for signals. + * If more than one dispatcher calls this routine in the process the behavior is undefined. + * + * @param signal_num supplies the signal to listen on. + * @param cb supplies the callback to invoke when the signal fires. + * @return SignalEventPtr a signal event that is owned by the caller. + */ + virtual SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) PURE; + + /** + * Posts a functor to the dispatcher. This is safe cross thread. The functor runs in the context + * of the dispatcher event loop which may be on a different thread than the caller. + */ + virtual void post(PostCb callback) PURE; + + /** + * Runs the event loop. This will not return until exit() is called either from within a callback + * or from a different thread. + * @param type specifies whether to run in blocking mode (run() will not return until exit() is + * called) or non-blocking mode where only active events will be executed and then + * run() will return. + */ + enum class RunType { + Block, // Runs the event-loop until there are no pending events. + NonBlock, // Checks for any pending events to activate, executes them, + // then exits. Exits immediately if there are no pending or + // active events. + RunUntilExit // Runs the event-loop until loopExit() is called, blocking + // until there are pending or active events. }; + virtual void run(RunType type) PURE; - using DispatcherPtr = std::unique_ptr; + /** + * Returns a factory which connections may use for watermark buffer creation. + * @return the watermark buffer factory for this dispatcher. + */ + virtual Buffer::WatermarkFactory& getWatermarkFactory() PURE; + + /** + * Updates approximate monotonic time to current value. + */ + virtual void updateApproximateMonotonicTime() PURE; +}; + +using DispatcherPtr = std::unique_ptr; } // namespace Event -} // namespace Event +} // namespace Envoy From 05ebce8c730388e447a33f43ffe463634bdd0aec Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Mon, 25 Jan 2021 18:49:35 +0200 Subject: [PATCH 13/19] Revert "Fix history of bad merge" This reverts commit 9a5c5455d3b94bd3c6ab9b91e4419267c24ee902. --- include/envoy/event/dispatcher.h | 412 +++++++++++++++---------------- 1 file changed, 200 insertions(+), 212 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 36e34f78bd1a..b9dd1ade0e5d 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -73,223 +73,211 @@ class DispatcherBase { uint32_t events) PURE; /** - * Allocates a timer. @see Timer for docs on how to use the timer. - * @param cb supplies the callback to invoke when the timer fires. + * Abstract event dispatching loop. */ - virtual Event::TimerPtr createTimer(TimerCb cb) PURE; - - /** - * Allocates a scaled timer. @see Timer for docs on how to use the timer. - * @param timer_type the type of timer to create. - * @param cb supplies the callback to invoke when the timer fires. - */ - virtual Event::TimerPtr createScaledTimer(Event::ScaledTimerType timer_type, TimerCb cb) PURE; - - /** - * Allocates a scaled timer. @see Timer for docs on how to use the timer. - * @param minimum the rule for computing the minimum value of the timer. - * @param cb supplies the callback to invoke when the timer fires. - */ - virtual Event::TimerPtr createScaledTimer(Event::ScaledTimerMinimum minimum, TimerCb cb) PURE; - - /** - * Allocates a schedulable callback. @see SchedulableCallback for docs on how to use the wrapped - * callback. - * @param cb supplies the callback to invoke when the SchedulableCallback is triggered on the - * event loop. - */ - virtual Event::SchedulableCallbackPtr createSchedulableCallback(std::function cb) PURE; - - /** - * Appends a tracked object to the current stack of tracked objects operating - * in the dispatcher. - * - * It's recommended to use ScopeTrackerScopeState to manage the object's tracking. If directly - * invoking, there needs to be a subsequent call to popTrackedObject(). - */ - virtual void pushTrackedObject(const ScopeTrackedObject* object) PURE; - - /** - * Removes the top of the stack of tracked object and asserts that it was expected. - */ - virtual void popTrackedObject(const ScopeTrackedObject* expected_object) PURE; - - /** - * Validates that an operation is thread-safe with respect to this dispatcher; i.e. that the - * current thread of execution is on the same thread upon which the dispatcher loop is running. - */ - virtual bool isThreadSafe() const PURE; - - /** - * Returns a recently cached MonotonicTime value. - */ - virtual MonotonicTime approximateMonotonicTime() const PURE; -}; - -/** - * Abstract event dispatching loop. - */ -class Dispatcher : public DispatcherBase { -public: - /** - * Returns the name that identifies this dispatcher, such as "worker_2" or "main_thread". - * @return const std::string& the name that identifies this dispatcher. - */ - virtual const std::string& name() PURE; - - /** - * Register a watchdog for this dispatcher. The dispatcher is responsible for touching the - * watchdog at least once per touch interval. Dispatcher implementations may choose to touch more - * often to avoid spurious miss events when processing long callback queues. - * @param min_touch_interval Touch interval for the watchdog. - */ - virtual void registerWatchdog(const Server::WatchDogSharedPtr& watchdog, - std::chrono::milliseconds min_touch_interval) PURE; - - /** - * Returns a time-source to use with this dispatcher. - */ - virtual TimeSource& timeSource() PURE; - - /** - * Initializes stats for this dispatcher. Note that this can't generally be done at construction - * time, since the main and worker thread dispatchers are constructed before - * ThreadLocalStoreImpl::initializeThreading. - * @param scope the scope to contain the new per-dispatcher stats created here. - * @param prefix the stats prefix to identify this dispatcher. If empty, the dispatcher will be - * identified by its name. - */ - virtual void initializeStats(Stats::Scope& scope, - const absl::optional& prefix = absl::nullopt) PURE; - - /** - * Clears any items in the deferred deletion queue. - */ - virtual void clearDeferredDeleteList() PURE; - - /** - * Wraps an already-accepted socket in an instance of Envoy's server Network::Connection. - * @param socket supplies an open file descriptor and connection metadata to use for the - * connection. Takes ownership of the socket. - * @param transport_socket supplies a transport socket to be used by the connection. - * @param stream_info info object for the server connection - * @return Network::ConnectionPtr a server connection that is owned by the caller. - */ - virtual Network::ServerConnectionPtr - createServerConnection(Network::ConnectionSocketPtr&& socket, - Network::TransportSocketPtr&& transport_socket, - StreamInfo::StreamInfo& stream_info) PURE; - - /** - * Creates an instance of Envoy's Network::ClientConnection. Does NOT initiate the connection; - * the caller must then call connect() on the returned Network::ClientConnection. - * @param address supplies the address to connect to. - * @param source_address supplies an address to bind to or nullptr if no bind is necessary. - * @param transport_socket supplies a transport socket to be used by the connection. - * @param options the socket options to be set on the underlying socket before anything is sent - * on the socket. - * @return Network::ClientConnectionPtr a client connection that is owned by the caller. - */ - virtual Network::ClientConnectionPtr - createClientConnection(Network::Address::InstanceConstSharedPtr address, - Network::Address::InstanceConstSharedPtr source_address, - Network::TransportSocketPtr&& transport_socket, - const Network::ConnectionSocket::OptionsSharedPtr& options) PURE; - - /** - * Creates an async DNS resolver. The resolver should only be used on the thread that runs this - * dispatcher. - * @param resolvers supplies the addresses of DNS resolvers that this resolver should use. If left - * empty, it will not use any specific resolvers, but use defaults (/etc/resolv.conf) - * @param use_tcp_for_dns_lookups if set to true, tcp will be used to perform dns lookups. - * Otherwise, udp is used. - * @return Network::DnsResolverSharedPtr that is owned by the caller. - */ - virtual Network::DnsResolverSharedPtr - createDnsResolver(const std::vector& resolvers, - bool use_tcp_for_dns_lookups) PURE; - - /** - * @return Filesystem::WatcherPtr a filesystem watcher owned by the caller. - */ - virtual Filesystem::WatcherPtr createFilesystemWatcher() PURE; - - /** - * Creates a listener on a specific port. - * @param socket supplies the socket to listen on. - * @param cb supplies the callbacks to invoke for listener events. - * @param bind_to_port controls whether the listener binds to a transport port or not. - * @param backlog_size controls listener pending connections backlog - * @return Network::ListenerPtr a new listener that is owned by the caller. - */ - virtual Network::ListenerPtr createListener(Network::SocketSharedPtr&& socket, - Network::TcpListenerCallbacks& cb, bool bind_to_port, - uint32_t backlog_size) PURE; - - /** - * Creates a logical udp listener on a specific port. - * @param socket supplies the socket to listen on. - * @param cb supplies the udp listener callbacks to invoke for listener events. - * @return Network::ListenerPtr a new listener that is owned by the caller. - */ - virtual Network::UdpListenerPtr createUdpListener(Network::SocketSharedPtr socket, - Network::UdpListenerCallbacks& cb) PURE; - /** - * Submits an item for deferred delete. @see DeferredDeletable. - */ - virtual void deferredDelete(DeferredDeletablePtr&& to_delete) PURE; - - /** - * Exits the event loop. - */ - virtual void exit() PURE; - - /** - * Listens for a signal event. Only a single dispatcher in the process can listen for signals. - * If more than one dispatcher calls this routine in the process the behavior is undefined. - * - * @param signal_num supplies the signal to listen on. - * @param cb supplies the callback to invoke when the signal fires. - * @return SignalEventPtr a signal event that is owned by the caller. - */ - virtual SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) PURE; - - /** - * Posts a functor to the dispatcher. This is safe cross thread. The functor runs in the context - * of the dispatcher event loop which may be on a different thread than the caller. - */ - virtual void post(PostCb callback) PURE; - - /** - * Runs the event loop. This will not return until exit() is called either from within a callback - * or from a different thread. - * @param type specifies whether to run in blocking mode (run() will not return until exit() is - * called) or non-blocking mode where only active events will be executed and then - * run() will return. - */ - enum class RunType { - Block, // Runs the event-loop until there are no pending events. - NonBlock, // Checks for any pending events to activate, executes them, - // then exits. Exits immediately if there are no pending or - // active events. - RunUntilExit // Runs the event-loop until loopExit() is called, blocking - // until there are pending or active events. + class Dispatcher : public DispatcherBase { + public: + /** + * Allocates a timer. @see Timer for docs on how to use the timer. + * @param cb supplies the callback to invoke when the timer fires. + */ + virtual Event::TimerPtr createTimer(TimerCb cb) PURE; + + /** + * Allocates a schedulable callback. @see SchedulableCallback for docs on how to use the wrapped + * callback. + * @param cb supplies the callback to invoke when the SchedulableCallback is triggered on the + * event loop. + */ + virtual Event::SchedulableCallbackPtr createSchedulableCallback(std::function cb) PURE; + + /** + * Sets a tracked object, which is currently operating in this Dispatcher. + * This should be cleared with another call to setTrackedObject() when the object is done doing + * work. Calling setTrackedObject(nullptr) results in no object being tracked. + * + * This is optimized for performance, to avoid allocation where we do scoped object tracking. + * + * @return The previously tracked object or nullptr if there was none. + */ + virtual const ScopeTrackedObject* setTrackedObject(const ScopeTrackedObject* object) PURE; + + /** + * Validates that an operation is thread-safe with respect to this dispatcher; i.e. that the + * current thread of execution is on the same thread upon which the dispatcher loop is running. + */ + virtual bool isThreadSafe() const PURE; + + /** + * Returns a recently cached MonotonicTime value. + */ + virtual MonotonicTime approximateMonotonicTime() const PURE; }; - virtual void run(RunType type) PURE; /** - * Returns a factory which connections may use for watermark buffer creation. - * @return the watermark buffer factory for this dispatcher. + * Abstract event dispatching loop. */ - virtual Buffer::WatermarkFactory& getWatermarkFactory() PURE; - - /** - * Updates approximate monotonic time to current value. - */ - virtual void updateApproximateMonotonicTime() PURE; -}; + class Dispatcher : public DispatcherBase { + public: + /** + * Returns the name that identifies this dispatcher, such as "worker_2" or "main_thread". + * @return const std::string& the name that identifies this dispatcher. + */ + virtual const std::string& name() PURE; + + /** + * Register a watchdog for this dispatcher. The dispatcher is responsible for touching the + * watchdog at least once per touch interval. Dispatcher implementations may choose to touch + * more often to avoid spurious miss events when processing long callback queues. + * @param min_touch_interval Touch interval for the watchdog. + */ + virtual void registerWatchdog(const Server::WatchDogSharedPtr& watchdog, + std::chrono::milliseconds min_touch_interval) PURE; + + /** + * Returns a time-source to use with this dispatcher. + */ + virtual TimeSource& timeSource() PURE; + + /** + * Initializes stats for this dispatcher. Note that this can't generally be done at construction + * time, since the main and worker thread dispatchers are constructed before + * ThreadLocalStoreImpl::initializeThreading. + * @param scope the scope to contain the new per-dispatcher stats created here. + * @param prefix the stats prefix to identify this dispatcher. If empty, the dispatcher will be + * identified by its name. + */ + virtual void initializeStats(Stats::Scope& scope, + const absl::optional& prefix = absl::nullopt) PURE; + + /** + * Clears any items in the deferred deletion queue. + */ + virtual void clearDeferredDeleteList() PURE; + + /** + * Wraps an already-accepted socket in an instance of Envoy's server Network::Connection. + * @param socket supplies an open file descriptor and connection metadata to use for the + * connection. Takes ownership of the socket. + * @param transport_socket supplies a transport socket to be used by the connection. + * @param stream_info info object for the server connection + * @return Network::ConnectionPtr a server connection that is owned by the caller. + */ + virtual Network::ServerConnectionPtr + createServerConnection(Network::ConnectionSocketPtr&& socket, + Network::TransportSocketPtr&& transport_socket, + StreamInfo::StreamInfo& stream_info) PURE; + + /** + * Creates an instance of Envoy's Network::ClientConnection. Does NOT initiate the connection; + * the caller must then call connect() on the returned Network::ClientConnection. + * @param address supplies the address to connect to. + * @param source_address supplies an address to bind to or nullptr if no bind is necessary. + * @param transport_socket supplies a transport socket to be used by the connection. + * @param options the socket options to be set on the underlying socket before anything is sent + * on the socket. + * @return Network::ClientConnectionPtr a client connection that is owned by the caller. + */ + virtual Network::ClientConnectionPtr + createClientConnection(Network::Address::InstanceConstSharedPtr address, + Network::Address::InstanceConstSharedPtr source_address, + Network::TransportSocketPtr&& transport_socket, + const Network::ConnectionSocket::OptionsSharedPtr& options) PURE; + + /** + * Creates an async DNS resolver. The resolver should only be used on the thread that runs this + * dispatcher. + * @param resolvers supplies the addresses of DNS resolvers that this resolver should use. If + * left empty, it will not use any specific resolvers, but use defaults (/etc/resolv.conf) + * @param use_tcp_for_dns_lookups if set to true, tcp will be used to perform dns lookups. + * Otherwise, udp is used. + * @return Network::DnsResolverSharedPtr that is owned by the caller. + */ + virtual Network::DnsResolverSharedPtr + createDnsResolver(const std::vector& resolvers, + bool use_tcp_for_dns_lookups) PURE; + + /** + * @return Filesystem::WatcherPtr a filesystem watcher owned by the caller. + */ + virtual Filesystem::WatcherPtr createFilesystemWatcher() PURE; + + /** + * Creates a listener on a specific port. + * @param socket supplies the socket to listen on. + * @param cb supplies the callbacks to invoke for listener events. + * @param bind_to_port controls whether the listener binds to a transport port or not. + * @param backlog_size controls listener pending connections backlog + * @return Network::ListenerPtr a new listener that is owned by the caller. + */ + virtual Network::ListenerPtr createListener(Network::SocketSharedPtr&& socket, + Network::TcpListenerCallbacks& cb, + bool bind_to_port, uint32_t backlog_size) PURE; + + /** + * Creates a logical udp listener on a specific port. + * @param socket supplies the socket to listen on. + * @param cb supplies the udp listener callbacks to invoke for listener events. + * @return Network::ListenerPtr a new listener that is owned by the caller. + */ + virtual Network::UdpListenerPtr createUdpListener(Network::SocketSharedPtr socket, + Network::UdpListenerCallbacks& cb) PURE; + /** + * Submits an item for deferred delete. @see DeferredDeletable. + */ + virtual void deferredDelete(DeferredDeletablePtr&& to_delete) PURE; + + /** + * Exits the event loop. + */ + virtual void exit() PURE; + + /** + * Listens for a signal event. Only a single dispatcher in the process can listen for signals. + * If more than one dispatcher calls this routine in the process the behavior is undefined. + * + * @param signal_num supplies the signal to listen on. + * @param cb supplies the callback to invoke when the signal fires. + * @return SignalEventPtr a signal event that is owned by the caller. + */ + virtual SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) PURE; + + /** + * Posts a functor to the dispatcher. This is safe cross thread. The functor runs in the context + * of the dispatcher event loop which may be on a different thread than the caller. + */ + virtual void post(PostCb callback) PURE; + + /** + * Runs the event loop. This will not return until exit() is called either from within a + * callback or from a different thread. + * @param type specifies whether to run in blocking mode (run() will not return until exit() is + * called) or non-blocking mode where only active events will be executed and then + * run() will return. + */ + enum class RunType { + Block, // Runs the event-loop until there are no pending events. + NonBlock, // Checks for any pending events to activate, executes them, + // then exits. Exits immediately if there are no pending or + // active events. + RunUntilExit // Runs the event-loop until loopExit() is called, blocking + // until there are pending or active events. + }; + virtual void run(RunType type) PURE; + + /** + * Returns a factory which connections may use for watermark buffer creation. + * @return the watermark buffer factory for this dispatcher. + */ + virtual Buffer::WatermarkFactory& getWatermarkFactory() PURE; + + /** + * Updates approximate monotonic time to current value. + */ + virtual void updateApproximateMonotonicTime() PURE; + }; -using DispatcherPtr = std::unique_ptr; + using DispatcherPtr = std::unique_ptr; } // namespace Event -} // namespace Envoy +} // namespace Event From 9296f96c60c1cdcbff47e5899d508e9ebdac5bc1 Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Mon, 25 Jan 2021 18:49:57 +0200 Subject: [PATCH 14/19] Revert "event: Extract DispatcherBase interface (#14446)" This reverts commit e73281dc992113ea5ae27324600e81bcb7e8ab14. --- include/envoy/event/dispatcher.h | 412 ++++++++++++++++--------------- 1 file changed, 212 insertions(+), 200 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index b9dd1ade0e5d..36e34f78bd1a 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -73,211 +73,223 @@ class DispatcherBase { uint32_t events) PURE; /** - * Abstract event dispatching loop. + * Allocates a timer. @see Timer for docs on how to use the timer. + * @param cb supplies the callback to invoke when the timer fires. */ - class Dispatcher : public DispatcherBase { - public: - /** - * Allocates a timer. @see Timer for docs on how to use the timer. - * @param cb supplies the callback to invoke when the timer fires. - */ - virtual Event::TimerPtr createTimer(TimerCb cb) PURE; - - /** - * Allocates a schedulable callback. @see SchedulableCallback for docs on how to use the wrapped - * callback. - * @param cb supplies the callback to invoke when the SchedulableCallback is triggered on the - * event loop. - */ - virtual Event::SchedulableCallbackPtr createSchedulableCallback(std::function cb) PURE; - - /** - * Sets a tracked object, which is currently operating in this Dispatcher. - * This should be cleared with another call to setTrackedObject() when the object is done doing - * work. Calling setTrackedObject(nullptr) results in no object being tracked. - * - * This is optimized for performance, to avoid allocation where we do scoped object tracking. - * - * @return The previously tracked object or nullptr if there was none. - */ - virtual const ScopeTrackedObject* setTrackedObject(const ScopeTrackedObject* object) PURE; - - /** - * Validates that an operation is thread-safe with respect to this dispatcher; i.e. that the - * current thread of execution is on the same thread upon which the dispatcher loop is running. - */ - virtual bool isThreadSafe() const PURE; - - /** - * Returns a recently cached MonotonicTime value. - */ - virtual MonotonicTime approximateMonotonicTime() const PURE; - }; + virtual Event::TimerPtr createTimer(TimerCb cb) PURE; + + /** + * Allocates a scaled timer. @see Timer for docs on how to use the timer. + * @param timer_type the type of timer to create. + * @param cb supplies the callback to invoke when the timer fires. + */ + virtual Event::TimerPtr createScaledTimer(Event::ScaledTimerType timer_type, TimerCb cb) PURE; + + /** + * Allocates a scaled timer. @see Timer for docs on how to use the timer. + * @param minimum the rule for computing the minimum value of the timer. + * @param cb supplies the callback to invoke when the timer fires. + */ + virtual Event::TimerPtr createScaledTimer(Event::ScaledTimerMinimum minimum, TimerCb cb) PURE; + + /** + * Allocates a schedulable callback. @see SchedulableCallback for docs on how to use the wrapped + * callback. + * @param cb supplies the callback to invoke when the SchedulableCallback is triggered on the + * event loop. + */ + virtual Event::SchedulableCallbackPtr createSchedulableCallback(std::function cb) PURE; + + /** + * Appends a tracked object to the current stack of tracked objects operating + * in the dispatcher. + * + * It's recommended to use ScopeTrackerScopeState to manage the object's tracking. If directly + * invoking, there needs to be a subsequent call to popTrackedObject(). + */ + virtual void pushTrackedObject(const ScopeTrackedObject* object) PURE; + + /** + * Removes the top of the stack of tracked object and asserts that it was expected. + */ + virtual void popTrackedObject(const ScopeTrackedObject* expected_object) PURE; + + /** + * Validates that an operation is thread-safe with respect to this dispatcher; i.e. that the + * current thread of execution is on the same thread upon which the dispatcher loop is running. + */ + virtual bool isThreadSafe() const PURE; + + /** + * Returns a recently cached MonotonicTime value. + */ + virtual MonotonicTime approximateMonotonicTime() const PURE; +}; + +/** + * Abstract event dispatching loop. + */ +class Dispatcher : public DispatcherBase { +public: + /** + * Returns the name that identifies this dispatcher, such as "worker_2" or "main_thread". + * @return const std::string& the name that identifies this dispatcher. + */ + virtual const std::string& name() PURE; + + /** + * Register a watchdog for this dispatcher. The dispatcher is responsible for touching the + * watchdog at least once per touch interval. Dispatcher implementations may choose to touch more + * often to avoid spurious miss events when processing long callback queues. + * @param min_touch_interval Touch interval for the watchdog. + */ + virtual void registerWatchdog(const Server::WatchDogSharedPtr& watchdog, + std::chrono::milliseconds min_touch_interval) PURE; + + /** + * Returns a time-source to use with this dispatcher. + */ + virtual TimeSource& timeSource() PURE; + + /** + * Initializes stats for this dispatcher. Note that this can't generally be done at construction + * time, since the main and worker thread dispatchers are constructed before + * ThreadLocalStoreImpl::initializeThreading. + * @param scope the scope to contain the new per-dispatcher stats created here. + * @param prefix the stats prefix to identify this dispatcher. If empty, the dispatcher will be + * identified by its name. + */ + virtual void initializeStats(Stats::Scope& scope, + const absl::optional& prefix = absl::nullopt) PURE; + + /** + * Clears any items in the deferred deletion queue. + */ + virtual void clearDeferredDeleteList() PURE; + + /** + * Wraps an already-accepted socket in an instance of Envoy's server Network::Connection. + * @param socket supplies an open file descriptor and connection metadata to use for the + * connection. Takes ownership of the socket. + * @param transport_socket supplies a transport socket to be used by the connection. + * @param stream_info info object for the server connection + * @return Network::ConnectionPtr a server connection that is owned by the caller. + */ + virtual Network::ServerConnectionPtr + createServerConnection(Network::ConnectionSocketPtr&& socket, + Network::TransportSocketPtr&& transport_socket, + StreamInfo::StreamInfo& stream_info) PURE; + + /** + * Creates an instance of Envoy's Network::ClientConnection. Does NOT initiate the connection; + * the caller must then call connect() on the returned Network::ClientConnection. + * @param address supplies the address to connect to. + * @param source_address supplies an address to bind to or nullptr if no bind is necessary. + * @param transport_socket supplies a transport socket to be used by the connection. + * @param options the socket options to be set on the underlying socket before anything is sent + * on the socket. + * @return Network::ClientConnectionPtr a client connection that is owned by the caller. + */ + virtual Network::ClientConnectionPtr + createClientConnection(Network::Address::InstanceConstSharedPtr address, + Network::Address::InstanceConstSharedPtr source_address, + Network::TransportSocketPtr&& transport_socket, + const Network::ConnectionSocket::OptionsSharedPtr& options) PURE; + + /** + * Creates an async DNS resolver. The resolver should only be used on the thread that runs this + * dispatcher. + * @param resolvers supplies the addresses of DNS resolvers that this resolver should use. If left + * empty, it will not use any specific resolvers, but use defaults (/etc/resolv.conf) + * @param use_tcp_for_dns_lookups if set to true, tcp will be used to perform dns lookups. + * Otherwise, udp is used. + * @return Network::DnsResolverSharedPtr that is owned by the caller. + */ + virtual Network::DnsResolverSharedPtr + createDnsResolver(const std::vector& resolvers, + bool use_tcp_for_dns_lookups) PURE; + + /** + * @return Filesystem::WatcherPtr a filesystem watcher owned by the caller. + */ + virtual Filesystem::WatcherPtr createFilesystemWatcher() PURE; + + /** + * Creates a listener on a specific port. + * @param socket supplies the socket to listen on. + * @param cb supplies the callbacks to invoke for listener events. + * @param bind_to_port controls whether the listener binds to a transport port or not. + * @param backlog_size controls listener pending connections backlog + * @return Network::ListenerPtr a new listener that is owned by the caller. + */ + virtual Network::ListenerPtr createListener(Network::SocketSharedPtr&& socket, + Network::TcpListenerCallbacks& cb, bool bind_to_port, + uint32_t backlog_size) PURE; /** - * Abstract event dispatching loop. + * Creates a logical udp listener on a specific port. + * @param socket supplies the socket to listen on. + * @param cb supplies the udp listener callbacks to invoke for listener events. + * @return Network::ListenerPtr a new listener that is owned by the caller. */ - class Dispatcher : public DispatcherBase { - public: - /** - * Returns the name that identifies this dispatcher, such as "worker_2" or "main_thread". - * @return const std::string& the name that identifies this dispatcher. - */ - virtual const std::string& name() PURE; - - /** - * Register a watchdog for this dispatcher. The dispatcher is responsible for touching the - * watchdog at least once per touch interval. Dispatcher implementations may choose to touch - * more often to avoid spurious miss events when processing long callback queues. - * @param min_touch_interval Touch interval for the watchdog. - */ - virtual void registerWatchdog(const Server::WatchDogSharedPtr& watchdog, - std::chrono::milliseconds min_touch_interval) PURE; - - /** - * Returns a time-source to use with this dispatcher. - */ - virtual TimeSource& timeSource() PURE; - - /** - * Initializes stats for this dispatcher. Note that this can't generally be done at construction - * time, since the main and worker thread dispatchers are constructed before - * ThreadLocalStoreImpl::initializeThreading. - * @param scope the scope to contain the new per-dispatcher stats created here. - * @param prefix the stats prefix to identify this dispatcher. If empty, the dispatcher will be - * identified by its name. - */ - virtual void initializeStats(Stats::Scope& scope, - const absl::optional& prefix = absl::nullopt) PURE; - - /** - * Clears any items in the deferred deletion queue. - */ - virtual void clearDeferredDeleteList() PURE; - - /** - * Wraps an already-accepted socket in an instance of Envoy's server Network::Connection. - * @param socket supplies an open file descriptor and connection metadata to use for the - * connection. Takes ownership of the socket. - * @param transport_socket supplies a transport socket to be used by the connection. - * @param stream_info info object for the server connection - * @return Network::ConnectionPtr a server connection that is owned by the caller. - */ - virtual Network::ServerConnectionPtr - createServerConnection(Network::ConnectionSocketPtr&& socket, - Network::TransportSocketPtr&& transport_socket, - StreamInfo::StreamInfo& stream_info) PURE; - - /** - * Creates an instance of Envoy's Network::ClientConnection. Does NOT initiate the connection; - * the caller must then call connect() on the returned Network::ClientConnection. - * @param address supplies the address to connect to. - * @param source_address supplies an address to bind to or nullptr if no bind is necessary. - * @param transport_socket supplies a transport socket to be used by the connection. - * @param options the socket options to be set on the underlying socket before anything is sent - * on the socket. - * @return Network::ClientConnectionPtr a client connection that is owned by the caller. - */ - virtual Network::ClientConnectionPtr - createClientConnection(Network::Address::InstanceConstSharedPtr address, - Network::Address::InstanceConstSharedPtr source_address, - Network::TransportSocketPtr&& transport_socket, - const Network::ConnectionSocket::OptionsSharedPtr& options) PURE; - - /** - * Creates an async DNS resolver. The resolver should only be used on the thread that runs this - * dispatcher. - * @param resolvers supplies the addresses of DNS resolvers that this resolver should use. If - * left empty, it will not use any specific resolvers, but use defaults (/etc/resolv.conf) - * @param use_tcp_for_dns_lookups if set to true, tcp will be used to perform dns lookups. - * Otherwise, udp is used. - * @return Network::DnsResolverSharedPtr that is owned by the caller. - */ - virtual Network::DnsResolverSharedPtr - createDnsResolver(const std::vector& resolvers, - bool use_tcp_for_dns_lookups) PURE; - - /** - * @return Filesystem::WatcherPtr a filesystem watcher owned by the caller. - */ - virtual Filesystem::WatcherPtr createFilesystemWatcher() PURE; - - /** - * Creates a listener on a specific port. - * @param socket supplies the socket to listen on. - * @param cb supplies the callbacks to invoke for listener events. - * @param bind_to_port controls whether the listener binds to a transport port or not. - * @param backlog_size controls listener pending connections backlog - * @return Network::ListenerPtr a new listener that is owned by the caller. - */ - virtual Network::ListenerPtr createListener(Network::SocketSharedPtr&& socket, - Network::TcpListenerCallbacks& cb, - bool bind_to_port, uint32_t backlog_size) PURE; - - /** - * Creates a logical udp listener on a specific port. - * @param socket supplies the socket to listen on. - * @param cb supplies the udp listener callbacks to invoke for listener events. - * @return Network::ListenerPtr a new listener that is owned by the caller. - */ - virtual Network::UdpListenerPtr createUdpListener(Network::SocketSharedPtr socket, - Network::UdpListenerCallbacks& cb) PURE; - /** - * Submits an item for deferred delete. @see DeferredDeletable. - */ - virtual void deferredDelete(DeferredDeletablePtr&& to_delete) PURE; - - /** - * Exits the event loop. - */ - virtual void exit() PURE; - - /** - * Listens for a signal event. Only a single dispatcher in the process can listen for signals. - * If more than one dispatcher calls this routine in the process the behavior is undefined. - * - * @param signal_num supplies the signal to listen on. - * @param cb supplies the callback to invoke when the signal fires. - * @return SignalEventPtr a signal event that is owned by the caller. - */ - virtual SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) PURE; - - /** - * Posts a functor to the dispatcher. This is safe cross thread. The functor runs in the context - * of the dispatcher event loop which may be on a different thread than the caller. - */ - virtual void post(PostCb callback) PURE; - - /** - * Runs the event loop. This will not return until exit() is called either from within a - * callback or from a different thread. - * @param type specifies whether to run in blocking mode (run() will not return until exit() is - * called) or non-blocking mode where only active events will be executed and then - * run() will return. - */ - enum class RunType { - Block, // Runs the event-loop until there are no pending events. - NonBlock, // Checks for any pending events to activate, executes them, - // then exits. Exits immediately if there are no pending or - // active events. - RunUntilExit // Runs the event-loop until loopExit() is called, blocking - // until there are pending or active events. - }; - virtual void run(RunType type) PURE; - - /** - * Returns a factory which connections may use for watermark buffer creation. - * @return the watermark buffer factory for this dispatcher. - */ - virtual Buffer::WatermarkFactory& getWatermarkFactory() PURE; - - /** - * Updates approximate monotonic time to current value. - */ - virtual void updateApproximateMonotonicTime() PURE; + virtual Network::UdpListenerPtr createUdpListener(Network::SocketSharedPtr socket, + Network::UdpListenerCallbacks& cb) PURE; + /** + * Submits an item for deferred delete. @see DeferredDeletable. + */ + virtual void deferredDelete(DeferredDeletablePtr&& to_delete) PURE; + + /** + * Exits the event loop. + */ + virtual void exit() PURE; + + /** + * Listens for a signal event. Only a single dispatcher in the process can listen for signals. + * If more than one dispatcher calls this routine in the process the behavior is undefined. + * + * @param signal_num supplies the signal to listen on. + * @param cb supplies the callback to invoke when the signal fires. + * @return SignalEventPtr a signal event that is owned by the caller. + */ + virtual SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) PURE; + + /** + * Posts a functor to the dispatcher. This is safe cross thread. The functor runs in the context + * of the dispatcher event loop which may be on a different thread than the caller. + */ + virtual void post(PostCb callback) PURE; + + /** + * Runs the event loop. This will not return until exit() is called either from within a callback + * or from a different thread. + * @param type specifies whether to run in blocking mode (run() will not return until exit() is + * called) or non-blocking mode where only active events will be executed and then + * run() will return. + */ + enum class RunType { + Block, // Runs the event-loop until there are no pending events. + NonBlock, // Checks for any pending events to activate, executes them, + // then exits. Exits immediately if there are no pending or + // active events. + RunUntilExit // Runs the event-loop until loopExit() is called, blocking + // until there are pending or active events. }; + virtual void run(RunType type) PURE; - using DispatcherPtr = std::unique_ptr; + /** + * Returns a factory which connections may use for watermark buffer creation. + * @return the watermark buffer factory for this dispatcher. + */ + virtual Buffer::WatermarkFactory& getWatermarkFactory() PURE; + + /** + * Updates approximate monotonic time to current value. + */ + virtual void updateApproximateMonotonicTime() PURE; +}; + +using DispatcherPtr = std::unique_ptr; } // namespace Event -} // namespace Event +} // namespace Envoy From 9627507af95c1b7bc956448fcee43bd711aa0da5 Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Mon, 25 Jan 2021 18:52:46 +0200 Subject: [PATCH 15/19] Revert "Fix GCC comnpilation" This reverts commit 23e22efc3a1e874a0c16c159ba9f07433e2f991f. --- source/common/formatter/substitution_formatter.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index 4d23d9ad5b0d..179a20eac1d1 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -123,10 +123,6 @@ class FormatterImpl : public Formatter { std::vector providers_; }; -// Helper classes for StructFormatter::StructFormatMapVisitor. -template struct StructFormatMapVisitorHelper : Ts... { using Ts::operator()...; }; -template StructFormatMapVisitorHelper(Ts...) -> StructFormatMapVisitorHelper; - /** * An formatter for structured log formats, which returns a Struct proto that * can be converted easily into multiple formats. @@ -162,6 +158,8 @@ class StructFormatter { StructFormatListPtr value_; }; + template struct StructFormatMapVisitorHelper : Ts... { using Ts::operator()...; }; + template StructFormatMapVisitorHelper(Ts...) -> StructFormatMapVisitorHelper; using StructFormatMapVisitor = StructFormatMapVisitorHelper< const std::function&)>, const std::function, From 372d3d0da8fe04267bf6ce3eba6bf4682ba7dbc0 Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Mon, 25 Jan 2021 18:53:01 +0200 Subject: [PATCH 16/19] Revert "Fix typo" This reverts commit deea2c69fc9266413b215579ee9829f2c97a2749. --- test/common/formatter/substitution_formatter_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc index 73b7ce9ecd99..100a1218da02 100644 --- a/test/common/formatter/substitution_formatter_test.cc +++ b/test/common/formatter/substitution_formatter_test.cc @@ -2067,7 +2067,7 @@ TEST(SubstitutionFormatterTest, StructFormatterTypedFilterStateTest) { fields.at("test_obj").struct_value().fields().at("inner_key").string_value()); } -// Test new specifier (PLAIN/TYPED) of FilterState. Ensure that after adding additional specifier, +// Test new specifier (PLAIN/TYPED) of FilterState. Ensure that after adding additiona specifier, // the FilterState can call the serializeAsProto or serializeAsString methods correctly. TEST(SubstitutionFormatterTest, FilterStateSpeciferTest) { Http::TestRequestHeaderMapImpl request_headers; From e4fc7ca307844353357ee8588a813edf1caf0e35 Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Mon, 25 Jan 2021 18:53:10 +0200 Subject: [PATCH 17/19] Revert "Restructure nested formatting tests" This reverts commit 9dd6649f46ec56bf7a2bbec20bbaba5fe9cea6bb. --- .../formatter/substitution_formatter_test.cc | 154 +++--------------- 1 file changed, 27 insertions(+), 127 deletions(-) diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc index 100a1218da02..836eae2f1c41 100644 --- a/test/common/formatter/substitution_formatter_test.cc +++ b/test/common/formatter/substitution_formatter_test.cc @@ -1660,7 +1660,7 @@ TEST(SubstitutionFormatterTest, StructFormatterPlainStringTest) { expected_json_map); } -TEST(SubstitutionFormatterTest, StructFormatterTypesTest) { +TEST(SubstitutionFormatterTest, StructFormatterNestedObject) { StreamInfo::MockStreamInfo stream_info; Http::TestRequestHeaderMapImpl request_header; Http::TestResponseHeaderMapImpl response_header; @@ -1674,35 +1674,31 @@ TEST(SubstitutionFormatterTest, StructFormatterTypesTest) { ProtobufWkt::Struct key_mapping; TestUtility::loadFromYaml(R"EOF( - string_type: plain_string_value - struct_type: - plain_string: plain_string_value - protocol: '%PROTOCOL%' - list_type: - - plain_string_value - - '%PROTOCOL%' + level_one: + level_two: + level_three: + plain_string: plain_string_value + protocol: '%PROTOCOL%' )EOF", key_mapping); StructFormatter formatter(key_mapping, false, false); const ProtobufWkt::Struct expected = TestUtility::jsonToStruct(R"EOF({ - "string_type": "plain_string_value", - "struct_type": { - "plain_string": "plain_string_value", - "protocol": "HTTP/1.1" - }, - "list_type": [ - "plain_string_value", - "HTTP/1.1" - ] + "level_one": { + "level_two": { + "level_three": { + "plain_string": "plain_string_value", + "protocol": "HTTP/1.1" + } + } + } })EOF"); const ProtobufWkt::Struct out_struct = formatter.format(request_header, response_header, response_trailer, stream_info, body); EXPECT_TRUE(TestUtility::protoEqual(out_struct, expected)); } -// Test that nested values are formatted properly, including inter-type nesting. -TEST(SubstitutionFormatterTest, StructFormatterNestedObjectsTest) { +TEST(SubstitutionFormatterTest, StructFormatterList) { StreamInfo::MockStreamInfo stream_info; Http::TestRequestHeaderMapImpl request_header; Http::TestResponseHeaderMapImpl response_header; @@ -1715,110 +1711,19 @@ TEST(SubstitutionFormatterTest, StructFormatterNestedObjectsTest) { EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); ProtobufWkt::Struct key_mapping; - // For both struct and list, we test 3 nesting levels of all types (string, struct and list). TestUtility::loadFromYaml(R"EOF( - struct: - struct_string: plain_string_value - struct_protocol: '%PROTOCOL%' - struct_struct: - struct_struct_string: plain_string_value - struct_struct_protocol: '%PROTOCOL%' - struct_struct_struct: - struct_struct_struct_string: plain_string_value - struct_struct_struct_protocol: '%PROTOCOL%' - struct_struct_list: - - struct_struct_list_string - - '%PROTOCOL%' - struct_list: - - struct_list_string - - '%PROTOCOL%' - # struct_list_struct - - struct_list_struct_string: plain_string_value - struct_list_struct_protocol: '%PROTOCOL%' - # struct_list_list - - - struct_list_list_string - - '%PROTOCOL%' list: - - list_string - - '%PROTOCOL%' - # list_struct - - list_struct_string: plain_string_value - list_struct_protocol: '%PROTOCOL%' - list_struct_struct: - list_struct_struct_string: plain_string_value - list_struct_struct_protocol: '%PROTOCOL%' - list_struct_list: - - list_struct_list_string - - '%PROTOCOL%' - # list_list - - - list_list_string - - '%PROTOCOL%' - # list_list_struct - - list_list_struct_string: plain_string_value - list_list_struct_protocol: '%PROTOCOL%' - # list_list_list - - - list_list_list_string - - '%PROTOCOL%' + - plain_string: plain_string_value + - protocol: '%PROTOCOL%' )EOF", key_mapping); StructFormatter formatter(key_mapping, false, false); + const ProtobufWkt::Struct expected = TestUtility::jsonToStruct(R"EOF({ - "struct": { - "struct_string": "plain_string_value", - "struct_protocol": "HTTP/1.1", - "struct_struct": { - "struct_struct_string": "plain_string_value", - "struct_struct_protocol": "HTTP/1.1", - "struct_struct_struct": { - "struct_struct_struct_string": "plain_string_value", - "struct_struct_struct_protocol": "HTTP/1.1", - }, - "struct_struct_list": [ - "struct_struct_list_string", - "HTTP/1.1", - ], - }, - "struct_list": [ - "struct_list_string", - "HTTP/1.1", - { - "struct_list_struct_string": "plain_string_value", - "struct_list_struct_protocol": "HTTP/1.1", - }, - [ - "struct_list_list_string", - "HTTP/1.1", - ], - ], - }, "list": [ - "list_string", - "HTTP/1.1", - { - "list_struct_string": "plain_string_value", - "list_struct_protocol": "HTTP/1.1", - "list_struct_struct": { - "list_struct_struct_string": "plain_string_value", - "list_struct_struct_protocol": "HTTP/1.1", - }, - "list_struct_list": [ - "list_struct_list_string", - "HTTP/1.1", - ] - }, - [ - "list_list_string", - "HTTP/1.1", - { - "list_list_struct_string": "plain_string_value", - "list_list_struct_protocol": "HTTP/1.1", - }, - [ - "list_list_list_string", - "HTTP/1.1", - ], - ], - ], + { "plain_string": "plain_string_value" }, + { "protocol": "HTTP/1.1" }, + ] })EOF"); const ProtobufWkt::Struct out_struct = formatter.format(request_header, response_header, response_trailer, stream_info, body); @@ -1899,14 +1804,10 @@ TEST(SubstitutionFormatterTest, StructFormatterAlternateHeaderTest) { ProtobufWkt::Struct key_mapping; TestUtility::loadFromYaml(R"EOF( - request_present_header_or_request_absent_header: - '%REQ(request_present_header?request_absent_header)%' - request_absent_header_or_request_present_header: - '%REQ(request_absent_header?request_present_header)%' - response_absent_header_or_response_absent_header: - '%RESP(response_absent_header?response_present_header)%' - response_present_header_or_response_absent_header: - '%RESP(response_present_header?response_absent_header)%' + request_present_header_or_request_absent_header: '%REQ(request_present_header?request_absent_header)%' + request_absent_header_or_request_present_header: '%REQ(request_absent_header?request_present_header)%' + response_absent_header_or_response_absent_header: '%RESP(response_absent_header?response_present_header)%' + response_present_header_or_response_absent_header: '%RESP(response_present_header?response_absent_header)%' )EOF", key_mapping); StructFormatter formatter(key_mapping, false, false); @@ -2067,7 +1968,7 @@ TEST(SubstitutionFormatterTest, StructFormatterTypedFilterStateTest) { fields.at("test_obj").struct_value().fields().at("inner_key").string_value()); } -// Test new specifier (PLAIN/TYPED) of FilterState. Ensure that after adding additiona specifier, +// Test new specifier (PLAIN/TYPED) of FilterState. Ensure that after adding additional specifier, // the FilterState can call the serializeAsProto or serializeAsString methods correctly. TEST(SubstitutionFormatterTest, FilterStateSpeciferTest) { Http::TestRequestHeaderMapImpl request_headers; @@ -2197,8 +2098,7 @@ TEST(SubstitutionFormatterTest, StructFormatterMultiTokenTest) { ProtobufWkt::Struct key_mapping; TestUtility::loadFromYaml(R"EOF( - multi_token_field: '%PROTOCOL% plainstring %REQ(some_request_header)% - %RESP(some_response_header)%' + multi_token_field: '%PROTOCOL% plainstring %REQ(some_request_header)% %RESP(some_response_header)%' )EOF", key_mapping); for (const bool preserve_types : {false, true}) { From 6c7faf0d07577d225b815a4c5c242983c4494aa5 Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Mon, 25 Jan 2021 18:53:22 +0200 Subject: [PATCH 18/19] Revert "Fix some method names" This reverts commit 635d6f0043123da9f750b94b6268daa9faa00e6b. --- .../formatter/substitution_formatter.cc | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index cf12239f8030..4ff19b706934 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -149,23 +149,23 @@ StructFormatter::StructFormatter(const ProtobufWkt::Struct& format_mapping, bool bool omit_empty_values) : omit_empty_values_(omit_empty_values), preserve_types_(preserve_types), empty_value_(omit_empty_values_ ? EMPTY_STRING : DefaultUnspecifiedValueString), - struct_output_format_(toFormatMapValue(format_mapping)) {} + struct_output_format_(toFormatValue(format_mapping)) {} StructFormatter::StructFormatMapWrapper -StructFormatter::toFormatMapValue(const ProtobufWkt::Struct& struct_format) const { +StructFormatter::toFormatValue(const ProtobufWkt::Struct& struct_format) const { auto output = std::make_unique(); for (const auto& pair : struct_format.fields()) { switch (pair.second.kind_case()) { case ProtobufWkt::Value::kStringValue: - output->emplace(pair.first, toFormatStringValue(pair.second.string_value())); + output->emplace(pair.first, toFormatValue(pair.second.string_value())); break; case ProtobufWkt::Value::kStructValue: - output->emplace(pair.first, toFormatMapValue(pair.second.struct_value())); + output->emplace(pair.first, toFormatValue(pair.second.struct_value())); break; case ProtobufWkt::Value::kListValue: - output->emplace(pair.first, toFormatListValue(pair.second.list_value())); + output->emplace(pair.first, toFormatValue(pair.second.list_value())); break; default: @@ -182,20 +182,20 @@ StructFormatter::toFormatStringValue(const std::string& string_format) const { }; StructFormatter::StructFormatListWrapper -StructFormatter::toFormatListValue(const ProtobufWkt::ListValue& list_value_format) const { +StructFormatter::toFormatValue(const ProtobufWkt::ListValue& list_value_format) const { auto output = std::make_unique(); for (const auto& value : list_value_format.values()) { switch (value.kind_case()) { case ProtobufWkt::Value::kStringValue: - output->emplace_back(toFormatStringValue(value.string_value())); + output->emplace_back(toFormatValue(value.string_value())); break; case ProtobufWkt::Value::kStructValue: - output->emplace_back(toFormatMapValue(value.struct_value())); + output->emplace_back(toFormatValue(value.struct_value())); break; case ProtobufWkt::Value::kListValue: - output->emplace_back(toFormatListValue(value.list_value())); + output->emplace_back(toFormatValue(value.list_value())); break; default: throw EnvoyException("Only string values, nested structs and list values are " @@ -206,7 +206,7 @@ StructFormatter::toFormatListValue(const ProtobufWkt::ListValue& list_value_form } std::vector -StructFormatter::toFormatStringValue(const std::string& string_format) const { +StructFormatter::toFormatValue(const std::string& string_format) const { return SubstitutionFormatParser::parse(string_format); }; From 48206ebc3503e2bd8ccb993d94993bc1c9cd60a5 Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Mon, 25 Jan 2021 18:53:43 +0200 Subject: [PATCH 19/19] Revert "Add support for proto ListVale formatting" This reverts commit 57c2668abbfcc2420562c7708b9ace58d746b277. --- .../formatter/substitution_formatter.cc | 178 ++++++------------ .../common/formatter/substitution_formatter.h | 50 +---- .../substitution_format_string_test.cc | 3 +- .../formatter/substitution_formatter_test.cc | 32 ---- 4 files changed, 65 insertions(+), 198 deletions(-) diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 4ff19b706934..be4a0130143e 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -49,6 +49,9 @@ const std::regex& getSystemTimeFormatNewlinePattern() { } const std::regex& getNewlinePattern() { CONSTRUCT_ON_FIRST_USE(std::regex, "\n"); } +template struct StructFormatMapVisitor : Ts... { using Ts::operator()...; }; +template StructFormatMapVisitor(Ts...) -> StructFormatMapVisitor; + } // namespace const std::string SubstitutionFormatUtils::DEFAULT_FORMAT = @@ -145,32 +148,20 @@ std::string JsonFormatterImpl::format(const Http::RequestHeaderMap& request_head return absl::StrCat(log_line, "\n"); } -StructFormatter::StructFormatter(const ProtobufWkt::Struct& format_mapping, bool preserve_types, - bool omit_empty_values) - : omit_empty_values_(omit_empty_values), preserve_types_(preserve_types), - empty_value_(omit_empty_values_ ? EMPTY_STRING : DefaultUnspecifiedValueString), - struct_output_format_(toFormatValue(format_mapping)) {} - StructFormatter::StructFormatMapWrapper -StructFormatter::toFormatValue(const ProtobufWkt::Struct& struct_format) const { +StructFormatter::toFormatMap(const ProtobufWkt::Struct& struct_format) const { auto output = std::make_unique(); for (const auto& pair : struct_format.fields()) { switch (pair.second.kind_case()) { case ProtobufWkt::Value::kStringValue: - output->emplace(pair.first, toFormatValue(pair.second.string_value())); + output->emplace(pair.first, SubstitutionFormatParser::parse(pair.second.string_value())); break; - case ProtobufWkt::Value::kStructValue: - output->emplace(pair.first, toFormatValue(pair.second.struct_value())); + output->emplace(pair.first, toFormatMap(pair.second.struct_value())); break; - - case ProtobufWkt::Value::kListValue: - output->emplace(pair.first, toFormatValue(pair.second.list_value())); - break; - default: - throw EnvoyException("Only string values, nested structs and list values are " - "supported in structured access log format."); + throw EnvoyException( + "Only string values or nested structs are supported in structured access log format."); } } return {std::move(output)}; @@ -181,114 +172,57 @@ StructFormatter::toFormatStringValue(const std::string& string_format) const { return SubstitutionFormatParser::parse(string_format); }; -StructFormatter::StructFormatListWrapper -StructFormatter::toFormatValue(const ProtobufWkt::ListValue& list_value_format) const { - auto output = std::make_unique(); - for (const auto& value : list_value_format.values()) { - switch (value.kind_case()) { - case ProtobufWkt::Value::kStringValue: - output->emplace_back(toFormatValue(value.string_value())); - break; - - case ProtobufWkt::Value::kStructValue: - output->emplace_back(toFormatValue(value.struct_value())); - break; - - case ProtobufWkt::Value::kListValue: - output->emplace_back(toFormatValue(value.list_value())); - break; - default: - throw EnvoyException("Only string values, nested structs and list values are " - "supported in structured access log format."); - } - } - return {std::move(output)}; -} - -std::vector -StructFormatter::toFormatValue(const std::string& string_format) const { - return SubstitutionFormatParser::parse(string_format); -}; - -ProtobufWkt::Value StructFormatter::providersCallback( - const std::vector& providers, - const Http::RequestHeaderMap& request_headers, const Http::ResponseHeaderMap& response_headers, - const Http::ResponseTrailerMap& response_trailers, const StreamInfo::StreamInfo& stream_info, - absl::string_view local_reply_body) const { - ASSERT(!providers.empty()); - if (providers.size() == 1) { - const auto& provider = providers.front(); - if (preserve_types_) { - return provider->formatValue(request_headers, response_headers, response_trailers, - stream_info, local_reply_body); - } - - if (omit_empty_values_) { - return ValueUtil::optionalStringValue(provider->format( - request_headers, response_headers, response_trailers, stream_info, local_reply_body)); - } - - const auto str = provider->format(request_headers, response_headers, response_trailers, - stream_info, local_reply_body); - return ValueUtil::stringValue(str.value_or(DefaultUnspecifiedValueString)); - } - // Multiple providers forces string output. - std::string str; - for (const auto& provider : providers) { - const auto bit = provider->format(request_headers, response_headers, response_trailers, - stream_info, local_reply_body); - str += bit.value_or(empty_value_); - } - return ValueUtil::stringValue(str); -} - -ProtobufWkt::Value StructFormatter::structFormatMapCallback( - const StructFormatter::StructFormatMapWrapper& format_map, - const StructFormatter::StructFormatMapVisitor& visitor) const { - ProtobufWkt::Struct output; - auto* fields = output.mutable_fields(); - for (const auto& pair : *format_map.value_) { - ProtobufWkt::Value value = absl::visit(visitor, pair.second); - if (omit_empty_values_ && value.kind_case() == ProtobufWkt::Value::kNullValue) { - continue; - } - (*fields)[pair.first] = value; - } - return ValueUtil::structValue(output); -} - -ProtobufWkt::Value StructFormatter::structFormatListCallback( - const StructFormatter::StructFormatListWrapper& format_list, - const StructFormatter::StructFormatMapVisitor& visitor) const { - std::vector output; - for (const auto& val : *format_list.value_) { - ProtobufWkt::Value value = absl::visit(visitor, val); - if (omit_empty_values_ && value.kind_case() == ProtobufWkt::Value::kNullValue) { - continue; - } - output.push_back(value); - } - return ValueUtil::listValue(output); -} - ProtobufWkt::Struct StructFormatter::format(const Http::RequestHeaderMap& request_headers, const Http::ResponseHeaderMap& response_headers, const Http::ResponseTrailerMap& response_trailers, const StreamInfo::StreamInfo& stream_info, absl::string_view local_reply_body) const { - StructFormatMapVisitor visitor{ - [&](const std::vector& providers) { - return providersCallback(providers, request_headers, response_headers, response_trailers, - stream_info, local_reply_body); - }, - [&, this](const StructFormatter::StructFormatMapWrapper& format_map) { - return structFormatMapCallback(format_map, visitor); - }, - [&, this](const StructFormatter::StructFormatListWrapper& format_list) { - return structFormatListCallback(format_list, visitor); - }, - }; - return structFormatMapCallback(struct_output_format_, visitor).struct_value(); + const std::string& empty_value = + omit_empty_values_ ? EMPTY_STRING : DefaultUnspecifiedValueString; + const std::function&)> + providers_callback = [&](const std::vector& providers) { + ASSERT(!providers.empty()); + if (providers.size() == 1) { + const auto& provider = providers.front(); + if (preserve_types_) { + return provider->formatValue(request_headers, response_headers, response_trailers, + stream_info, local_reply_body); + } + + if (omit_empty_values_) { + return ValueUtil::optionalStringValue( + provider->format(request_headers, response_headers, response_trailers, stream_info, + local_reply_body)); + } + + const auto str = provider->format(request_headers, response_headers, response_trailers, + stream_info, local_reply_body); + return ValueUtil::stringValue(str.value_or(DefaultUnspecifiedValueString)); + } + // Multiple providers forces string output. + std::string str; + for (const auto& provider : providers) { + const auto bit = provider->format(request_headers, response_headers, response_trailers, + stream_info, local_reply_body); + str += bit.value_or(empty_value); + } + return ValueUtil::stringValue(str); + }; + const std::function + struct_format_map_callback = [&](const StructFormatter::StructFormatMapWrapper& format) { + ProtobufWkt::Struct output; + auto* fields = output.mutable_fields(); + StructFormatMapVisitor visitor{struct_format_map_callback, providers_callback}; + for (const auto& pair : *format.value_) { + ProtobufWkt::Value value = absl::visit(visitor, pair.second); + if (omit_empty_values_ && value.kind_case() == ProtobufWkt::Value::kNullValue) { + continue; + } + (*fields)[pair.first] = value; + } + return ValueUtil::structValue(output); + }; + return struct_format_map_callback(struct_output_format_).struct_value(); } void SubstitutionFormatParser::parseCommandHeader(const std::string& token, const size_t start, @@ -1146,8 +1080,8 @@ MetadataFormatter::formatMetadataValue(const envoy::config::core::v3::Metadata& return val; } -// TODO(glicht): Consider adding support for route/listener/cluster metadata as suggested by -// @htuch. See: https://github.com/envoyproxy/envoy/issues/3006 +// TODO(glicht): Consider adding support for route/listener/cluster metadata as suggested by @htuch. +// See: https://github.com/envoyproxy/envoy/issues/3006 DynamicMetadataFormatter::DynamicMetadataFormatter(const std::string& filter_namespace, const std::vector& path, absl::optional max_length) diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index 179a20eac1d1..4d53c03374f6 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -1,7 +1,6 @@ #pragma once #include -#include #include #include @@ -130,7 +129,9 @@ class FormatterImpl : public Formatter { class StructFormatter { public: StructFormatter(const ProtobufWkt::Struct& format_mapping, bool preserve_types, - bool omit_empty_values); + bool omit_empty_values) + : omit_empty_values_(omit_empty_values), preserve_types_(preserve_types), + struct_output_format_(toFormatMap(format_mapping)) {} ProtobufWkt::Struct format(const Http::RequestHeaderMap& request_headers, const Http::ResponseHeaderMap& response_headers, @@ -140,57 +141,22 @@ class StructFormatter { private: struct StructFormatMapWrapper; - struct StructFormatListWrapper; - using StructFormatValue = - absl::variant, const StructFormatMapWrapper, - const StructFormatListWrapper>; + using StructFormatMapValue = + absl::variant, const StructFormatMapWrapper>; // Although not required for Struct/JSON, it is nice to have the order of // properties preserved between the format and the log entry, thus std::map. - using StructFormatMap = std::map; + using StructFormatMap = std::map; using StructFormatMapPtr = std::unique_ptr; struct StructFormatMapWrapper { StructFormatMapPtr value_; }; - using StructFormatList = std::list; - using StructFormatListPtr = std::unique_ptr; - struct StructFormatListWrapper { - StructFormatListPtr value_; - }; - - template struct StructFormatMapVisitorHelper : Ts... { using Ts::operator()...; }; - template StructFormatMapVisitorHelper(Ts...) -> StructFormatMapVisitorHelper; - using StructFormatMapVisitor = StructFormatMapVisitorHelper< - const std::function&)>, - const std::function, - const std::function>; - - // Methods for building the format map. - std::vector toFormatValue(const std::string& string_format) const; - StructFormatMapWrapper toFormatValue(const ProtobufWkt::Struct& struct_format) const; - StructFormatListWrapper toFormatValue(const ProtobufWkt::ListValue& list_value_format) const; - - // Methods for doing the actual formatting. - ProtobufWkt::Value providersCallback(const std::vector& providers, - const Http::RequestHeaderMap& request_headers, - const Http::ResponseHeaderMap& response_headers, - const Http::ResponseTrailerMap& response_trailers, - const StreamInfo::StreamInfo& stream_info, - absl::string_view local_reply_body) const; - ProtobufWkt::Value - structFormatMapCallback(const StructFormatter::StructFormatMapWrapper& format_map, - const StructFormatMapVisitor& visitor) const; - ProtobufWkt::Value - structFormatListCallback(const StructFormatter::StructFormatListWrapper& format_list, - const StructFormatMapVisitor& visitor) const; + StructFormatMapWrapper toFormatMap(const ProtobufWkt::Struct& struct_format) const; const bool omit_empty_values_; const bool preserve_types_; - const std::string empty_value_; const StructFormatMapWrapper struct_output_format_; -}; // namespace Formatter - -using StructFormatterPtr = std::unique_ptr; +}; class JsonFormatterImpl : public Formatter { public: diff --git a/test/common/formatter/substitution_format_string_test.cc b/test/common/formatter/substitution_format_string_test.cc index 37e83fe3caa9..3b7bd29abad8 100644 --- a/test/common/formatter/substitution_format_string_test.cc +++ b/test/common/formatter/substitution_format_string_test.cc @@ -95,8 +95,7 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestInvalidConfigs) { TestUtility::loadFromYaml(yaml, config_); EXPECT_THROW_WITH_MESSAGE( SubstitutionFormatStringUtils::fromProtoConfig(config_, context_.api()), EnvoyException, - "Only string values, nested structs and list values are supported in structured access log " - "format."); + "Only string values or nested structs are supported in structured access log format."); } } diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc index 836eae2f1c41..75e9a2c8abdb 100644 --- a/test/common/formatter/substitution_formatter_test.cc +++ b/test/common/formatter/substitution_formatter_test.cc @@ -1698,38 +1698,6 @@ TEST(SubstitutionFormatterTest, StructFormatterNestedObject) { EXPECT_TRUE(TestUtility::protoEqual(out_struct, expected)); } -TEST(SubstitutionFormatterTest, StructFormatterList) { - StreamInfo::MockStreamInfo stream_info; - Http::TestRequestHeaderMapImpl request_header; - Http::TestResponseHeaderMapImpl response_header; - Http::TestResponseTrailerMapImpl response_trailer; - std::string body; - - envoy::config::core::v3::Metadata metadata; - populateMetadataTestData(metadata); - absl::optional protocol = Http::Protocol::Http11; - EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); - - ProtobufWkt::Struct key_mapping; - TestUtility::loadFromYaml(R"EOF( - list: - - plain_string: plain_string_value - - protocol: '%PROTOCOL%' - )EOF", - key_mapping); - StructFormatter formatter(key_mapping, false, false); - - const ProtobufWkt::Struct expected = TestUtility::jsonToStruct(R"EOF({ - "list": [ - { "plain_string": "plain_string_value" }, - { "protocol": "HTTP/1.1" }, - ] - })EOF"); - const ProtobufWkt::Struct out_struct = - formatter.format(request_header, response_header, response_trailer, stream_info, body); - EXPECT_TRUE(TestUtility::protoEqual(out_struct, expected)); -} - TEST(SubstitutionFormatterTest, StructFormatterSingleOperatorTest) { StreamInfo::MockStreamInfo stream_info; Http::TestRequestHeaderMapImpl request_header;