From c19e5dcd6b4b22cc00ca61a6b836ff3edd44e16d Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 18 Oct 2023 12:31:38 -0700 Subject: [PATCH] [build] Remove linkage of logging_filter -> otel (#34729) Reverse dependency edge, so instead of saying `logging_filter _after_ otel`, instead say `otel _before_ logging_filter` - since this doesn't inadvertently bring otel into builds where it's unnecessary. Required moving filter class definitions into the header - which mirrors all other filters, so I think this is fine. Also required removing the bespoke visibility rules on logging_filter - which also seems relatively fine (the defaults limit to grpc usage, and it's hard to see a firm requirement for tighter visibility that that). --- BUILD | 1 + src/core/BUILD | 5 - .../ext/filters/logging/logging_filter.cc | 366 +++++++++--------- src/core/ext/filters/logging/logging_filter.h | 39 ++ src/cpp/ext/filters/census/grpc_plugin.cc | 8 +- 5 files changed, 219 insertions(+), 200 deletions(-) diff --git a/BUILD b/BUILD index 9be5d55d0a0..a242028f8d6 100644 --- a/BUILD +++ b/BUILD @@ -2372,6 +2372,7 @@ grpc_cc_library( "//src/core:channel_stack_type", "//src/core:context", "//src/core:error", + "//src/core:logging_filter", "//src/core:slice", "//src/core:slice_buffer", "//src/core:slice_refcount", diff --git a/src/core/BUILD b/src/core/BUILD index ed9a9325d94..7a1f088fe12 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -6100,10 +6100,6 @@ grpc_cc_library( "absl/types:optional", ], language = "c++", - visibility = [ - "//src/cpp/ext/gcp:__subpackages__", - "//test:__subpackages__", - ], deps = [ "arena", "arena_promise", @@ -6124,7 +6120,6 @@ grpc_cc_library( "//:gpr_platform", "//:grpc_base", "//:grpc_client_channel", - "//:grpc_opencensus_plugin", "//:grpc_public_hdrs", "//:grpc_resolver", "//:legacy_context", diff --git a/src/core/ext/filters/logging/logging_filter.cc b/src/core/ext/filters/logging/logging_filter.cc index 0e80dca4f7b..85e4871a086 100644 --- a/src/core/ext/filters/logging/logging_filter.cc +++ b/src/core/ext/filters/logging/logging_filter.cc @@ -54,7 +54,6 @@ #include "src/core/lib/channel/channel_fwd.h" #include "src/core/lib/channel/channel_stack.h" #include "src/core/lib/channel/context.h" -#include "src/core/lib/channel/promise_based_filter.h" #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/gprpp/host_port.h" #include "src/core/lib/gprpp/time.h" @@ -71,7 +70,6 @@ #include "src/core/lib/transport/metadata_batch.h" #include "src/core/lib/transport/transport.h" #include "src/core/lib/uri/uri_parser.h" -#include "src/cpp/ext/filters/census/client_filter.h" namespace grpc_core { @@ -342,112 +340,104 @@ class CallData { LoggingSink::Config config_; }; -class ClientLoggingFilter final : public ChannelFilter { - public: - static const grpc_channel_filter kFilter; - - static absl::StatusOr Create( - const ChannelArgs& args, ChannelFilter::Args /*filter_args*/) { - absl::optional default_authority = - args.GetString(GRPC_ARG_DEFAULT_AUTHORITY); - if (default_authority.has_value()) { - return ClientLoggingFilter(std::string(default_authority.value())); - } - absl::optional server_uri = - args.GetOwnedString(GRPC_ARG_SERVER_URI); - if (server_uri.has_value()) { - return ClientLoggingFilter( - CoreConfiguration::Get().resolver_registry().GetDefaultAuthority( - *server_uri)); - } - return ClientLoggingFilter(""); - } +} // namespace - // Construct a promise for one call. - ArenaPromise MakeCallPromise( - CallArgs call_args, NextPromiseFactory next_promise_factory) override { - CallData* calld = GetContext()->ManagedNew( - true, call_args, default_authority_); - if (!calld->ShouldLog()) { - return next_promise_factory(std::move(call_args)); - } - calld->LogClientHeader( - /*is_client=*/true, - static_cast( - GetContext() - [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] - .value), - call_args.client_initial_metadata); - call_args.server_initial_metadata->InterceptAndMap( - [calld](ServerMetadataHandle metadata) { - calld->LogServerHeader( - /*is_client=*/true, - static_cast( - GetContext() - [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] - .value), - metadata.get()); - return metadata; - }); - call_args.client_to_server_messages->InterceptAndMapWithHalfClose( - [calld](MessageHandle message) { - calld->LogClientMessage( - /*is_client=*/true, - static_cast( - GetContext() - [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] - .value), - message->payload()); - return message; - }, - [calld] { - calld->LogClientHalfClose( - /*is_client=*/true, - static_cast( - GetContext() - [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] - .value)); - }); - call_args.server_to_client_messages->InterceptAndMap( - [calld](MessageHandle message) { - calld->LogServerMessage( - /*is_client=*/true, - static_cast( - GetContext() - [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] - .value), - message->payload()); - return message; - }); - return OnCancel( - Map(next_promise_factory(std::move(call_args)), - [calld](ServerMetadataHandle md) { - calld->LogServerTrailer( - /*is_client=*/true, - static_cast( - GetContext() - [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] - .value), - md.get()); - return md; - }), - // TODO(yashykt/ctiller): GetContext is not - // valid for the cancellation function requiring us to capture it here. - // This ought to be easy to fix once client side promises are completely - // rolled out. - [calld, ctx = GetContext()]() { - calld->LogCancel( - /*is_client=*/true, - static_cast( - ctx[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value)); - }); +absl::StatusOr ClientLoggingFilter::Create( + const ChannelArgs& args, ChannelFilter::Args /*filter_args*/) { + absl::optional default_authority = + args.GetString(GRPC_ARG_DEFAULT_AUTHORITY); + if (default_authority.has_value()) { + return ClientLoggingFilter(std::string(default_authority.value())); } + absl::optional server_uri = + args.GetOwnedString(GRPC_ARG_SERVER_URI); + if (server_uri.has_value()) { + return ClientLoggingFilter( + CoreConfiguration::Get().resolver_registry().GetDefaultAuthority( + *server_uri)); + } + return ClientLoggingFilter(""); +} - private: - explicit ClientLoggingFilter(std::string default_authority) - : default_authority_(std::move(default_authority)) {} - std::string default_authority_; -}; +// Construct a promise for one call. +ArenaPromise ClientLoggingFilter::MakeCallPromise( + CallArgs call_args, NextPromiseFactory next_promise_factory) { + CallData* calld = GetContext()->ManagedNew( + true, call_args, default_authority_); + if (!calld->ShouldLog()) { + return next_promise_factory(std::move(call_args)); + } + calld->LogClientHeader( + /*is_client=*/true, + static_cast( + GetContext() + [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] + .value), + call_args.client_initial_metadata); + call_args.server_initial_metadata->InterceptAndMap( + [calld](ServerMetadataHandle metadata) { + calld->LogServerHeader( + /*is_client=*/true, + static_cast( + GetContext() + [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] + .value), + metadata.get()); + return metadata; + }); + call_args.client_to_server_messages->InterceptAndMapWithHalfClose( + [calld](MessageHandle message) { + calld->LogClientMessage( + /*is_client=*/true, + static_cast( + GetContext() + [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] + .value), + message->payload()); + return message; + }, + [calld] { + calld->LogClientHalfClose( + /*is_client=*/true, + static_cast( + GetContext() + [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] + .value)); + }); + call_args.server_to_client_messages->InterceptAndMap( + [calld](MessageHandle message) { + calld->LogServerMessage( + /*is_client=*/true, + static_cast( + GetContext() + [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] + .value), + message->payload()); + return message; + }); + return OnCancel( + Map(next_promise_factory(std::move(call_args)), + [calld](ServerMetadataHandle md) { + calld->LogServerTrailer( + /*is_client=*/true, + static_cast( + GetContext() + [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] + .value), + md.get()); + return md; + }), + // TODO(yashykt/ctiller): GetContext is not + // valid for the cancellation function requiring us to capture it here. + // This ought to be easy to fix once client side promises are completely + // rolled out. + [calld, ctx = GetContext()]() { + calld->LogCancel( + /*is_client=*/true, + static_cast( + ctx[GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE].value)); + }); +} const grpc_channel_filter ClientLoggingFilter::kFilter = MakePromiseBasedFilter("logging"); -class ServerLoggingFilter final : public ChannelFilter { - public: - static const grpc_channel_filter kFilter; - - static absl::StatusOr Create( - const ChannelArgs& /*args*/, ChannelFilter::Args /*filter_args*/) { - return ServerLoggingFilter(); - } +absl::StatusOr ServerLoggingFilter::Create( + const ChannelArgs& /*args*/, ChannelFilter::Args /*filter_args*/) { + return ServerLoggingFilter(); +} - // Construct a promise for one call. - ArenaPromise MakeCallPromise( - CallArgs call_args, NextPromiseFactory next_promise_factory) override { - CallData* calld = GetContext()->ManagedNew( - false, call_args, /*default_authority=*/""); - if (!calld->ShouldLog()) { - return next_promise_factory(std::move(call_args)); - } - auto* call_tracer = static_cast( - GetContext() - [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] - .value); - calld->LogClientHeader( - /*is_client=*/false, call_tracer, call_args.client_initial_metadata); - call_args.server_initial_metadata->InterceptAndMap( - [calld](ServerMetadataHandle metadata) { - calld->LogServerHeader( - /*is_client=*/false, - static_cast( - GetContext() - [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] - .value), - metadata.get()); - return metadata; - }); - call_args.client_to_server_messages->InterceptAndMapWithHalfClose( - [calld](MessageHandle message) { - calld->LogClientMessage( - /*is_client=*/false, - static_cast( - GetContext() - [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] - .value), - message->payload()); - return message; - }, - [calld] { - calld->LogClientHalfClose( - /*is_client=*/false, - static_cast( - GetContext() - [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] - .value)); - }); - call_args.server_to_client_messages->InterceptAndMap( - [calld](MessageHandle message) { - calld->LogServerMessage( - /*is_client=*/false, - static_cast( - GetContext() - [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] - .value), - message->payload()); - return message; - }); - return OnCancel( - Map(next_promise_factory(std::move(call_args)), - [calld](ServerMetadataHandle md) { - calld->LogServerTrailer( - /*is_client=*/false, - static_cast( - GetContext() - [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] - .value), - md.get()); - return md; - }), - // TODO(yashykt/ctiller): GetContext is not - // valid for the cancellation function requiring us to capture - // call_tracer. - [calld, call_tracer]() { - calld->LogCancel( - /*is_client=*/false, call_tracer); - }); +// Construct a promise for one call. +ArenaPromise ServerLoggingFilter::MakeCallPromise( + CallArgs call_args, NextPromiseFactory next_promise_factory) { + CallData* calld = GetContext()->ManagedNew( + false, call_args, /*default_authority=*/""); + if (!calld->ShouldLog()) { + return next_promise_factory(std::move(call_args)); } -}; + auto* call_tracer = static_cast( + GetContext() + [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] + .value); + calld->LogClientHeader( + /*is_client=*/false, call_tracer, call_args.client_initial_metadata); + call_args.server_initial_metadata->InterceptAndMap( + [calld](ServerMetadataHandle metadata) { + calld->LogServerHeader( + /*is_client=*/false, + static_cast( + GetContext() + [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] + .value), + metadata.get()); + return metadata; + }); + call_args.client_to_server_messages->InterceptAndMapWithHalfClose( + [calld](MessageHandle message) { + calld->LogClientMessage( + /*is_client=*/false, + static_cast( + GetContext() + [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] + .value), + message->payload()); + return message; + }, + [calld] { + calld->LogClientHalfClose( + /*is_client=*/false, + static_cast( + GetContext() + [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] + .value)); + }); + call_args.server_to_client_messages->InterceptAndMap( + [calld](MessageHandle message) { + calld->LogServerMessage( + /*is_client=*/false, + static_cast( + GetContext() + [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] + .value), + message->payload()); + return message; + }); + return OnCancel( + Map(next_promise_factory(std::move(call_args)), + [calld](ServerMetadataHandle md) { + calld->LogServerTrailer( + /*is_client=*/false, + static_cast( + GetContext() + [GRPC_CONTEXT_CALL_TRACER_ANNOTATION_INTERFACE] + .value), + md.get()); + return md; + }), + // TODO(yashykt/ctiller): GetContext is not + // valid for the cancellation function requiring us to capture + // call_tracer. + [calld, call_tracer]() { + calld->LogCancel( + /*is_client=*/false, call_tracer); + }); +} const grpc_channel_filter ServerLoggingFilter::kFilter = MakePromiseBasedFilter("logging"); -} // namespace - void RegisterLoggingFilter(LoggingSink* sink) { g_logging_sink = sink; CoreConfiguration::RegisterBuilder([](CoreConfiguration::Builder* builder) { @@ -559,8 +542,7 @@ void RegisterLoggingFilter(LoggingSink* sink) { builder->channel_init() ->RegisterFilter(GRPC_CLIENT_CHANNEL, &ClientLoggingFilter::kFilter) // TODO(yashykt) : Figure out a good place to place this channel arg - .IfChannelArg("grpc.experimental.enable_observability", true) - .After({&grpc::internal::OpenCensusClientFilter::kFilter}); + .IfChannelArg("grpc.experimental.enable_observability", true); }); } diff --git a/src/core/ext/filters/logging/logging_filter.h b/src/core/ext/filters/logging/logging_filter.h index b2a5efb6c80..6a27a653860 100644 --- a/src/core/ext/filters/logging/logging_filter.h +++ b/src/core/ext/filters/logging/logging_filter.h @@ -21,10 +21,49 @@ #include +#include +#include + +#include "absl/status/statusor.h" + #include "src/core/ext/filters/logging/logging_sink.h" +#include "src/core/lib/channel/channel_args.h" +#include "src/core/lib/channel/channel_fwd.h" +#include "src/core/lib/channel/promise_based_filter.h" +#include "src/core/lib/promise/arena_promise.h" +#include "src/core/lib/transport/transport.h" namespace grpc_core { +class ClientLoggingFilter final : public ChannelFilter { + public: + static const grpc_channel_filter kFilter; + + static absl::StatusOr Create( + const ChannelArgs& args, ChannelFilter::Args /*filter_args*/); + + // Construct a promise for one call. + ArenaPromise MakeCallPromise( + CallArgs call_args, NextPromiseFactory next_promise_factory) override; + + private: + explicit ClientLoggingFilter(std::string default_authority) + : default_authority_(std::move(default_authority)) {} + std::string default_authority_; +}; + +class ServerLoggingFilter final : public ChannelFilter { + public: + static const grpc_channel_filter kFilter; + + static absl::StatusOr Create( + const ChannelArgs& args, ChannelFilter::Args /*filter_args*/); + + // Construct a promise for one call. + ArenaPromise MakeCallPromise( + CallArgs call_args, NextPromiseFactory next_promise_factory) override; +}; + void RegisterLoggingFilter(LoggingSink* sink); } // namespace grpc_core diff --git a/src/cpp/ext/filters/census/grpc_plugin.cc b/src/cpp/ext/filters/census/grpc_plugin.cc index 630bd45ded9..f875f374a77 100644 --- a/src/cpp/ext/filters/census/grpc_plugin.cc +++ b/src/cpp/ext/filters/census/grpc_plugin.cc @@ -30,6 +30,7 @@ #include #include +#include "src/core/ext/filters/logging/logging_filter.h" #include "src/core/lib/channel/call_tracer.h" #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/surface/channel_stack_type.h" @@ -44,9 +45,10 @@ void RegisterOpenCensusPlugin() { new grpc::internal::OpenCensusServerCallTracerFactory); grpc_core::CoreConfiguration::RegisterBuilder( [](grpc_core::CoreConfiguration::Builder* builder) { - builder->channel_init()->RegisterFilter( - GRPC_CLIENT_CHANNEL, - &grpc::internal::OpenCensusClientFilter::kFilter); + builder->channel_init() + ->RegisterFilter(GRPC_CLIENT_CHANNEL, + &grpc::internal::OpenCensusClientFilter::kFilter) + .Before({&grpc_core::ClientLoggingFilter::kFilter}); }); // Access measures to ensure they are initialized. Otherwise, creating a view