From a41d9ad6f8e0c8f8b92c8f7daf8b6c9d810b1bfa Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 28 Feb 2024 20:42:37 +0000 Subject: [PATCH 01/12] build(deps): bump slack-sdk from 3.27.0 to 3.27.1 in /tools/base (#32615) Bumps [slack-sdk](https://github.com/slackapi/python-slack-sdk) from 3.27.0 to 3.27.1. - [Release notes](https://github.com/slackapi/python-slack-sdk/releases) - [Changelog](https://github.com/slackapi/python-slack-sdk/blob/main/docs-v2/changelog.html) - [Commits](https://github.com/slackapi/python-slack-sdk/compare/v3.27.0...v3.27.1) --- updated-dependencies: - dependency-name: slack-sdk dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- tools/base/requirements.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/base/requirements.txt b/tools/base/requirements.txt index 87cfe35fb798..26f39ac48a40 100644 --- a/tools/base/requirements.txt +++ b/tools/base/requirements.txt @@ -1220,9 +1220,9 @@ six==1.16.0 \ # pyu2f # sphinxcontrib-httpdomain # thrift -slack-sdk==3.27.0 \ - --hash=sha256:811472ce598db855ab3c02f098fa430323ccb253cfe17ba20c7b05ab206d984d \ - --hash=sha256:a901c68cb5547d5459cdefd81343d116db56d65f6b33f4081ddf1cdd243bf07e +slack-sdk==3.27.1 \ + --hash=sha256:85d86b34d807c26c8bb33c1569ec0985876f06ae4a2692afba765b7a5490d28c \ + --hash=sha256:c108e509160cf1324c5c8b1f47ca52fb5e287021b8caf9f4ec78ad737ab7b1d9 # via -r requirements.in smmap==5.0.1 \ --hash=sha256:dceeb6c0028fdb6734471eb07c0cd2aae706ccaecab45965ee83f11c8d3b1f62 \ From 50ed6189e7e4f588c45e500e511c5d22eaa19380 Mon Sep 17 00:00:00 2001 From: Kuo-Chung Hsu Date: Wed, 28 Feb 2024 13:03:05 -0800 Subject: [PATCH 02/12] use createScope to guruantee longer lifetime than shadow writer (#32595) Signed-off-by: kuochunghsu --- .../network/thrift_proxy/router/router.h | 17 ++++++++++------- .../network/thrift_proxy/router/router_impl.cc | 2 +- .../thrift_proxy/router/shadow_writer_impl.cc | 2 +- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/source/extensions/filters/network/thrift_proxy/router/router.h b/source/extensions/filters/network/thrift_proxy/router/router.h index 0ffff508c81f..851f04ccc06a 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router.h +++ b/source/extensions/filters/network/thrift_proxy/router/router.h @@ -129,9 +129,10 @@ class RouterStats { public: RouterStats(const std::string& stat_prefix, Stats::Scope& scope, const LocalInfo::LocalInfo& local_info) - : named_(RouterNamedStats::generateStats(stat_prefix, scope)), - stat_name_set_(scope.symbolTable().makeSet("thrift_proxy")), - symbol_table_(scope.symbolTable()), + : stats_scope_(scope.createScope("")), + named_(RouterNamedStats::generateStats(stat_prefix, *stats_scope_)), + stat_name_set_(stats_scope_->symbolTable().makeSet("thrift_proxy")), + symbol_table_(stats_scope_->symbolTable()), upstream_rq_call_(stat_name_set_->add("thrift.upstream_rq_call")), upstream_rq_oneway_(stat_name_set_->add("thrift.upstream_rq_oneway")), upstream_rq_invalid_type_(stat_name_set_->add("thrift.upstream_rq_invalid_type")), @@ -340,7 +341,7 @@ class RouterStats { Stats::Histogram::Unit::Milliseconds, value); } - const RouterNamedStats named_; + const RouterNamedStats& routerStats() const { return named_; } private: void incClusterScopeCounter(const Upstream::ClusterInfo& cluster, @@ -385,6 +386,8 @@ class RouterStats { return symbol_table_.join({zone_, local_zone_name_, upstream_zone_name, stat_name}); } + Stats::ScopeSharedPtr stats_scope_; + const RouterNamedStats named_; Stats::StatNameSetPtr stat_name_set_; Stats::SymbolTable& symbol_table_; const Stats::StatName upstream_rq_call_; @@ -506,7 +509,7 @@ class RequestOwner : public ProtocolConverter, public Logger::LoggablemaintenanceMode()) { - stats().named_.upstream_rq_maintenance_mode_.inc(); + stats().routerStats().upstream_rq_maintenance_mode_.inc(); if (metadata->messageType() == MessageType::Call) { stats().incResponseLocalException(*cluster_); } @@ -551,7 +554,7 @@ class RequestOwner : public ProtocolConverter, public Logger::LoggabletcpConnPool(Upstream::ResourcePriority::Default, lb_context); if (!conn_pool_data) { - stats().named_.no_healthy_upstream_.inc(); + stats().routerStats().no_healthy_upstream_.inc(); if (metadata->messageType() == MessageType::Call) { stats().incResponseLocalException(*cluster_); } diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc index 4075376459c8..445a23690b39 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc @@ -278,7 +278,7 @@ FilterStatus Router::messageBegin(MessageMetadataSharedPtr metadata) { route_ = callbacks_->route(); if (!route_) { ENVOY_STREAM_LOG(debug, "no route match for method '{}'", *callbacks_, metadata->methodName()); - stats().named_.route_missing_.inc(); + stats().routerStats().route_missing_.inc(); callbacks_->sendLocalReply( AppException(AppExceptionType::UnknownMethod, fmt::format("no route for method '{}'", metadata->methodName())), diff --git a/source/extensions/filters/network/thrift_proxy/router/shadow_writer_impl.cc b/source/extensions/filters/network/thrift_proxy/router/shadow_writer_impl.cc index 7c27c675d55f..2ab6544b1a0a 100644 --- a/source/extensions/filters/network/thrift_proxy/router/shadow_writer_impl.cc +++ b/source/extensions/filters/network/thrift_proxy/router/shadow_writer_impl.cc @@ -22,7 +22,7 @@ OptRef ShadowWriterImpl::submit(const std::string& cluster_n original_transport, original_protocol); const bool created = shadow_router->createUpstreamRequest(); if (!created || !tls_.get().has_value()) { - stats_.named_.shadow_request_submit_failure_.inc(); + stats_.routerStats().shadow_request_submit_failure_.inc(); return absl::nullopt; } From 6f7645516801be3a8259aefd2faad412620f3c1c Mon Sep 17 00:00:00 2001 From: Keith Smiley Date: Wed, 28 Feb 2024 15:20:23 -0800 Subject: [PATCH 03/12] bazel: fix opentelemetry-cpp warning (#32624) Fixes https://github.com/envoyproxy/envoy/issues/32591 Signed-off-by: Keith Smiley --- bazel/io_opentelemetry_cpp.patch | 13 +++++++++++++ bazel/repositories.bzl | 6 +++++- 2 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 bazel/io_opentelemetry_cpp.patch diff --git a/bazel/io_opentelemetry_cpp.patch b/bazel/io_opentelemetry_cpp.patch new file mode 100644 index 000000000000..6eb4cfe2a9a5 --- /dev/null +++ b/bazel/io_opentelemetry_cpp.patch @@ -0,0 +1,13 @@ +# TODO: Remove once https://github.com/open-telemetry/opentelemetry-cpp/issues/2556 is merged + +--- a/api/include/opentelemetry/trace/span_context.h ++++ b/api/include/opentelemetry/trace/span_context.h +@@ -30,7 +30,7 @@ class SpanContext final + SpanContext(bool sampled_flag, bool is_remote) noexcept + : trace_id_(), + span_id_(), +- trace_flags_(trace::TraceFlags((uint8_t)sampled_flag)), ++ trace_flags_(trace::TraceFlags(static_cast(sampled_flag))), + is_remote_(is_remote), + trace_state_(TraceState::GetDefault()) + {} diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index b5ddba3eeacf..025bf5efdb87 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -777,7 +777,11 @@ def _io_opentracing_cpp(): ) def _io_opentelemetry_api_cpp(): - external_http_archive("io_opentelemetry_cpp") + external_http_archive( + name = "io_opentelemetry_cpp", + patch_args = ["-p1"], + patches = ["@envoy//bazel:io_opentelemetry_cpp.patch"], + ) native.bind( name = "opentelemetry_api", actual = "@io_opentelemetry_cpp//api:api", From eb51ea0ff70a40cd4bbafc6f03333e38c62a2ca3 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 28 Feb 2024 16:21:38 -0800 Subject: [PATCH 04/12] Fix circular dependency between matchers and stats, grpc (#32587) Signed-off-by: Greg Greenway --- .../filters/network/source/BUILD | 1 + source/common/access_log/access_log_impl.cc | 1 + source/common/common/BUILD | 1 + source/common/config/BUILD | 18 ++++++++---- source/common/config/stats_utility.cc | 17 +++++++++++ source/common/config/stats_utility.h | 28 +++++++++++++++++++ source/common/config/utility.cc | 9 ------ source/common/config/utility.h | 17 ----------- source/common/listener_manager/BUILD | 2 ++ source/common/listener_manager/lds_api.cc | 1 + source/common/router/config_impl.cc | 1 + source/common/router/vhds.cc | 1 + source/common/secret/sds_api.cc | 1 + source/common/tcp_proxy/BUILD | 1 + source/extensions/clusters/eds/eds.cc | 1 + .../common/dynamic_forward_proxy/BUILD | 1 + .../extensions/filters/http/basic_auth/BUILD | 1 + .../filters/network/dubbo_proxy/router/BUILD | 2 ++ .../filters/header_to_metadata/BUILD | 3 ++ .../filters/payload_to_metadata/BUILD | 2 ++ .../network/dns_resolver/getaddrinfo/BUILD | 1 + .../extensions/stat_sinks/common/statsd/BUILD | 2 ++ source/extensions/tracers/datadog/BUILD | 2 ++ .../tls/cert_validator/BUILD | 1 + source/server/BUILD | 2 ++ source/server/config_validation/server.cc | 3 +- source/server/server.cc | 4 ++- test/common/config/BUILD | 1 + test/common/config/utility_test.cc | 5 ++-- 29 files changed, 95 insertions(+), 35 deletions(-) create mode 100644 source/common/config/stats_utility.cc create mode 100644 source/common/config/stats_utility.h diff --git a/contrib/generic_proxy/filters/network/source/BUILD b/contrib/generic_proxy/filters/network/source/BUILD index 4df7bc7b6f28..67129461e1a5 100644 --- a/contrib/generic_proxy/filters/network/source/BUILD +++ b/contrib/generic_proxy/filters/network/source/BUILD @@ -74,6 +74,7 @@ envoy_cc_library( "//source/common/common:matchers_lib", "//source/common/config:metadata_lib", "//source/common/config:utility_lib", + "//source/common/http:header_utility_lib", "//source/common/matcher:matcher_lib", "@envoy_api//contrib/envoy/extensions/filters/network/generic_proxy/action/v3:pkg_cc_proto", "@envoy_api//contrib/envoy/extensions/filters/network/generic_proxy/v3:pkg_cc_proto", diff --git a/source/common/access_log/access_log_impl.cc b/source/common/access_log/access_log_impl.cc index fd72e00b1a74..c13213d9cbbe 100644 --- a/source/common/access_log/access_log_impl.cc +++ b/source/common/access_log/access_log_impl.cc @@ -15,6 +15,7 @@ #include "source/common/common/utility.h" #include "source/common/config/metadata.h" #include "source/common/config/utility.h" +#include "source/common/grpc/common.h" #include "source/common/http/header_map_impl.h" #include "source/common/http/header_utility.h" #include "source/common/http/headers.h" diff --git a/source/common/common/BUILD b/source/common/common/BUILD index aa0edac5aa61..6d630ec2a028 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -319,6 +319,7 @@ envoy_cc_library( "//envoy/common:matchers_interface", "//source/common/common:regex_lib", "//source/common/config:metadata_lib", + "//source/common/config:utility_lib", "//source/common/http:path_utility_lib", "//source/common/protobuf", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", diff --git a/source/common/config/BUILD b/source/common/config/BUILD index dffb0c2b7f3f..2965f617062a 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -226,15 +226,10 @@ envoy_cc_library( "//source/common/common:backoff_lib", "//source/common/common:hash_lib", "//source/common/common:hex_lib", - "//source/common/grpc:common_lib", "//source/common/protobuf", "//source/common/protobuf:utility_lib", "//source/common/runtime:runtime_features_lib", "//source/common/singleton:const_singleton", - "//source/common/stats:histogram_lib", - "//source/common/stats:stats_lib", - "//source/common/stats:stats_matcher_lib", - "//source/common/stats:tag_producer_lib", "//source/common/version:api_version_lib", "@com_github_cncf_xds//udpa/type/v1:pkg_cc_proto", "@com_github_cncf_xds//xds/type/v3:pkg_cc_proto", @@ -245,6 +240,19 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "stats_utility_lib", + srcs = ["stats_utility.cc"], + hdrs = ["stats_utility.h"], + deps = [ + "//source/common/stats:histogram_lib", + "//source/common/stats:stats_lib", + "//source/common/stats:stats_matcher_lib", + "//source/common/stats:tag_producer_lib", + "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", + ], +) + envoy_cc_library( name = "subscription_base_interface", hdrs = ["subscription_base.h"], diff --git a/source/common/config/stats_utility.cc b/source/common/config/stats_utility.cc new file mode 100644 index 000000000000..eed7f4c98b40 --- /dev/null +++ b/source/common/config/stats_utility.cc @@ -0,0 +1,17 @@ +#include "source/common/config/stats_utility.h" + +#include "source/common/stats/histogram_impl.h" +#include "source/common/stats/stats_matcher_impl.h" +#include "source/common/stats/tag_producer_impl.h" + +namespace Envoy { +namespace Config { + +Stats::TagProducerPtr +StatsUtility::createTagProducer(const envoy::config::bootstrap::v3::Bootstrap& bootstrap, + const Stats::TagVector& cli_tags) { + return std::make_unique(bootstrap.stats_config(), cli_tags); +} + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/stats_utility.h b/source/common/config/stats_utility.h new file mode 100644 index 000000000000..409bd1ddc0c8 --- /dev/null +++ b/source/common/config/stats_utility.h @@ -0,0 +1,28 @@ +#pragma once + +#include "envoy/config/bootstrap/v3/bootstrap.pb.h" +#include "envoy/stats/histogram.h" +#include "envoy/stats/scope.h" +#include "envoy/stats/stats_macros.h" +#include "envoy/stats/stats_matcher.h" +#include "envoy/stats/tag_producer.h" + +namespace Envoy { +namespace Config { + +class StatsUtility { +public: + /** + * Create TagProducer instance. Check all tag names for conflicts to avoid + * unexpected tag name overwriting. + * @param bootstrap bootstrap proto. + * @param cli_tags tags that are provided by the cli + * @throws EnvoyException when the conflict of tag names is found. + */ + static Stats::TagProducerPtr + createTagProducer(const envoy::config::bootstrap::v3::Bootstrap& bootstrap, + const Stats::TagVector& cli_tags); +}; + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index 7728019c228e..83556c148054 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -11,9 +11,6 @@ #include "source/common/common/assert.h" #include "source/common/protobuf/utility.h" -#include "source/common/stats/histogram_impl.h" -#include "source/common/stats/stats_matcher_impl.h" -#include "source/common/stats/tag_producer_impl.h" namespace Envoy { namespace Config { @@ -204,12 +201,6 @@ Utility::parseRateLimitSettings(const envoy::config::core::v3::ApiConfigSource& return rate_limit_settings; } -Stats::TagProducerPtr -Utility::createTagProducer(const envoy::config::bootstrap::v3::Bootstrap& bootstrap, - const Stats::TagVector& cli_tags) { - return std::make_unique(bootstrap.stats_config(), cli_tags); -} - absl::StatusOr Utility::factoryForGrpcApiConfigSource( Grpc::AsyncClientManager& async_client_manager, const envoy::config::core::v3::ApiConfigSource& api_config_source, Stats::Scope& scope, diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 27971e06ac85..ede6515d3a07 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -12,11 +12,6 @@ #include "envoy/local_info/local_info.h" #include "envoy/registry/registry.h" #include "envoy/server/filter_config.h" -#include "envoy/stats/histogram.h" -#include "envoy/stats/scope.h" -#include "envoy/stats/stats_macros.h" -#include "envoy/stats/stats_matcher.h" -#include "envoy/stats/tag_producer.h" #include "envoy/upstream/cluster_manager.h" #include "source/common/common/assert.h" @@ -24,7 +19,6 @@ #include "source/common/common/hash.h" #include "source/common/common/hex.h" #include "source/common/common/utility.h" -#include "source/common/grpc/common.h" #include "source/common/protobuf/protobuf.h" #include "source/common/protobuf/utility.h" #include "source/common/runtime/runtime_features.h" @@ -392,17 +386,6 @@ class Utility { */ static std::string truncateGrpcStatusMessage(absl::string_view error_message); - /** - * Create TagProducer instance. Check all tag names for conflicts to avoid - * unexpected tag name overwriting. - * @param bootstrap bootstrap proto. - * @param cli_tags tags that are provided by the cli - * @throws EnvoyException when the conflict of tag names is found. - */ - static Stats::TagProducerPtr - createTagProducer(const envoy::config::bootstrap::v3::Bootstrap& bootstrap, - const Stats::TagVector& cli_tags); - /** * Obtain gRPC async client factory from a envoy::config::core::v3::ApiConfigSource. * @param async_client_manager gRPC async client manager. diff --git a/source/common/listener_manager/BUILD b/source/common/listener_manager/BUILD index 1b8e49427033..e567cb0ac856 100644 --- a/source/common/listener_manager/BUILD +++ b/source/common/listener_manager/BUILD @@ -137,8 +137,10 @@ envoy_cc_library( "//source/common/config:api_version_lib", "//source/common/config:subscription_base_interface", "//source/common/config:utility_lib", + "//source/common/grpc:common_lib", "//source/common/init:target_lib", "//source/common/protobuf:utility_lib", + "@com_google_absl//absl/container:node_hash_set", "@envoy_api//envoy/admin/v3:pkg_cc_proto", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/config/listener/v3:pkg_cc_proto", diff --git a/source/common/listener_manager/lds_api.cc b/source/common/listener_manager/lds_api.cc index ee669d306e5d..052872846da6 100644 --- a/source/common/listener_manager/lds_api.cc +++ b/source/common/listener_manager/lds_api.cc @@ -12,6 +12,7 @@ #include "source/common/common/cleanup.h" #include "source/common/config/api_version.h" #include "source/common/config/utility.h" +#include "source/common/grpc/common.h" #include "source/common/protobuf/utility.h" #include "absl/container/node_hash_set.h" diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index ecf2c3639d90..19f1d7367f3f 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -32,6 +32,7 @@ #include "source/common/config/metadata.h" #include "source/common/config/utility.h" #include "source/common/config/well_known_names.h" +#include "source/common/grpc/common.h" #include "source/common/http/header_utility.h" #include "source/common/http/headers.h" #include "source/common/http/matching/data_impl.h" diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc index a805597b0870..823ee0b0f86d 100644 --- a/source/common/router/vhds.cc +++ b/source/common/router/vhds.cc @@ -13,6 +13,7 @@ #include "source/common/common/fmt.h" #include "source/common/config/api_version.h" #include "source/common/config/utility.h" +#include "source/common/grpc/common.h" #include "source/common/protobuf/utility.h" #include "source/common/router/config_impl.h" diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 5492fb92de28..344da3ff5833 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -6,6 +6,7 @@ #include "source/common/common/assert.h" #include "source/common/config/api_version.h" +#include "source/common/grpc/common.h" #include "source/common/protobuf/utility.h" namespace Envoy { diff --git a/source/common/tcp_proxy/BUILD b/source/common/tcp_proxy/BUILD index 4d1309a5a4b7..3e1f10dba933 100644 --- a/source/common/tcp_proxy/BUILD +++ b/source/common/tcp_proxy/BUILD @@ -62,6 +62,7 @@ envoy_cc_library( "//source/common/common:enum_to_int", "//source/common/common:macros", "//source/common/common:minimal_logger_lib", + "//source/common/config:well_known_names", "//source/common/formatter:substitution_format_string_lib", "//source/common/http:codec_client_lib", "//source/common/network:application_protocol_lib", diff --git a/source/extensions/clusters/eds/eds.cc b/source/extensions/clusters/eds/eds.cc index 76d0b258514f..1b60873be93f 100644 --- a/source/extensions/clusters/eds/eds.cc +++ b/source/extensions/clusters/eds/eds.cc @@ -9,6 +9,7 @@ #include "source/common/common/utility.h" #include "source/common/config/api_version.h" #include "source/common/config/decoded_resource_impl.h" +#include "source/common/grpc/common.h" namespace Envoy { namespace Upstream { diff --git a/source/extensions/common/dynamic_forward_proxy/BUILD b/source/extensions/common/dynamic_forward_proxy/BUILD index bb3535dab9ec..f8b68b72daad 100644 --- a/source/extensions/common/dynamic_forward_proxy/BUILD +++ b/source/extensions/common/dynamic_forward_proxy/BUILD @@ -18,6 +18,7 @@ envoy_cc_library( "//envoy/singleton:manager_interface", "//envoy/thread_local:thread_local_interface", "//envoy/upstream:resource_manager_interface", + "//source/common/http:header_utility_lib", "@envoy_api//envoy/extensions/common/dynamic_forward_proxy/v3:pkg_cc_proto", ], ) diff --git a/source/extensions/filters/http/basic_auth/BUILD b/source/extensions/filters/http/basic_auth/BUILD index f610d4fee905..7f47e0226048 100644 --- a/source/extensions/filters/http/basic_auth/BUILD +++ b/source/extensions/filters/http/basic_auth/BUILD @@ -19,6 +19,7 @@ envoy_cc_library( "//source/common/common:base64_lib", "//source/common/config:utility_lib", "//source/common/http:header_map_lib", + "//source/common/http:header_utility_lib", "//source/common/protobuf:utility_lib", "//source/extensions/filters/http/common:pass_through_filter_lib", ], diff --git a/source/extensions/filters/network/dubbo_proxy/router/BUILD b/source/extensions/filters/network/dubbo_proxy/router/BUILD index f75fc1d75d62..0901f4a09516 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/BUILD +++ b/source/extensions/filters/network/dubbo_proxy/router/BUILD @@ -26,6 +26,7 @@ envoy_cc_library( "//envoy/router:router_interface", "//source/common/common:logger_lib", "//source/common/common:matchers_lib", + "//source/common/config:well_known_names", "//source/common/http:header_utility_lib", "//source/common/protobuf:utility_lib", "//source/common/router:metadatamatchcriteria_lib", @@ -63,6 +64,7 @@ envoy_cc_library( "//envoy/upstream:load_balancer_interface", "//envoy/upstream:thread_local_cluster_interface", "//source/common/common:logger_lib", + "//source/common/config:well_known_names", "//source/common/http:header_utility_lib", "//source/common/router:metadatamatchcriteria_lib", "//source/common/upstream:load_balancer_lib", diff --git a/source/extensions/filters/network/thrift_proxy/filters/header_to_metadata/BUILD b/source/extensions/filters/network/thrift_proxy/filters/header_to_metadata/BUILD index 810944a4ac04..76be28f0b1fd 100644 --- a/source/extensions/filters/network/thrift_proxy/filters/header_to_metadata/BUILD +++ b/source/extensions/filters/network/thrift_proxy/filters/header_to_metadata/BUILD @@ -27,6 +27,9 @@ envoy_cc_library( hdrs = ["header_to_metadata_filter.h"], deps = [ "//envoy/server:filter_config_interface", + "//source/common/common:base64_lib", + "//source/common/common:matchers_lib", + "//source/common/network:utility_lib", "//source/extensions/filters/network/thrift_proxy/filters:pass_through_filter_lib", "@envoy_api//envoy/extensions/filters/network/thrift_proxy/filters/header_to_metadata/v3:pkg_cc_proto", ], diff --git a/source/extensions/filters/network/thrift_proxy/filters/payload_to_metadata/BUILD b/source/extensions/filters/network/thrift_proxy/filters/payload_to_metadata/BUILD index cc6eeb8630a0..8597af0e12e7 100644 --- a/source/extensions/filters/network/thrift_proxy/filters/payload_to_metadata/BUILD +++ b/source/extensions/filters/network/thrift_proxy/filters/payload_to_metadata/BUILD @@ -27,6 +27,8 @@ envoy_cc_library( hdrs = ["payload_to_metadata_filter.h"], deps = [ "//envoy/server:filter_config_interface", + "//source/common/common:matchers_lib", + "//source/common/network:utility_lib", "//source/extensions/filters/network/thrift_proxy:auto_protocol_lib", "//source/extensions/filters/network/thrift_proxy:auto_transport_lib", "//source/extensions/filters/network/thrift_proxy:decoder_lib", diff --git a/source/extensions/network/dns_resolver/getaddrinfo/BUILD b/source/extensions/network/dns_resolver/getaddrinfo/BUILD index e2b26df7941c..05b119daa59c 100644 --- a/source/extensions/network/dns_resolver/getaddrinfo/BUILD +++ b/source/extensions/network/dns_resolver/getaddrinfo/BUILD @@ -15,6 +15,7 @@ envoy_cc_extension( deps = [ "//envoy/network:dns_resolver_interface", "//envoy/registry", + "//source/common/api:os_sys_calls_lib", "@envoy_api//envoy/extensions/network/dns_resolver/getaddrinfo/v3:pkg_cc_proto", ], ) diff --git a/source/extensions/stat_sinks/common/statsd/BUILD b/source/extensions/stat_sinks/common/statsd/BUILD index 2d74e42869e9..54fe8b9ac588 100644 --- a/source/extensions/stat_sinks/common/statsd/BUILD +++ b/source/extensions/stat_sinks/common/statsd/BUILD @@ -30,5 +30,7 @@ envoy_cc_library( "//source/common/common:utility_lib", "//source/common/config:utility_lib", "//source/common/network:address_lib", + "//source/common/network:default_socket_interface_lib", + "//source/common/network:utility_lib", ], ) diff --git a/source/extensions/tracers/datadog/BUILD b/source/extensions/tracers/datadog/BUILD index da0862389706..cd7327a67d09 100644 --- a/source/extensions/tracers/datadog/BUILD +++ b/source/extensions/tracers/datadog/BUILD @@ -43,6 +43,8 @@ envoy_cc_library( deps = [ "//source/common/config:utility_lib", "//source/common/http:async_client_utility_lib", + "//source/common/http:message_lib", + "//source/common/http:utility_lib", "//source/common/tracing:common_values_lib", "//source/common/tracing:null_span_lib", "//source/common/tracing:trace_context_lib", diff --git a/source/extensions/transport_sockets/tls/cert_validator/BUILD b/source/extensions/transport_sockets/tls/cert_validator/BUILD index c79416930672..5abc18bc2595 100644 --- a/source/extensions/transport_sockets/tls/cert_validator/BUILD +++ b/source/extensions/transport_sockets/tls/cert_validator/BUILD @@ -36,6 +36,7 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/common:base64_lib", "//source/common/common:hex_lib", + "//source/common/common:matchers_lib", "//source/common/common:minimal_logger_lib", "//source/common/common:utility_lib", "//source/common/config:utility_lib", diff --git a/source/server/BUILD b/source/server/BUILD index f0d63ba47174..7ee402750e1f 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -313,6 +313,7 @@ envoy_cc_library( "//source/common/event:scaled_range_timer_manager_lib", "//source/common/stats:symbol_table_lib", "//source/server:resource_monitor_config_lib", + "@com_google_absl//absl/container:node_hash_set", "@envoy_api//envoy/config/overload/v3:pkg_cc_proto", ], ) @@ -440,6 +441,7 @@ envoy_cc_library( "//source/common/common:mutex_tracer_lib", "//source/common/common:perf_tracing_lib", "//source/common/common:utility_lib", + "//source/common/config:stats_utility_lib", "//source/common/config:utility_lib", "//source/common/config:xds_resource_lib", "//source/common/grpc:async_client_manager_lib", diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index a8685dbd28eb..054cb3f11b24 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -5,6 +5,7 @@ #include "envoy/config/bootstrap/v3/bootstrap.pb.h" #include "source/common/common/utility.h" +#include "source/common/config/stats_utility.h" #include "source/common/config/utility.h" #include "source/common/config/well_known_names.h" #include "source/common/event/real_time_system.h" @@ -100,7 +101,7 @@ void ValidationInstance::initialize(const Options& options, Regex::EnginePtr regex_engine = createRegexEngine( bootstrap_, messageValidationContext().staticValidationVisitor(), serverFactoryContext()); - Config::Utility::createTagProducer(bootstrap_, options_.statsTags()); + Config::StatsUtility::createTagProducer(bootstrap_, options_.statsTags()); if (!bootstrap_.node().user_agent_build_version().has_version()) { *bootstrap_.mutable_node()->mutable_user_agent_build_version() = VersionInfo::buildVersion(); } diff --git a/source/server/server.cc b/source/server/server.cc index 205823a43f6c..376ebb48e829 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -29,6 +29,7 @@ #include "source/common/common/enum_to_int.h" #include "source/common/common/mutex_tracer_impl.h" #include "source/common/common/utility.h" +#include "source/common/config/stats_utility.h" #include "source/common/config/utility.h" #include "source/common/config/well_known_names.h" #include "source/common/config/xds_resource.h" @@ -500,7 +501,8 @@ void InstanceBase::initializeOrThrow(Network::Address::InstanceConstSharedPtr lo // Needs to happen as early as possible in the instantiation to preempt the objects that require // stats. - stats_store_.setTagProducer(Config::Utility::createTagProducer(bootstrap_, options_.statsTags())); + stats_store_.setTagProducer( + Config::StatsUtility::createTagProducer(bootstrap_, options_.statsTags())); stats_store_.setStatsMatcher(std::make_unique( bootstrap_.stats_config(), stats_store_.symbolTable())); stats_store_.setHistogramSettings( diff --git a/test/common/config/BUILD b/test/common/config/BUILD index 08fb0d1208eb..2aef53b1499b 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -126,6 +126,7 @@ envoy_cc_test( external_deps = ["abseil_optional"], deps = [ "//source/common/config:api_version_lib", + "//source/common/config:stats_utility_lib", "//source/common/config:utility_lib", "//source/common/config:well_known_names", "//source/common/stats:stats_lib", diff --git a/test/common/config/utility_test.cc b/test/common/config/utility_test.cc index 767c0e7db1a9..2d313ca1f981 100644 --- a/test/common/config/utility_test.cc +++ b/test/common/config/utility_test.cc @@ -8,6 +8,7 @@ #include "source/common/common/fmt.h" #include "source/common/config/api_version.h" +#include "source/common/config/stats_utility.h" #include "source/common/config/utility.h" #include "source/common/config/well_known_names.h" #include "source/common/protobuf/protobuf.h" @@ -57,7 +58,7 @@ TEST(UtilityTest, ConfigSourceInitFetchTimeout) { TEST(UtilityTest, createTagProducer) { envoy::config::bootstrap::v3::Bootstrap bootstrap; - auto producer = Utility::createTagProducer(bootstrap, {}); + auto producer = StatsUtility::createTagProducer(bootstrap, {}); ASSERT_TRUE(producer != nullptr); Stats::TagVector tags; auto extracted_name = producer->produceTags("http.config_test.rq_total", tags); @@ -67,7 +68,7 @@ TEST(UtilityTest, createTagProducer) { TEST(UtilityTest, createTagProducerWithDefaultTgs) { envoy::config::bootstrap::v3::Bootstrap bootstrap; - auto producer = Utility::createTagProducer(bootstrap, {{"foo", "bar"}}); + auto producer = StatsUtility::createTagProducer(bootstrap, {{"foo", "bar"}}); ASSERT_TRUE(producer != nullptr); Stats::TagVector tags; auto extracted_name = producer->produceTags("http.config_test.rq_total", tags); From dee4ff7519fb27da93387941a3ff0cffe0abb993 Mon Sep 17 00:00:00 2001 From: botengyao Date: Wed, 28 Feb 2024 23:42:52 -0500 Subject: [PATCH 05/12] hcm: codec creation load shed point (#32528) --------- Signed-off-by: Boteng Yao --- changelogs/current.yaml | 4 ++ .../overload_manager/overload_manager.rst | 5 ++ envoy/server/overload/load_shed_point.h | 2 + source/common/http/conn_manager_impl.cc | 11 ++++ source/common/http/conn_manager_impl.h | 1 + test/common/http/conn_manager_impl_test_2.cc | 56 ++++++++++++++++++- test/integration/overload_integration_test.cc | 35 ++++++++++++ 7 files changed, 113 insertions(+), 1 deletion(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 3ff9eeab0609..2815679340e8 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -234,6 +234,10 @@ new_features: added an option to dynamically set a per downstream connection idle timeout period object under the key ``envoy.tcp_proxy.per_connection_idle_timeout_ms``. If this filter state value exists, it will override the idle timeout specified in the filter configuration and the default idle timeout. +- area: load shed point + change: | + Added load shed point ``envoy.load_shed_points.hcm_ondata_creating_codec`` that closes connections before creating codec if + Envoy is under pressure, typically memory. - area: overload change: | added a :ref:`configuration option diff --git a/docs/root/configuration/operations/overload_manager/overload_manager.rst b/docs/root/configuration/operations/overload_manager/overload_manager.rst index 7d45a1bbd758..dcc522e31d12 100644 --- a/docs/root/configuration/operations/overload_manager/overload_manager.rst +++ b/docs/root/configuration/operations/overload_manager/overload_manager.rst @@ -165,6 +165,11 @@ The following core load shed points are supported: - Envoy will send a ``GOAWAY`` while processing HTTP2 requests at the codec level which will eventually drain the HTTP/2 connection. + * - envoy.load_shed_points.hcm_ondata_creating_codec + - Envoy will close the connections before creating codec if Envoy is under + pressure, typically memory. This happens once geting data from the + connection. + .. _config_overload_manager_reducing_timeouts: Reducing timeouts diff --git a/envoy/server/overload/load_shed_point.h b/envoy/server/overload/load_shed_point.h index 50018a2906fc..076eff36b708 100644 --- a/envoy/server/overload/load_shed_point.h +++ b/envoy/server/overload/load_shed_point.h @@ -26,6 +26,8 @@ class LoadShedPointNameValues { // which will eventually drain the HTTP/2 connection. const std::string H2ServerGoAwayOnDispatch = "envoy.load_shed_points.http2_server_go_away_on_dispatch"; + + const std::string HcmCodecCreation = "envoy.load_shed_points.hcm_ondata_creating_codec"; }; using LoadShedPointName = ConstSingleton; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index e486973bec90..4c1420575eb5 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -116,6 +116,8 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config, overload_state_(overload_manager.getThreadLocalOverloadState()), accept_new_http_stream_( overload_manager.getLoadShedPoint(Server::LoadShedPointName::get().HcmDecodeHeaders)), + hcm_ondata_creating_codec_( + overload_manager.getLoadShedPoint(Server::LoadShedPointName::get().HcmCodecCreation)), overload_stop_accepting_requests_ref_( overload_state_.getState(Server::OverloadActionNames::get().StopAcceptingRequests)), overload_disable_keepalive_ref_( @@ -132,6 +134,9 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config, trace, accept_new_http_stream_ == nullptr, "LoadShedPoint envoy.load_shed_points.http_connection_manager_decode_headers is not " "found. Is it configured?"); + ENVOY_LOG_ONCE_IF(trace, hcm_ondata_creating_codec_ == nullptr, + "LoadShedPoint envoy.load_shed_points.hcm_ondata_creating_codec is not found. " + "Is it configured?"); } const ResponseHeaderMap& ConnectionManagerImpl::continueHeader() { @@ -479,6 +484,12 @@ void ConnectionManagerImpl::createCodec(Buffer::Instance& data) { Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool) { requests_during_dispatch_count_ = 0; if (!codec_) { + // Close connections if Envoy is under pressure, typically memory, before creating codec. + if (hcm_ondata_creating_codec_ != nullptr && hcm_ondata_creating_codec_->shouldShedLoad()) { + stats_.named_.downstream_rq_overload_close_.inc(); + handleCodecOverloadError("onData codec creation overload"); + return Network::FilterStatus::StopIteration; + } // Http3 codec should have been instantiated by now. createCodec(data); } diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index aac42af4b34a..11151b157bf5 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -642,6 +642,7 @@ class ConnectionManagerImpl : Logger::Loggable, Server::OverloadManager& overload_manager_; Server::ThreadLocalOverloadState& overload_state_; Server::LoadShedPoint* accept_new_http_stream_{nullptr}; + Server::LoadShedPoint* hcm_ondata_creating_codec_{nullptr}; // References into the overload manager thread local state map. Using these lets us avoid a // map lookup in the hot path of processing each request. const Server::OverloadActionState& overload_stop_accepting_requests_ref_; diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index 287507afa2a1..83b3971523f7 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -2489,10 +2489,64 @@ TEST_F(HttpConnectionManagerImplTest, DisableHttp2KeepAliveWhenOverloaded) { EXPECT_EQ(1, stats_.named_.downstream_cx_overload_disable_keepalive_.value()); } +TEST_F(HttpConnectionManagerImplTest, CodecCreationLoadShedPointCanCloseConnection) { + Server::MockLoadShedPoint close_connection_creating_codec_point; + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().HcmCodecCreation)) + .WillOnce(Return(&close_connection_creating_codec_point)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().HcmDecodeHeaders)) + .WillOnce(Return(nullptr)); + + setup(false, ""); + + EXPECT_CALL(close_connection_creating_codec_point, shouldShedLoad()).WillOnce(Return(true)); + EXPECT_CALL(filter_callbacks_.connection_, close(_, _)); + + Buffer::OwnedImpl fake_input("hello"); + conn_manager_->onData(fake_input, false); + + delete codec_; + EXPECT_EQ(1U, stats_.named_.downstream_rq_overload_close_.value()); + EXPECT_TRUE(filter_callbacks_.connection().streamInfo().hasResponseFlag( + StreamInfo::CoreResponseFlag::OverloadManager)); +} + +TEST_F(HttpConnectionManagerImplTest, CodecCreationLoadShedPointBypasscheck) { + Server::MockLoadShedPoint close_connection_creating_codec_point; + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().HcmCodecCreation)) + .WillOnce(Return(&close_connection_creating_codec_point)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().HcmDecodeHeaders)) + .WillOnce(Return(nullptr)); + + setup(false, ""); + + EXPECT_CALL(close_connection_creating_codec_point, shouldShedLoad()).WillOnce(Return(false)); + + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { + conn_manager_->newStream(response_encoder_); + data.drain(2); + return Http::okStatus(); + })); + + Buffer::OwnedImpl fake_input("12"); + conn_manager_->onData(fake_input, false); + conn_manager_->onEvent(Network::ConnectionEvent::RemoteClose); + EXPECT_EQ(0U, stats_.named_.downstream_rq_overload_close_.value()); + EXPECT_FALSE(filter_callbacks_.connection().streamInfo().hasResponseFlag( + StreamInfo::CoreResponseFlag::OverloadManager)); +} + TEST_F(HttpConnectionManagerImplTest, DecodeHeaderLoadShedPointCanRejectNewStreams) { Server::MockLoadShedPoint accept_new_stream_point; - EXPECT_CALL(overload_manager_, getLoadShedPoint(testing::_)) + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().HcmDecodeHeaders)) .WillOnce(Return(&accept_new_stream_point)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().HcmCodecCreation)) + .WillOnce(Return(nullptr)); setup(false, ""); setupFilterChain(1, 0); diff --git a/test/integration/overload_integration_test.cc b/test/integration/overload_integration_test.cc index efaf470afce0..56d6df0ffdb9 100644 --- a/test/integration/overload_integration_test.cc +++ b/test/integration/overload_integration_test.cc @@ -814,4 +814,39 @@ TEST_P(LoadShedPointIntegrationTest, Http2ServerDispatchSendsGoAwayCompletingPen "overload.envoy.load_shed_points.http2_server_go_away_on_dispatch.scale_percent", 0); } +TEST_P(LoadShedPointIntegrationTest, HttpConnectionMnagerCloseConnectionCreatingCodec) { + if (downstreamProtocol() == Http::CodecClient::Type::HTTP3) { + return; + } + autonomous_upstream_ = true; + initializeOverloadManager( + TestUtility::parseYaml(R"EOF( + name: "envoy.load_shed_points.hcm_ondata_creating_codec" + triggers: + - name: "envoy.resource_monitors.testonly.fake_resource_monitor" + threshold: + value: 0.90 + )EOF")); + + codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + + updateResource(0.95); + test_server_->waitForGaugeEq( + "overload.envoy.load_shed_points.hcm_ondata_creating_codec.scale_percent", 100); + auto encoder_decoder = codec_client_->startRequest(default_request_headers_); + + test_server_->waitForCounterEq("http.config_test.downstream_rq_overload_close", 1); + ASSERT_TRUE(codec_client_->waitForDisconnect()); + + updateResource(0.80); + test_server_->waitForGaugeEq( + "overload.envoy.load_shed_points.hcm_ondata_creating_codec.scale_percent", 0); + + codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + test_server_->waitForCounterEq("http.config_test.downstream_rq_overload_close", 1); + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_EQ(response->headers().getStatusValue(), "200"); +} + } // namespace Envoy From b79d9f17bb6f9e42c6ce3fabd1570a43dceaaa69 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 29 Feb 2024 00:10:20 -0800 Subject: [PATCH 06/12] tools: fix python regex warnings for newer python (#32630) example error: ./tools/code_format/check_format.py:260: SyntaxWarning: invalid escape sequence '\s' return re.compile("^\s*namespace\s+%s\s*{" % self.namespace_check, re.MULTILINE) Signed-off-by: Greg Greenway --- tools/code_format/check_format.py | 2 +- tools/code_format/envoy_build_fixer.py | 12 ++++++------ tools/code_format/header_order.py | 6 +++--- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index 368478ac2c34..bae3ce814b77 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -257,7 +257,7 @@ def namespace_check_excluded_paths(self): @cached_property def namespace_re(self): - return re.compile("^\s*namespace\s+%s\s*{" % self.namespace_check, re.MULTILINE) + return re.compile(r"^\s*namespace\s+%s\s*{" % self.namespace_check, re.MULTILINE) # Map a line transformation function across each line of a file, # writing the result lines as requested. diff --git a/tools/code_format/envoy_build_fixer.py b/tools/code_format/envoy_build_fixer.py index 470d25f20b6a..87fcc8909559 100755 --- a/tools/code_format/envoy_build_fixer.py +++ b/tools/code_format/envoy_build_fixer.py @@ -33,18 +33,18 @@ ENVOY_RULE_REGEX = re.compile(r'envoy[_\w]+\(') # Match a load() statement for the envoy_package macros. -PACKAGE_LOAD_BLOCK_REGEX = re.compile('("envoy_package".*?\)\n)', re.DOTALL) -EXTENSION_PACKAGE_LOAD_BLOCK_REGEX = re.compile('("envoy_extension_package".*?\)\n)', re.DOTALL) -CONTRIB_PACKAGE_LOAD_BLOCK_REGEX = re.compile('("envoy_contrib_package".*?\)\n)', re.DOTALL) -MOBILE_PACKAGE_LOAD_BLOCK_REGEX = re.compile('("envoy_mobile_package".*?\)\n)', re.DOTALL) +PACKAGE_LOAD_BLOCK_REGEX = re.compile(r'("envoy_package".*?\)\n)', re.DOTALL) +EXTENSION_PACKAGE_LOAD_BLOCK_REGEX = re.compile(r'("envoy_extension_package".*?\)\n)', re.DOTALL) +CONTRIB_PACKAGE_LOAD_BLOCK_REGEX = re.compile(r'("envoy_contrib_package".*?\)\n)', re.DOTALL) +MOBILE_PACKAGE_LOAD_BLOCK_REGEX = re.compile(r'("envoy_mobile_package".*?\)\n)', re.DOTALL) # Match Buildozer 'print' output. Example of Buildozer print output: # cc_library json_transcoder_filter_lib [json_transcoder_filter.cc] (missing) (missing) BUILDOZER_PRINT_REGEX = re.compile( - '\s*([\w_]+)\s+([\w_]+)\s+[(\[](.*?)[)\]]\s+[(\[](.*?)[)\]]\s+[(\[](.*?)[)\]]') + r'\s*([\w_]+)\s+([\w_]+)\s+[(\[](.*?)[)\]]\s+[(\[](.*?)[)\]]\s+[(\[](.*?)[)\]]') # Match API header include in Envoy source file? -API_INCLUDE_REGEX = re.compile('#include "(contrib/envoy/.*|envoy/.*)/[^/]+\.pb\.(validate\.)?h"') +API_INCLUDE_REGEX = re.compile(r'#include "(contrib/envoy/.*|envoy/.*)/[^/]+\.pb\.(validate\.)?h"') class EnvoyBuildFixerError(Exception): diff --git a/tools/code_format/header_order.py b/tools/code_format/header_order.py index bd8ec81577f2..df1c8cb77f81 100755 --- a/tools/code_format/header_order.py +++ b/tools/code_format/header_order.py @@ -65,11 +65,11 @@ def regex_filter(regex): # Filters that define the #include blocks block_filters = [ file_header_filter(), - regex_filter('<.*\.h>'), - regex_filter('<.*>'), + regex_filter(r'<.*\.h>'), + regex_filter(r'<.*>'), ] for subdir in include_dir_order: - block_filters.append(regex_filter('"' + subdir + '/.*"')) + block_filters.append(regex_filter(r'"' + subdir + r'/.*"')) blocks = [] already_included = set([]) From 09e182ec15e597780f861f2b1b33af1951cdcb59 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 29 Feb 2024 10:26:18 +0000 Subject: [PATCH 07/12] build(deps): bump distroless/base-nossl-debian12 from `0e777c6` to `28dc895` in /ci (#32613) build(deps): bump distroless/base-nossl-debian12 in /ci Bumps distroless/base-nossl-debian12 from `0e777c6` to `28dc895`. --- updated-dependencies: - dependency-name: distroless/base-nossl-debian12 dependency-type: direct:production ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- ci/Dockerfile-envoy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/Dockerfile-envoy b/ci/Dockerfile-envoy index 07affa3502e3..9a6f0abe70fd 100644 --- a/ci/Dockerfile-envoy +++ b/ci/Dockerfile-envoy @@ -58,7 +58,7 @@ COPY --chown=0:0 --chmod=755 \ # STAGE: envoy-distroless -FROM gcr.io/distroless/base-nossl-debian12:nonroot@sha256:0e777c69ba810353b9f3f2033280bbe7d029d81fa55760f6eec817ef595aa19c AS envoy-distroless +FROM gcr.io/distroless/base-nossl-debian12:nonroot@sha256:28dc8956c04a92ffc192d06c5da69fa747c675ee44043ba18128e747c2f539f5 AS envoy-distroless EXPOSE 10000 ENTRYPOINT ["/usr/local/bin/envoy"] CMD ["-c", "/etc/envoy/envoy.yaml"] From 97ef386aeb4d12a3fd38f151c38be7decc5c37bf Mon Sep 17 00:00:00 2001 From: Joao Grassi <5938087+joaopgrassi@users.noreply.github.com> Date: Thu, 29 Feb 2024 17:55:04 +0100 Subject: [PATCH 08/12] opentelemetrytracer: Added support to configure a Dynatrace sampler (#32598) Signed-off-by: Thomas Ebner <96168670+samohte@users.noreply.github.com> Signed-off-by: Joao Grassi <5938087+joaopgrassi@users.noreply.github.com> Signed-off-by: thomas.ebner --- .../tracers/opentelemetry/samplers/v3/BUILD | 5 +- .../samplers/v3/dynatrace_sampler.proto | 53 +++ bazel/repository_locations.bzl | 1 + changelogs/current.yaml | 3 + source/extensions/extensions_build_config.bzl | 1 + source/extensions/extensions_metadata.yaml | 7 + .../opentelemetry/samplers/dynatrace/BUILD | 46 +++ .../samplers/dynatrace/config.cc | 38 ++ .../opentelemetry/samplers/dynatrace/config.h | 42 +++ .../samplers/dynatrace/dynatrace_sampler.cc | 188 ++++++++++ .../samplers/dynatrace/dynatrace_sampler.h | 54 +++ .../samplers/dynatrace/sampler_config.cc | 28 ++ .../samplers/dynatrace/sampler_config.h | 47 +++ .../dynatrace/sampler_config_provider.cc | 21 ++ .../dynatrace/sampler_config_provider.h | 63 ++++ .../samplers/dynatrace/sampling_controller.cc | 166 +++++++++ .../samplers/dynatrace/sampling_controller.h | 134 +++++++ .../samplers/dynatrace/stream_summary.h | 204 +++++++++++ .../samplers/dynatrace/tenant_id.h | 50 +++ .../tracers/opentelemetry/tracer.cc | 2 +- .../opentelemetry/samplers/dynatrace/BUILD | 62 ++++ .../samplers/dynatrace/config_test.cc | 38 ++ .../dynatrace_sampler_integration_test.cc | 151 ++++++++ .../dynatrace/dynatrace_sampler_test.cc | 336 ++++++++++++++++++ .../dynatrace/sampler_config_provider_test.cc | 95 +++++ .../samplers/dynatrace/sampler_config_test.cc | 64 ++++ .../dynatrace/sampling_controller_test.cc | 321 +++++++++++++++++ .../samplers/dynatrace/stream_summary_test.cc | 148 ++++++++ .../samplers/dynatrace/tenant_id_test.cc | 31 ++ .../opentelemetry/samplers/sampler_test.cc | 6 + tools/spelling/spelling_dictionary.txt | 1 + 31 files changed, 2404 insertions(+), 2 deletions(-) create mode 100644 api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto create mode 100644 source/extensions/tracers/opentelemetry/samplers/dynatrace/BUILD create mode 100644 source/extensions/tracers/opentelemetry/samplers/dynatrace/config.cc create mode 100644 source/extensions/tracers/opentelemetry/samplers/dynatrace/config.h create mode 100644 source/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler.cc create mode 100644 source/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler.h create mode 100644 source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.cc create mode 100644 source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.h create mode 100644 source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc create mode 100644 source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h create mode 100644 source/extensions/tracers/opentelemetry/samplers/dynatrace/sampling_controller.cc create mode 100644 source/extensions/tracers/opentelemetry/samplers/dynatrace/sampling_controller.h create mode 100644 source/extensions/tracers/opentelemetry/samplers/dynatrace/stream_summary.h create mode 100644 source/extensions/tracers/opentelemetry/samplers/dynatrace/tenant_id.h create mode 100644 test/extensions/tracers/opentelemetry/samplers/dynatrace/BUILD create mode 100644 test/extensions/tracers/opentelemetry/samplers/dynatrace/config_test.cc create mode 100644 test/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler_integration_test.cc create mode 100644 test/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler_test.cc create mode 100644 test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider_test.cc create mode 100644 test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_test.cc create mode 100644 test/extensions/tracers/opentelemetry/samplers/dynatrace/sampling_controller_test.cc create mode 100644 test/extensions/tracers/opentelemetry/samplers/dynatrace/stream_summary_test.cc create mode 100644 test/extensions/tracers/opentelemetry/samplers/dynatrace/tenant_id_test.cc diff --git a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/BUILD b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/BUILD index 29ebf0741406..09a37ad16b83 100644 --- a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/BUILD +++ b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/BUILD @@ -5,5 +5,8 @@ load("@envoy_api//bazel:api_build_system.bzl", "api_proto_package") licenses(["notice"]) # Apache 2 api_proto_package( - deps = ["@com_github_cncf_xds//udpa/annotations:pkg"], + deps = [ + "//envoy/config/core/v3:pkg", + "@com_github_cncf_xds//udpa/annotations:pkg", + ], ) diff --git a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto new file mode 100644 index 000000000000..b74e96a6a416 --- /dev/null +++ b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto @@ -0,0 +1,53 @@ +syntax = "proto3"; + +package envoy.extensions.tracers.opentelemetry.samplers.v3; + +import "envoy/config/core/v3/http_uri.proto"; + +import "udpa/annotations/status.proto"; + +option java_package = "io.envoyproxy.envoy.extensions.tracers.opentelemetry.samplers.v3"; +option java_outer_classname = "DynatraceSamplerProto"; +option java_multiple_files = true; +option go_package = "github.com/envoyproxy/go-control-plane/envoy/extensions/tracers/opentelemetry/samplers/v3;samplersv3"; +option (udpa.annotations.file_status).package_version_status = ACTIVE; + +// [#protodoc-title: Dynatrace Sampler config] +// Configuration for the Dynatrace Sampler extension. +// [#extension: envoy.tracers.opentelemetry.samplers.dynatrace] + +// [#next-free-field: 6] +message DynatraceSamplerConfig { + // The Dynatrace tenant. + // + // The value can be obtained from the Envoy deployment page in Dynatrace. + string tenant = 1; + + // The id of the Dynatrace cluster id. + // + // The value can be obtained from the Envoy deployment page in Dynatrace. + int32 cluster_id = 2; + + // The HTTP URI to fetch the sampler configuration (root spans per minute). For example: + // + // .. code-block:: yaml + // + // http_uri: + // uri: .dev.dynatracelabs.com/api/v2/otlp/v1/traces + // cluster: dynatrace + // timeout: 10s + // + config.core.v3.HttpUri http_uri = 3; + + // The access token to fetch the sampling configuration from the Dynatrace API + string token = 4; + + // Default number of root spans per minute, used when the value can't be obtained from the Dynatrace API. + // + // A default value of ``1000`` is used when: + // + // - ``root_spans_per_minute`` is unset + // - ``root_spans_per_minute`` is set to 0 + // + uint32 root_spans_per_minute = 5; +} diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 9aa875702a92..7fffdb9bd777 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -565,6 +565,7 @@ REPOSITORY_LOCATIONS_SPEC = dict( extensions = [ "envoy.tracers.opentelemetry", "envoy.tracers.opentelemetry.samplers.always_on", + "envoy.tracers.opentelemetry.samplers.dynatrace", ], release_date = "2024-02-17", cpe = "N/A", diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 2815679340e8..0fc50260fb23 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -243,6 +243,9 @@ new_features: added a :ref:`configuration option ` to add ``x-envoy-local-overloaded`` header when Overload Manager is triggered. +- area: tracing + change: | + Added support to configure a Dynatrace sampler for the OpenTelemetry tracer. deprecated: - area: listener diff --git a/source/extensions/extensions_build_config.bzl b/source/extensions/extensions_build_config.bzl index a3dc5309a637..4b7ed2228f47 100644 --- a/source/extensions/extensions_build_config.bzl +++ b/source/extensions/extensions_build_config.bzl @@ -279,6 +279,7 @@ EXTENSIONS = { # "envoy.tracers.opentelemetry.samplers.always_on": "//source/extensions/tracers/opentelemetry/samplers/always_on:config", + "envoy.tracers.opentelemetry.samplers.dynatrace": "//source/extensions/tracers/opentelemetry/samplers/dynatrace:config", # # Transport sockets diff --git a/source/extensions/extensions_metadata.yaml b/source/extensions/extensions_metadata.yaml index bf75bb02917f..2822b8e21329 100644 --- a/source/extensions/extensions_metadata.yaml +++ b/source/extensions/extensions_metadata.yaml @@ -1180,6 +1180,13 @@ envoy.tracers.opentelemetry.samplers.always_on: status: wip type_urls: - envoy.extensions.tracers.opentelemetry.samplers.v3.AlwaysOnSamplerConfig +envoy.tracers.opentelemetry.samplers.dynatrace: + categories: + - envoy.tracers.opentelemetry.samplers + security_posture: unknown + status: wip + type_urls: + - envoy.extensions.tracers.opentelemetry.samplers.v3.DynatraceSamplerConfig envoy.tracers.skywalking: categories: - envoy.tracers diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/BUILD b/source/extensions/tracers/opentelemetry/samplers/dynatrace/BUILD new file mode 100644 index 000000000000..629674856083 --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/BUILD @@ -0,0 +1,46 @@ +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_extension", + "envoy_cc_library", + "envoy_extension_package", +) + +licenses(["notice"]) # Apache 2 + +envoy_extension_package() + +envoy_cc_extension( + name = "config", + srcs = ["config.cc"], + hdrs = ["config.h"], + deps = [ + ":dynatrace_sampler_lib", + "//envoy/registry", + "//source/common/config:utility_lib", + "@envoy_api//envoy/extensions/tracers/opentelemetry/samplers/v3:pkg_cc_proto", + ], +) + +envoy_cc_library( + name = "dynatrace_sampler_lib", + srcs = [ + "dynatrace_sampler.cc", + "sampler_config.cc", + "sampler_config_provider.cc", + "sampling_controller.cc", + ], + hdrs = [ + "dynatrace_sampler.h", + "sampler_config.h", + "sampler_config_provider.h", + "sampling_controller.h", + "stream_summary.h", + "tenant_id.h", + ], + deps = [ + "//source/common/config:datasource_lib", + "//source/extensions/tracers/opentelemetry:opentelemetry_tracer_lib", + "//source/extensions/tracers/opentelemetry/samplers:sampler_lib", + "@envoy_api//envoy/extensions/tracers/opentelemetry/samplers/v3:pkg_cc_proto", + ], +) diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/config.cc b/source/extensions/tracers/opentelemetry/samplers/dynatrace/config.cc new file mode 100644 index 000000000000..bfcfb2d382f9 --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/config.cc @@ -0,0 +1,38 @@ +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/config.h" + +#include + +#include "envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.pb.validate.h" + +#include "source/common/config/utility.h" +#include "source/common/protobuf/utility.h" +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +SamplerSharedPtr +DynatraceSamplerFactory::createSampler(const Protobuf::Message& config, + Server::Configuration::TracerFactoryContext& context) { + auto mptr = Envoy::Config::Utility::translateAnyToFactoryConfig( + dynamic_cast(config), context.messageValidationVisitor(), *this); + + const auto& proto_config = MessageUtil::downcastAndValidate< + const envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig&>( + *mptr, context.messageValidationVisitor()); + + SamplerConfigProviderPtr cf = std::make_unique(context, proto_config); + return std::make_shared(proto_config, context, std::move(cf)); +} + +/** + * Static registration for the Dynatrace sampler factory. @see RegisterFactory. + */ +REGISTER_FACTORY(DynatraceSamplerFactory, SamplerFactory); + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/config.h b/source/extensions/tracers/opentelemetry/samplers/dynatrace/config.h new file mode 100644 index 000000000000..c317416dbfd7 --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/config.h @@ -0,0 +1,42 @@ +#pragma once + +#include + +#include "envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.pb.h" +#include "envoy/registry/registry.h" + +#include "source/extensions/tracers/opentelemetry/samplers/sampler.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +/** + * Config registration for the DynatraceSampler. @see SamplerFactory. + */ +class DynatraceSamplerFactory : public SamplerFactory { +public: + /** + * @brief Creates a Dynatrace sampler + * + * @param config The sampler configuration + * @param context The tracer factory context. + * @return SamplerSharedPtr + */ + SamplerSharedPtr createSampler(const Protobuf::Message& config, + Server::Configuration::TracerFactoryContext& context) override; + + ProtobufTypes::MessagePtr createEmptyConfigProto() override { + return std::make_unique< + envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig>(); + } + std::string name() const override { return "envoy.tracers.opentelemetry.samplers.dynatrace"; } +}; + +DECLARE_FACTORY(DynatraceSamplerFactory); + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler.cc b/source/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler.cc new file mode 100644 index 000000000000..5853d86e7650 --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler.cc @@ -0,0 +1,188 @@ +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler.h" + +#include +#include +#include + +#include "source/common/common/hash.h" +#include "source/common/config/datasource.h" +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/tenant_id.h" +#include "source/extensions/tracers/opentelemetry/samplers/sampler.h" +#include "source/extensions/tracers/opentelemetry/span_context.h" + +#include "absl/strings/str_cat.h" +#include "opentelemetry/trace/trace_state.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +namespace { + +constexpr std::chrono::minutes SAMPLING_UPDATE_TIMER_DURATION{1}; + +/** + * @brief Helper for creating and reading the Dynatrace tag in the tracestate http header + * This tag has at least 8 values delimited by semicolon: + * - tag[0]: version (currently version 4) + * - tag[1] - tag[4]: unused in the sampler (always 0) + * - tag[5]: ignored field. 1 if a span is ignored (not sampled), 0 otherwise + * - tag[6]: sampling exponent + * - tag[7]: path info + */ +class DynatraceTag { +public: + static DynatraceTag createInvalid() { return {false, false, 0, 0}; } + + // Creates a tag using the given values. + static DynatraceTag create(bool ignored, uint32_t sampling_exponent, uint32_t path_info) { + return {true, ignored, sampling_exponent, path_info}; + } + + // Creates a tag from a string. + static DynatraceTag create(const std::string& value) { + std::vector tracestate_components = + absl::StrSplit(value, ';', absl::AllowEmpty()); + if (tracestate_components.size() < 8) { + return createInvalid(); + } + + if (tracestate_components[0] != "fw4") { + return createInvalid(); + } + bool ignored = tracestate_components[5] == "1"; + uint32_t sampling_exponent; + uint32_t path_info; + if (absl::SimpleAtoi(tracestate_components[6], &sampling_exponent) && + absl::SimpleHexAtoi(tracestate_components[7], &path_info)) { + return {true, ignored, sampling_exponent, path_info}; + } + return createInvalid(); + } + + // Returns a Dynatrace tag as string. + std::string asString() const { + std::string ret = absl::StrCat("fw4;0;0;0;0;", ignored_ ? "1" : "0", ";", sampling_exponent_, + ";", absl::Hex(path_info_)); + return ret; + } + + // Returns true if parsing was successful. + bool isValid() const { return valid_; }; + bool isIgnored() const { return ignored_; }; + uint32_t getSamplingExponent() const { return sampling_exponent_; }; + +private: + DynatraceTag(bool valid, bool ignored, uint32_t sampling_exponent, uint32_t path_info) + : valid_(valid), ignored_(ignored), sampling_exponent_(sampling_exponent), + path_info_(path_info) {} + + bool valid_; + bool ignored_; + uint32_t sampling_exponent_; + uint32_t path_info_; +}; + +// add Dynatrace specific span attributes +void addSamplingAttributes(uint32_t sampling_exponent, + std::map& attributes) { + + const auto multiplicity = SamplingState::toMultiplicity(sampling_exponent); + // The denominator of the sampling ratio. If, for example, the Dynatrace OneAgent samples with a + // probability of 1/16, the value of supportability sampling ratio would be 16. + // Note: Ratio is also known as multiplicity. + attributes["supportability.atm_sampling_ratio"] = std::to_string(multiplicity); + + if (multiplicity > 1) { + static constexpr uint64_t two_pow_56 = 1llu << 56; // 2^56 + // The sampling probability can be interpreted as the number of spans + // that are discarded out of 2^56. The attribute is only available if the sampling.threshold is + // not 0 and therefore sampling happened. + const uint64_t sampling_threshold = two_pow_56 - two_pow_56 / multiplicity; + attributes["sampling.threshold"] = std::to_string(sampling_threshold); + } +} + +} // namespace + +DynatraceSampler::DynatraceSampler( + const envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig& config, + Server::Configuration::TracerFactoryContext& context, + SamplerConfigProviderPtr sampler_config_provider) + : dt_tracestate_key_(absl::StrCat(calculateTenantId(config.tenant()), "-", + absl::Hex(config.cluster_id()), "@dt")), + sampling_controller_(std::move(sampler_config_provider)) { + + // start a timer to periodically recalculate the sampling exponents + timer_ = context.serverFactoryContext().mainThreadDispatcher().createTimer([this]() -> void { + sampling_controller_.update(); + timer_->enableTimer(SAMPLING_UPDATE_TIMER_DURATION); + }); + timer_->enableTimer(SAMPLING_UPDATE_TIMER_DURATION); +} + +SamplingResult DynatraceSampler::shouldSample(const absl::optional parent_context, + const std::string& trace_id, + const std::string& /*name*/, OTelSpanKind /*kind*/, + OptRef trace_context, + const std::vector& /*links*/) { + + SamplingResult result; + std::map att; + + // trace_context->path() returns path and query. query part is removed in getSamplingKey() + const std::string sampling_key = + trace_context.has_value() + ? sampling_controller_.getSamplingKey(trace_context->path(), trace_context->method()) + : ""; + + sampling_controller_.offer(sampling_key); + + auto trace_state = opentelemetry::trace::TraceState::FromHeader( + parent_context.has_value() ? parent_context->tracestate() : ""); + + std::string trace_state_value; + bool is_root_span = true; + + if (trace_state->Get(dt_tracestate_key_, trace_state_value)) { + // we found a Dynatrace tag in the tracestate header. Respect the sampling decision in the tag. + if (DynatraceTag dynatrace_tag = DynatraceTag::create(trace_state_value); + dynatrace_tag.isValid()) { + result.decision = dynatrace_tag.isIgnored() ? Decision::Drop : Decision::RecordAndSample; + addSamplingAttributes(dynatrace_tag.getSamplingExponent(), att); + result.tracestate = parent_context->tracestate(); + is_root_span = false; + } + } + + if (is_root_span) { + // do a decision based on the calculated exponent + // we use a hash of the trace_id as random number + const auto hash = MurmurHash::murmurHash2(trace_id); + const auto sampling_state = sampling_controller_.getSamplingState(sampling_key); + const bool sample = sampling_state.shouldSample(hash); + const auto sampling_exponent = sampling_state.getExponent(); + + addSamplingAttributes(sampling_exponent, att); + + result.decision = sample ? Decision::RecordAndSample : Decision::Drop; + // create a new Dynatrace tag and add it to tracestate + DynatraceTag new_tag = + DynatraceTag::create(!sample, sampling_exponent, static_cast(hash)); + trace_state = trace_state->Set(dt_tracestate_key_, new_tag.asString()); + result.tracestate = trace_state->ToHeader(); + } + + if (!att.empty()) { + result.attributes = std::make_unique>(std::move(att)); + } + return result; +} + +std::string DynatraceSampler::getDescription() const { return "DynatraceSampler"; } + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler.h b/source/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler.h new file mode 100644 index 000000000000..80711ef77b3d --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler.h @@ -0,0 +1,54 @@ +#pragma once + +#include + +#include "envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.pb.h" +#include "envoy/server/factory_context.h" + +#include "source/common/common/logger.h" +#include "source/common/config/datasource.h" +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h" +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/sampling_controller.h" +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/stream_summary.h" +#include "source/extensions/tracers/opentelemetry/samplers/sampler.h" + +#include "absl/synchronization/mutex.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +/** + * @brief A Dynatrace specific sampler. + * + * For sampling, the requests are categorized based on the http method and the http path. + * A sampling multiplicity is calculated for every request category based on the + * number of requests. A Dynatrace specific tag is added to the http tracestate header. + */ +class DynatraceSampler : public Sampler, Logger::Loggable { +public: + DynatraceSampler( + const envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig& config, + Server::Configuration::TracerFactoryContext& context, + SamplerConfigProviderPtr sampler_config_provider); + + /** @see Sampler#shouldSample */ + SamplingResult shouldSample(const absl::optional parent_context, + const std::string& trace_id, const std::string& name, + OTelSpanKind spankind, + OptRef trace_context, + const std::vector& links) override; + + std::string getDescription() const override; + +private: + std::string dt_tracestate_key_; // used as key in the http tracestate header + Event::TimerPtr timer_; // used to periodically calculate sampling multiplicity + SamplingController sampling_controller_; +}; + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.cc b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.cc new file mode 100644 index 000000000000..b3a6f2b91c45 --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.cc @@ -0,0 +1,28 @@ +#include + +#include "source/common/json/json_loader.h" +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +void SamplerConfig::parse(const std::string& json) { + const auto result = Envoy::Json::Factory::loadFromStringNoThrow(json); + if (result.ok()) { + const auto& obj = result.value(); + if (obj->hasObject("rootSpansPerMinute")) { + const auto value = obj->getInteger("rootSpansPerMinute", default_root_spans_per_minute_); + root_spans_per_minute_.store(value); + return; + } + } + // Didn't get a value, reset to default + root_spans_per_minute_.store(default_root_spans_per_minute_); +} + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.h b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.h new file mode 100644 index 000000000000..6f576b2d5afb --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.h @@ -0,0 +1,47 @@ +#pragma once + +#include +#include +#include + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +/** + * @brief Contains the configuration for the sampler. Might be extended in a future version + * + */ +class SamplerConfig { +public: + static constexpr uint32_t ROOT_SPANS_PER_MINUTE_DEFAULT = 1000; + + explicit SamplerConfig(uint32_t default_root_spans_per_minute) + : default_root_spans_per_minute_(default_root_spans_per_minute > 0 + ? default_root_spans_per_minute + : ROOT_SPANS_PER_MINUTE_DEFAULT), + root_spans_per_minute_(default_root_spans_per_minute_) {} + /** + * @brief Parses a json string containing the expected root spans per minute. + * + * @param json A string containing the configuration. + */ + void parse(const std::string& json); + + /** + * @brief Returns wanted root spans per minute + * + * @return uint32_t wanted root spans per minute + */ + uint32_t getRootSpansPerMinute() const { return root_spans_per_minute_.load(); } + +private: + const uint32_t default_root_spans_per_minute_; + std::atomic root_spans_per_minute_; +}; + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc new file mode 100644 index 000000000000..465cb9e1bd5f --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc @@ -0,0 +1,21 @@ +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h" + +#include "source/common/common/enum_to_int.h" +#include "source/common/http/utility.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +SamplerConfigProviderImpl::SamplerConfigProviderImpl( + Server::Configuration::TracerFactoryContext& /*context*/, + const envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig& config) + : sampler_config_(config.root_spans_per_minute()) {} + +const SamplerConfig& SamplerConfigProviderImpl::getSamplerConfig() const { return sampler_config_; } + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h new file mode 100644 index 000000000000..6dd464d4f6c7 --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h @@ -0,0 +1,63 @@ +#pragma once + +#include +#include +#include + +#include "envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.pb.h" +#include "envoy/http/async_client.h" +#include "envoy/http/message.h" +#include "envoy/server/tracer_config.h" + +#include "source/common/http/async_client_impl.h" +#include "source/common/http/async_client_utility.h" +#include "source/common/http/headers.h" +#include "source/common/http/message_impl.h" +#include "source/common/http/utility.h" +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +/** + * @brief The Dynatrace sampling configuration provider. + * + * The configuration provider obtains sampling configuration from the Dynatrace API + * in order to dynamically tune up/down the sampling rate of spans. + */ +class SamplerConfigProvider { +public: + virtual ~SamplerConfigProvider() = default; + + /** + * @brief Get the Dynatrace Sampler configuration. + * + * @return const SamplerConfig& + */ + virtual const SamplerConfig& getSamplerConfig() const = 0; +}; + +class SamplerConfigProviderImpl : public SamplerConfigProvider, + public Logger::Loggable { +public: + SamplerConfigProviderImpl( + Server::Configuration::TracerFactoryContext& context, + const envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig& + config); + + const SamplerConfig& getSamplerConfig() const override; + + ~SamplerConfigProviderImpl() override = default; + +private: + SamplerConfig sampler_config_; +}; + +using SamplerConfigProviderPtr = std::unique_ptr; + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampling_controller.cc b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampling_controller.cc new file mode 100644 index 000000000000..e14ad0fa3493 --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampling_controller.cc @@ -0,0 +1,166 @@ +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/sampling_controller.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +namespace {} + +void SamplingController::update() { + absl::WriterMutexLock lock{&stream_summary_mutex_}; + const auto top_k = stream_summary_->getTopK(); + const auto last_period_count = stream_summary_->getN(); + + // update sampling exponents + update(top_k, last_period_count, + sampler_config_provider_->getSamplerConfig().getRootSpansPerMinute()); + // Note: getTopK() returns references to values in StreamSummary. + // Do not destroy it while top_k is used! + stream_summary_ = std::make_unique(STREAM_SUMMARY_SIZE); +} + +void SamplingController::update(const TopKListT& top_k, uint64_t last_period_count, + uint32_t total_wanted) { + + SamplingExponentsT new_sampling_exponents; + // start with sampling exponent 0, which means multiplicity == 1 (every span is sampled) + for (auto const& counter : top_k) { + new_sampling_exponents[counter.getItem()] = SamplingState(0); + } + + // use the last entry as "rest bucket", which is used for new/unknown requests + rest_bucket_key_ = (!top_k.empty()) ? top_k.back().getItem() : ""; + + calculateSamplingExponents(top_k, total_wanted, new_sampling_exponents); + last_effective_count_ = calculateEffectiveCount(top_k, new_sampling_exponents); + logSamplingInfo(top_k, new_sampling_exponents, last_period_count, total_wanted); + + absl::WriterMutexLock lock{&sampling_exponents_mutex_}; + sampling_exponents_ = std::move(new_sampling_exponents); +} + +uint64_t SamplingController::getEffectiveCount() const { + absl::ReaderMutexLock lock{&stream_summary_mutex_}; + return last_effective_count_; +} + +void SamplingController::offer(const std::string& sampling_key) { + if (!sampling_key.empty()) { + absl::WriterMutexLock lock{&stream_summary_mutex_}; + stream_summary_->offer(sampling_key); + } +} + +SamplingState SamplingController::getSamplingState(const std::string& sampling_key) const { + { // scope for lock + absl::ReaderMutexLock sax_lock{&sampling_exponents_mutex_}; + auto iter = sampling_exponents_.find(sampling_key); + if (iter != sampling_exponents_.end()) { + return iter->second; + } + + // try to use "rest bucket" + auto rest_bucket_iter = sampling_exponents_.find(rest_bucket_key_); + if (rest_bucket_iter != sampling_exponents_.end()) { + return rest_bucket_iter->second; + } + } + + // If we can't find a sampling exponent, we calculate it based on the total number of requests + // in this period. This should also handle the "warm up phase" where no top_k is available + const auto divisor = sampler_config_provider_->getSamplerConfig().getRootSpansPerMinute() / 2; + if (divisor == 0) { + return SamplingState{MAX_SAMPLING_EXPONENT}; + } + absl::ReaderMutexLock ss_lock{&stream_summary_mutex_}; + const uint32_t exp = stream_summary_->getN() / divisor; + return SamplingState{exp}; +} + +std::string SamplingController::getSamplingKey(const absl::string_view path_query, + const absl::string_view method) { + // remove query part (truncate after first '?') + const size_t query_offset = path_query.find('?'); + auto path = + path_query.substr(0, query_offset != path_query.npos ? query_offset : path_query.size()); + return absl::StrCat(method, "_", path); +} + +void SamplingController::logSamplingInfo(const TopKListT& top_k, + const SamplingExponentsT& new_sampling_exponents, + uint64_t last_period_count, uint32_t total_wanted) const { + ENVOY_LOG(debug, + "Updating sampling info. top_k.size(): {}, last_period_count: {}, total_wanted: {}", + top_k.size(), last_period_count, total_wanted); + for (auto const& counter : top_k) { + auto sampling_state = new_sampling_exponents.find(counter.getItem()); + ENVOY_LOG(debug, "- {}: value: {}, exponent: {}", counter.getItem(), counter.getValue(), + sampling_state->second.getExponent()); + } +} + +uint64_t SamplingController::calculateEffectiveCount(const TopKListT& top_k, + const SamplingExponentsT& sampling_exponents) { + uint64_t cnt = 0; + for (auto const& counter : top_k) { + auto sampling_state = sampling_exponents.find(counter.getItem()); + if (sampling_state != sampling_exponents.end()) { + auto counterVal = counter.getValue(); + auto mul = sampling_state->second.getMultiplicity(); + auto res = counterVal / mul; + cnt += res; + } + } + return cnt; +} + +void SamplingController::calculateSamplingExponents( + const TopKListT& top_k, uint32_t total_wanted, + SamplingExponentsT& new_sampling_exponents) const { + const auto top_k_size = top_k.size(); + if (top_k_size == 0 || total_wanted == 0) { + return; + } + + // number of requests which are allowed for every entry + const uint32_t allowed_per_entry = total_wanted / top_k_size; + + for (auto& counter : top_k) { + // allowed multiplicity for this entry + auto wanted_multiplicity = counter.getValue() / allowed_per_entry; + auto sampling_state = new_sampling_exponents.find(counter.getItem()); + // sampling exponent has to be a power of 2. Find the exponent to have multiplicity near to + // wanted_multiplicity + while (wanted_multiplicity > sampling_state->second.getMultiplicity() && + sampling_state->second.getExponent() < MAX_SAMPLING_EXPONENT) { + sampling_state->second.increaseExponent(); + } + if (wanted_multiplicity < sampling_state->second.getMultiplicity()) { + // we want to have multiplicity <= wanted_multiplicity, therefore exponent is decreased once. + sampling_state->second.decreaseExponent(); + } + } + + auto effective_count = calculateEffectiveCount(top_k, new_sampling_exponents); + // There might be entries where allowed_per_entry is greater than their count. + // Therefore, we would sample less than total_wanted. + // To avoid this, we decrease the exponent of other entries if possible + if (effective_count < total_wanted) { + for (int i = 0; i < 5; i++) { // max tries + for (auto reverse_it = top_k.rbegin(); reverse_it != top_k.rend(); + ++reverse_it) { // start with lowest frequency + auto rev_sampling_state = new_sampling_exponents.find(reverse_it->getItem()); + rev_sampling_state->second.decreaseExponent(); + effective_count = calculateEffectiveCount(top_k, new_sampling_exponents); + if (effective_count >= total_wanted) { // we are done + return; + } + } + } + } +} +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampling_controller.h b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampling_controller.h new file mode 100644 index 000000000000..b5871d4d4b6e --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampling_controller.h @@ -0,0 +1,134 @@ +#pragma once + +#include +#include + +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h" +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/stream_summary.h" + +#include "absl/synchronization/mutex.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +/** + * @brief Container for sampling exponent / multiplicity. + * based on the "Space Saving algorithm", AKA "HeavyHitter" + * See: + * https://cse.hkust.edu.hk/~raywong/comp5331/References/EfficientComputationOfFrequentAndTop-kElementsInDataStreams.pdf + * + */ +class SamplingState { +public: + // Convert exponent to multiplicity + [[nodiscard]] static uint32_t toMultiplicity(uint32_t exponent) { return 1 << exponent; } + [[nodiscard]] uint32_t getExponent() const { return exponent_; } + [[nodiscard]] uint32_t getMultiplicity() const { return toMultiplicity(exponent_); } + void increaseExponent() { exponent_++; } + void decreaseExponent() { + if (exponent_ > 0) { + exponent_--; + } + } + + explicit SamplingState(uint32_t exponent) : exponent_(exponent){}; + + SamplingState() = default; + + /** + * @brief Does a sampling decision based on random number attribute and multiplicity + * + * @param random_nr Random number used for sampling decision. + * @return true if request should be sampled, false otherwise + */ + bool shouldSample(const uint64_t random_nr) const { return (random_nr % getMultiplicity() == 0); } + +private: + uint32_t exponent_{0}; +}; + +using StreamSummaryT = StreamSummary; +using TopKListT = std::list>; + +/** + * @brief Counts the requests per sampling key in the current period. Calculates the sampling + * exponents based on the request count in the latest period. + * + */ +class SamplingController : public Logger::Loggable { + +public: + explicit SamplingController(SamplerConfigProviderPtr sampler_config_provider) + : stream_summary_(std::make_unique(STREAM_SUMMARY_SIZE)), + sampler_config_provider_(std::move(sampler_config_provider)) {} + + /** + * @brief Trigger calculating the sampling exponents based on the request count since last update + * + */ + void update(); + + /** + * @brief Get the Sampling State object for a sampling key + * + * @param sampling_key Sampling Key to search for + * @return SamplingState Current Sampling State for key + */ + SamplingState getSamplingState(const std::string& sampling_key) const; + + /** + * @brief Returns the number of spans which would have been sampled in the last period using the + * current sampling states + * + * @return effective count + */ + uint64_t getEffectiveCount() const; + + /** + * @brief Counts the occurrence of sampling_key + * + * @param sampling_key Sampling Key used to categorize the request + */ + void offer(const std::string& sampling_key); + + /** + * @brief Creates the Sampling Key which is used to categorize a request + * + * @param path_query The request path. May contain the query. + * @param method The request method. + * @return The sampling key. + */ + static std::string getSamplingKey(const absl::string_view path_query, + const absl::string_view method); + + static constexpr size_t STREAM_SUMMARY_SIZE{100}; + static constexpr uint32_t MAX_SAMPLING_EXPONENT = (1 << 4) - 1; // 15 + +private: + using SamplingExponentsT = absl::flat_hash_map; + SamplingExponentsT sampling_exponents_; + mutable absl::Mutex sampling_exponents_mutex_{}; + std::string rest_bucket_key_{}; + std::unique_ptr stream_summary_; + uint64_t last_effective_count_{}; + mutable absl::Mutex stream_summary_mutex_{}; + SamplerConfigProviderPtr sampler_config_provider_; + + void logSamplingInfo(const TopKListT& top_k, const SamplingExponentsT& new_sampling_exponents, + uint64_t last_period_count, uint32_t total_wanted) const; + + static uint64_t calculateEffectiveCount(const TopKListT& top_k, + const SamplingExponentsT& sampling_exponents); + + void calculateSamplingExponents(const TopKListT& top_k, uint32_t total_wanted, + SamplingExponentsT& new_sampling_exponents) const; + + void update(const TopKListT& top_k, uint64_t last_period_count, uint32_t total_wanted); +}; + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/stream_summary.h b/source/extensions/tracers/opentelemetry/samplers/dynatrace/stream_summary.h new file mode 100644 index 000000000000..b6385c91313d --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/stream_summary.h @@ -0,0 +1,204 @@ +#pragma once + +#include +#include +#include + +#include "source/common/common/assert.h" + +#include "absl/container/flat_hash_map.h" +#include "absl/status/status.h" +#include "absl/types/optional.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +namespace detail { + +template struct Bucket; + +template using BucketIterator = typename std::list>::iterator; + +template struct Counter { + BucketIterator bucket; + absl::optional item{}; + uint64_t value{}; + uint64_t error{}; + + explicit Counter(BucketIterator bucket) : bucket(bucket) {} + Counter(Counter const&) = delete; + Counter& operator=(Counter const&) = delete; +}; + +template using CounterIterator = typename std::list>::iterator; + +template struct Bucket { + uint64_t value; + std::list> children{}; + + explicit Bucket(uint64_t value) : value(value) {} + Bucket(Bucket const&) = delete; + Bucket& operator=(Bucket const&) = delete; +}; + +} // namespace detail + +template class Counter { +private: + T const& item_; + uint64_t value_; + uint64_t error_; + +public: + Counter(detail::Counter const& c) : item_(*c.item), value_(c.value), error_(c.error) {} + + T const& getItem() const { return item_; } + uint64_t getValue() const { return value_; } + uint64_t getError() const { return error_; } +}; + +/** + * @brief Space Saving algorithm implementation also know as "HeavyHitter". + * based on the "Space Saving algorithm", AKA "HeavyHitter" + * See: + * https://cse.hkust.edu.hk/~raywong/comp5331/References/EfficientComputationOfFrequentAndTop-kElementsInDataStreams.pdf + * https://github.com/fzakaria/space-saving/tree/master + * + */ +template class StreamSummary { +private: + const size_t capacity_; + uint64_t n_{}; + absl::flat_hash_map> cache_{}; + std::list> buckets_{}; + + typename detail::CounterIterator incrementCounter(detail::CounterIterator counter_iter, + uint64_t increment) { + auto const bucket = counter_iter->bucket; + auto bucket_next = std::prev(bucket); + counter_iter->value += increment; + + detail::CounterIterator elem; + if (bucket_next != buckets_.end() && counter_iter->value == bucket_next->value) { + counter_iter->bucket = bucket_next; + bucket_next->children.splice(bucket_next->children.end(), bucket->children, counter_iter); + elem = std::prev(bucket_next->children.end()); + } else { + auto bucket_new = buckets_.emplace(bucket, counter_iter->value); + counter_iter->bucket = bucket_new; + bucket_new->children.splice(bucket_new->children.end(), bucket->children, counter_iter); + elem = std::prev(bucket_new->children.end()); + } + if (bucket->children.empty()) { + buckets_.erase(bucket); + } + return elem; + } + + absl::Status validateInternal() const { + auto cache_copy = cache_; + auto current_bucket = buckets_.begin(); + uint64_t value_sum = 0; + while (current_bucket != buckets_.end()) { + auto prev = std::prev(current_bucket); + if (prev != buckets_.end() && prev->value <= current_bucket->value) { + return absl::InternalError("buckets should be in descending order."); + } + auto current_child = current_bucket->children.begin(); + while (current_child != current_bucket->children.end()) { + if (current_child->bucket != current_bucket || + current_child->value != current_bucket->value) { + return absl::InternalError("entry does not point to a bucket with the same value."); + } + if (current_child->item) { + auto old_iter = cache_copy.find(*current_child->item); + if (old_iter != cache_copy.end()) { + cache_copy.erase(old_iter); + } + } + value_sum += current_child->value; + current_child++; + } + current_bucket++; + } + if (!cache_copy.empty() || cache_.size() > capacity_ || value_sum != n_) { + return absl::InternalError("unexpected size."); + } + return absl::OkStatus(); + } + + inline void validateDbg() { +#if !defined(NDEBUG) + ASSERT(validate().ok()); +#endif + } + +public: + explicit StreamSummary(size_t capacity) : capacity_(capacity) { + auto& new_bucket = buckets_.emplace_back(0); + for (size_t i = 0; i < capacity; ++i) { + // initialize with empty counters, optional item will not be set + new_bucket.children.emplace_back(buckets_.begin()); + } + validateDbg(); + } + + size_t getCapacity() const { return capacity_; } + + absl::Status validate() const { return validateInternal(); } + + Counter offer(T const& item, uint64_t increment = 1) { + ++n_; + auto iter = cache_.find(item); + if (iter != cache_.end()) { + iter->second = incrementCounter(iter->second, increment); + validateDbg(); + return *iter->second; + } else { + auto min_element = std::prev(buckets_.back().children.end()); + auto original_min_value = min_element->value; + if (min_element + ->item) { // element was already used (otherwise optional item would be not set) + // remove old from cache + auto old_iter = cache_.find(*min_element->item); + if (old_iter != cache_.end()) { + cache_.erase(old_iter); + } + } + min_element->item = item; + min_element = incrementCounter(min_element, increment); + cache_[item] = min_element; + if (cache_.size() <= capacity_) { + // should always be true, but keep it to be aligned to reference implementation + // originalMinValue will be 0 if element wasn't already used + min_element->error = original_min_value; + } + validateDbg(); + return *min_element; + } + } + + uint64_t getN() const { return n_; } + + typename std::list> getTopK(size_t k = SIZE_MAX) const { + std::list> r; + for (auto const& bucket : buckets_) { + for (auto const& child : bucket.children) { + if (child.item) { + r.emplace_back(child); + if (r.size() == k) { + return r; + } + } + } + } + return r; + } +}; + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/tenant_id.h b/source/extensions/tracers/opentelemetry/samplers/dynatrace/tenant_id.h new file mode 100644 index 000000000000..e87e55f8c8d1 --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/tenant_id.h @@ -0,0 +1,50 @@ +#pragma once + +#include +#include + +#include "absl/strings/str_cat.h" +#include "openssl/md5.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +namespace { + +/** + * @brief Calculates a Dynatrace specific tenant id which is used in the Dynatrace tag added to the + * tracestate header. + * + * @param file_name The tenant as received via config file + * @return the tenant id as Hex + */ +absl::Hex calculateTenantId(std::string tenant_uuid) { + if (tenant_uuid.empty()) { + return absl::Hex(0); + } + + for (char& c : tenant_uuid) { + if (c & 0x80) { + c = 0x3f; // '?' + } + } + + uint8_t digest[16]; + MD5(reinterpret_cast(tenant_uuid.data()), tenant_uuid.size(), digest); + + int32_t hash = 0; + for (int i = 0; i < 16; i++) { + const int shift_for_target_byte = (3 - (i % 4)) * 8; + // 24, 16, 8, 0 respectively + hash ^= + (static_cast(digest[i]) << shift_for_target_byte) & (0xff << shift_for_target_byte); + } + return absl::Hex(hash); +} +} // namespace +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/tracer.cc b/source/extensions/tracers/opentelemetry/tracer.cc index 8c8553015510..ec28f6f69b18 100644 --- a/source/extensions/tracers/opentelemetry/tracer.cc +++ b/source/extensions/tracers/opentelemetry/tracer.cc @@ -38,7 +38,7 @@ void callSampler(SamplerSharedPtr sampler, const absl::optional spa return; } const auto sampling_result = - sampler->shouldSample(span_context, operation_name, new_span.getTraceIdAsHex(), + sampler->shouldSample(span_context, new_span.getTraceIdAsHex(), operation_name, new_span.spankind(), trace_context, {}); new_span.setSampled(sampling_result.isSampled()); diff --git a/test/extensions/tracers/opentelemetry/samplers/dynatrace/BUILD b/test/extensions/tracers/opentelemetry/samplers/dynatrace/BUILD new file mode 100644 index 000000000000..b708876da067 --- /dev/null +++ b/test/extensions/tracers/opentelemetry/samplers/dynatrace/BUILD @@ -0,0 +1,62 @@ +load( + "//bazel:envoy_build_system.bzl", + "envoy_package", +) +load( + "//test/extensions:extensions_build_system.bzl", + "envoy_extension_cc_test", +) + +licenses(["notice"]) # Apache 2 + +envoy_package() + +envoy_extension_cc_test( + name = "config_test", + srcs = ["config_test.cc"], + extension_names = ["envoy.tracers.opentelemetry.samplers.dynatrace"], + deps = [ + "//envoy/registry", + "//source/extensions/tracers/opentelemetry/samplers/dynatrace:config", + "//source/extensions/tracers/opentelemetry/samplers/dynatrace:dynatrace_sampler_lib", + "//test/mocks/server:tracer_factory_context_mocks", + "//test/test_common:utility_lib", + ], +) + +envoy_extension_cc_test( + name = "dynatrace_sampler_test", + srcs = [ + "dynatrace_sampler_test.cc", + "sampler_config_provider_test.cc", + "sampler_config_test.cc", + "sampling_controller_test.cc", + "stream_summary_test.cc", + "tenant_id_test.cc", + ], + extension_names = ["envoy.tracers.opentelemetry.samplers.dynatrace"], + deps = [ + "//source/extensions/tracers/opentelemetry/samplers/dynatrace:dynatrace_sampler_lib", + "//test/mocks/server:tracer_factory_context_mocks", + "//test/test_common:utility_lib", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/tracers/opentelemetry/samplers/v3:pkg_cc_proto", + ], +) + +envoy_extension_cc_test( + name = "dynatrace_sampler_integration_test", + srcs = [ + "dynatrace_sampler_integration_test.cc", + ], + extension_names = ["envoy.tracers.opentelemetry.samplers.dynatrace"], + deps = [ + "//source/exe:main_common_lib", + "//source/extensions/tracers/opentelemetry:config", + "//source/extensions/tracers/opentelemetry/resource_detectors/dynatrace:config", + "//source/extensions/tracers/opentelemetry/samplers/dynatrace:config", + "//test/integration:http_integration_lib", + "//test/test_common:utility_lib", + "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", + ], +) diff --git a/test/extensions/tracers/opentelemetry/samplers/dynatrace/config_test.cc b/test/extensions/tracers/opentelemetry/samplers/dynatrace/config_test.cc new file mode 100644 index 000000000000..6860df5c0ece --- /dev/null +++ b/test/extensions/tracers/opentelemetry/samplers/dynatrace/config_test.cc @@ -0,0 +1,38 @@ +#include "envoy/registry/registry.h" + +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/config.h" + +#include "test/mocks/server/tracer_factory_context.h" +#include "test/test_common/utility.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +// Test creating a Dynatrace sampler via factory +TEST(DynatraceSamplerFactoryTest, Test) { + auto* factory = Registry::FactoryRegistry::getFactory( + "envoy.tracers.opentelemetry.samplers.dynatrace"); + ASSERT_NE(factory, nullptr); + EXPECT_STREQ(factory->name().c_str(), "envoy.tracers.opentelemetry.samplers.dynatrace"); + EXPECT_NE(factory->createEmptyConfigProto(), nullptr); + + envoy::config::core::v3::TypedExtensionConfig typed_config; + const std::string sampler_yaml = R"EOF( + name: envoy.tracers.opentelemetry.samplers.dynatrace + typed_config: + "@type": type.googleapis.com/envoy.extensions.tracers.opentelemetry.samplers.v3.DynatraceSamplerConfig + )EOF"; + TestUtility::loadFromYaml(sampler_yaml, typed_config); + + NiceMock context; + EXPECT_NE(factory->createSampler(typed_config.typed_config(), context), nullptr); +} + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler_integration_test.cc b/test/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler_integration_test.cc new file mode 100644 index 000000000000..a3887eaf5ab4 --- /dev/null +++ b/test/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler_integration_test.cc @@ -0,0 +1,151 @@ +#include +#include + +#include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" + +#include "test/integration/http_integration.h" +#include "test/test_common/utility.h" + +#include "absl/strings/match.h" +#include "absl/strings/string_view.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { +namespace { + +const char* TRACEPARENT_VALUE = "00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01"; +const char* TRACEPARENT_VALUE_START = "00-0af7651916cd43dd8448eb211c80319c"; + +class DynatraceSamplerIntegrationTest : public Envoy::HttpIntegrationTest, + public testing::TestWithParam { +public: + DynatraceSamplerIntegrationTest() : HttpIntegrationTest(Http::CodecType::HTTP1, GetParam()) { + const std::string yaml_string = R"EOF( + provider: + name: envoy.tracers.opentelemetry + typed_config: + "@type": type.googleapis.com/envoy.config.trace.v3.OpenTelemetryConfig + grpc_service: + envoy_grpc: + cluster_name: opentelemetry_collector + timeout: 0.250s + service_name: "a_service_name" + sampler: + name: envoy.tracers.opentelemetry.samplers.dynatrace + typed_config: + "@type": type.googleapis.com/envoy.extensions.tracers.opentelemetry.samplers.v3.DynatraceSamplerConfig + tenant: "abc12345" + cluster_id: -1743916452 + )EOF"; + + auto tracing_config = + std::make_unique<::envoy::extensions::filters::network::http_connection_manager::v3:: + HttpConnectionManager_Tracing>(); + TestUtility::loadFromYaml(yaml_string, *tracing_config.get()); + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { hcm.set_allocated_tracing(tracing_config.release()); }); + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + } +}; + +INSTANTIATE_TEST_SUITE_P(IpVersions, DynatraceSamplerIntegrationTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + +// Sends a request with traceparent and tracestate header. +TEST_P(DynatraceSamplerIntegrationTest, TestWithTraceparentAndTracestate) { + // tracestate does not contain a Dynatrace tag + Http::TestRequestHeaderMapImpl request_headers{ + {":method", "GET"}, {":path", "/test/long/url"}, {":scheme", "http"}, + {":authority", "host"}, {"tracestate", "key=value"}, {"traceparent", TRACEPARENT_VALUE}}; + + auto response = sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 0); + + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ(response->headers().getStatusValue(), "200"); + + // traceparent should be set: traceid should be re-used, span id should be different + absl::string_view traceparent_value = upstream_request_->headers() + .get(Http::LowerCaseString("traceparent"))[0] + ->value() + .getStringView(); + EXPECT_TRUE(absl::StartsWith(traceparent_value, TRACEPARENT_VALUE_START)); + EXPECT_NE(TRACEPARENT_VALUE, traceparent_value); + // Dynatrace tracestate should be added to existing tracestate + absl::string_view tracestate_value = upstream_request_->headers() + .get(Http::LowerCaseString("tracestate"))[0] + ->value() + .getStringView(); + // use StartsWith because path-info (last element in trace state) contains a random value + EXPECT_TRUE(absl::StartsWith(tracestate_value, "5b3f9fed-980df25c@dt=fw4;0;0;0;0;0;0;")) + << "Received tracestate: " << tracestate_value; + EXPECT_TRUE(absl::StrContains(tracestate_value, ",key=value")) + << "Received tracestate: " << tracestate_value; +} + +// Sends a request with traceparent but no tracestate header. +TEST_P(DynatraceSamplerIntegrationTest, TestWithTraceparentOnly) { + Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"traceparent", TRACEPARENT_VALUE}}; + auto response = sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 0); + + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ(response->headers().getStatusValue(), "200"); + + // traceparent should be set: traceid should be re-used, span id should be different + absl::string_view traceparent_value = upstream_request_->headers() + .get(Http::LowerCaseString("traceparent"))[0] + ->value() + .getStringView(); + EXPECT_TRUE(absl::StartsWith(traceparent_value, TRACEPARENT_VALUE_START)); + EXPECT_NE(TRACEPARENT_VALUE, traceparent_value); + // Dynatrace tag should be added to tracestate + absl::string_view tracestate_value = upstream_request_->headers() + .get(Http::LowerCaseString("tracestate"))[0] + ->value() + .getStringView(); + // use StartsWith because path-info (last element in trace state contains a random value) + EXPECT_TRUE(absl::StartsWith(tracestate_value, "5b3f9fed-980df25c@dt=fw4;0;0;0;0;0;0;")) + << "Received tracestate: " << tracestate_value; +} + +// Sends a request without traceparent and tracestate header. +TEST_P(DynatraceSamplerIntegrationTest, TestWithoutTraceparentAndTracestate) { + Http::TestRequestHeaderMapImpl request_headers{ + {":method", "GET"}, {":path", "/test/long/url"}, {":scheme", "http"}, {":authority", "host"}}; + + auto response = sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 0); + + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ(response->headers().getStatusValue(), "200"); + + // traceparent will be added, trace_id and span_id will be generated, so there is nothing we can + // assert + EXPECT_EQ(upstream_request_->headers().get(::Envoy::Http::LowerCaseString("traceparent")).size(), + 1); + // Dynatrace tag should be added to tracestate + absl::string_view tracestate_value = upstream_request_->headers() + .get(Http::LowerCaseString("tracestate"))[0] + ->value() + .getStringView(); + EXPECT_TRUE(absl::StartsWith(tracestate_value, "5b3f9fed-980df25c@dt=fw4;0;0;0;0;0;0;")) + << "Received tracestate: " << tracestate_value; +} + +} // namespace +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler_test.cc b/test/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler_test.cc new file mode 100644 index 000000000000..44f09c7a600c --- /dev/null +++ b/test/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler_test.cc @@ -0,0 +1,336 @@ +#include +#include + +#include "envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.pb.h" + +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler.h" +#include "source/extensions/tracers/opentelemetry/span_context.h" + +#include "test/mocks/server/tracer_factory_context.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +namespace { + +const char* trace_id = "67a9a23155e1741b5b35368e08e6ece5"; + +const char* parent_span_id = "9d83def9a4939b7b"; + +const char* dt_tracestate_ignored = + "5b3f9fed-980df25c@dt=fw4;4;4af38366;0;0;1;2;123;8eae;2h01;3h4af38366;4h00;5h01;" + "6h67a9a23155e1741b5b35368e08e6ece5;7h9d83def9a4939b7b"; +const char* dt_tracestate_sampled = + "5b3f9fed-980df25c@dt=fw4;4;4af38366;0;0;0;0;123;8eae;2h01;3h4af38366;4h00;5h01;" + "6h67a9a23155e1741b5b35368e08e6ece5;7h9d83def9a4939b7b"; +const char* dt_tracestate_ignored_different_tenant = + "6666ad40-980df25c@dt=fw4;4;4af38366;0;0;1;2;123;8eae;2h01;3h4af38366;4h00;5h01;" + "6h67a9a23155e1741b5b35368e08e6ece5;7h9d83def9a4939b7b"; + +} // namespace + +class MockSamplerConfigProvider : public SamplerConfigProvider { +public: + MOCK_METHOD(const SamplerConfig&, getSamplerConfig, (), (const override)); +}; + +class DynatraceSamplerTest : public testing::Test { + + const std::string yaml_string_ = R"EOF( + tenant: "abc12345" + cluster_id: -1743916452 + )EOF"; + +public: + DynatraceSamplerTest() { + TestUtility::loadFromYaml(yaml_string_, proto_config_); + auto scf = std::make_unique>(); + ON_CALL(*scf, getSamplerConfig()).WillByDefault(testing::ReturnRef(sampler_config_)); + + timer_ = new NiceMock( + &tracer_factory_context_.server_factory_context_.dispatcher_); + ON_CALL(tracer_factory_context_.server_factory_context_.dispatcher_, createTimer_(_)) + .WillByDefault(Invoke([this](Event::TimerCb) { return timer_; })); + sampler_ = + std::make_unique(proto_config_, tracer_factory_context_, std::move(scf)); + } + +protected: + NiceMock tracer_factory_context_; + envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig proto_config_; + SamplerConfig sampler_config_{SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT}; + NiceMock* timer_; + std::unique_ptr sampler_; +}; + +// Verify getDescription +TEST_F(DynatraceSamplerTest, TestGetDescription) { + EXPECT_STREQ(sampler_->getDescription().c_str(), "DynatraceSampler"); +} + +// Verify sampler being invoked with an invalid/empty span context +TEST_F(DynatraceSamplerTest, TestWithoutParentContext) { + auto sampling_result = + sampler_->shouldSample(absl::nullopt, trace_id, "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {}); + EXPECT_EQ(sampling_result.decision, Decision::RecordAndSample); + EXPECT_EQ(sampling_result.attributes->size(), 1); + EXPECT_STREQ( + sampling_result.attributes->find("supportability.atm_sampling_ratio")->second.c_str(), "1"); + EXPECT_STREQ(sampling_result.tracestate.c_str(), "5b3f9fed-980df25c@dt=fw4;0;0;0;0;0;0;95"); + EXPECT_TRUE(sampling_result.isRecording()); + EXPECT_TRUE(sampling_result.isSampled()); +} + +// Verify sampler being invoked without a Dynatrace tracestate +TEST_F(DynatraceSamplerTest, TestWithUnknownParentContext) { + SpanContext parent_context("00", trace_id, parent_span_id, true, "some_vendor=some_value"); + + auto sampling_result = + sampler_->shouldSample(parent_context, trace_id, "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {}); + EXPECT_EQ(sampling_result.decision, Decision::RecordAndSample); + EXPECT_EQ(sampling_result.attributes->size(), 1); + EXPECT_STREQ( + sampling_result.attributes->find("supportability.atm_sampling_ratio")->second.c_str(), "1"); + // Dynatrace tracestate should be prepended + EXPECT_STREQ(sampling_result.tracestate.c_str(), + "5b3f9fed-980df25c@dt=fw4;0;0;0;0;0;0;95,some_vendor=some_value"); + EXPECT_TRUE(sampling_result.isRecording()); + EXPECT_TRUE(sampling_result.isSampled()); +} + +// Verify sampler being invoked with Dynatrace trace state +TEST_F(DynatraceSamplerTest, TestWithDynatraceParentContextSampled) { + SpanContext parent_context("00", trace_id, parent_span_id, true, dt_tracestate_sampled); + + auto sampling_result = + sampler_->shouldSample(parent_context, trace_id, "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {}); + EXPECT_EQ(sampling_result.decision, Decision::RecordAndSample); + EXPECT_EQ(sampling_result.attributes->size(), 1); + EXPECT_STREQ( + sampling_result.attributes->find("supportability.atm_sampling_ratio")->second.c_str(), "1"); + // tracestate should be forwarded + EXPECT_STREQ(sampling_result.tracestate.c_str(), dt_tracestate_sampled); + // sampling decision from parent should be respected + EXPECT_TRUE(sampling_result.isRecording()); + EXPECT_TRUE(sampling_result.isSampled()); +} + +// Verify sampler being invoked with an invalid Dynatrace trace state +TEST_F(DynatraceSamplerTest, TestWithInvalidDynatraceParentContext) { + const char* invalidts = "5b3f9fed-980df25c@dt=fw4;4"; + SpanContext parent_context("00", trace_id, parent_span_id, true, invalidts); + + auto sampling_result = + sampler_->shouldSample(parent_context, trace_id, "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {}); + EXPECT_EQ(sampling_result.decision, Decision::RecordAndSample); + EXPECT_STREQ(sampling_result.tracestate.c_str(), + "5b3f9fed-980df25c@dt=fw4;0;0;0;0;0;0;95,5b3f9fed-980df25c@dt=fw4;4"); + EXPECT_TRUE(sampling_result.isRecording()); + EXPECT_TRUE(sampling_result.isSampled()); +} + +// Verify sampler being invoked with an invalid Dynatrace trace state +TEST_F(DynatraceSamplerTest, TestWithInvalidDynatraceParentContext1) { + // invalid tracestate[6] has to be an int + const char* invalidts = "5b3f9fed-980df25c@dt=fw4;4;4af38366;0;0;0;X;123"; + SpanContext parent_context("00", trace_id, parent_span_id, true, invalidts); + + auto sampling_result = + sampler_->shouldSample(parent_context, trace_id, "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {}); + EXPECT_EQ(sampling_result.decision, Decision::RecordAndSample); + EXPECT_STREQ( + sampling_result.tracestate.c_str(), + "5b3f9fed-980df25c@dt=fw4;0;0;0;0;0;0;95,5b3f9fed-980df25c@dt=fw4;4;4af38366;0;0;0;X;123"); + EXPECT_TRUE(sampling_result.isRecording()); + EXPECT_TRUE(sampling_result.isSampled()); +} + +// Verify sampler being invoked with an old Dynatrace trace state version +TEST_F(DynatraceSamplerTest, TestWithDynatraceParentContextOtherVersion) { + const char* oldts = + "5b3f9fed-980df25c@dt=fw3;4;4af38366;0;0;0;0;123;8eae;2h01;3h4af38366;4h00;5h01;" + "6h67a9a23155e1741b5b35368e08e6ece5;7h9d83def9a4939b7b"; + SpanContext parent_context("00", trace_id, parent_span_id, true, oldts); + + auto sampling_result = + sampler_->shouldSample(parent_context, trace_id, "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {}); + EXPECT_EQ(sampling_result.decision, Decision::RecordAndSample); + EXPECT_STREQ( + sampling_result.tracestate.c_str(), + "5b3f9fed-980df25c@dt=fw4;0;0;0;0;0;0;95,5b3f9fed-980df25c@dt=fw3;4;4af38366;0;0;0;0;123;" + "8eae;2h01;3h4af38366;4h00;5h01;6h67a9a23155e1741b5b35368e08e6ece5;7h9d83def9a4939b7b"); + EXPECT_TRUE(sampling_result.isRecording()); + EXPECT_TRUE(sampling_result.isSampled()); +} + +// Verify sampler being invoked with Dynatrace trace parent where ignored flag is set +TEST_F(DynatraceSamplerTest, TestWithDynatraceParentContextIgnored) { + SpanContext parent_context("00", trace_id, parent_span_id, true, dt_tracestate_ignored); + + auto sampling_result = + sampler_->shouldSample(parent_context, trace_id, "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {}); + EXPECT_EQ(sampling_result.decision, Decision::Drop); + EXPECT_EQ(sampling_result.attributes->size(), 2); + EXPECT_STREQ( + sampling_result.attributes->find("supportability.atm_sampling_ratio")->second.c_str(), "4"); + EXPECT_STREQ(sampling_result.attributes->find("sampling.threshold")->second.c_str(), + "54043195528445952"); + // tracestate should be forwarded + EXPECT_STREQ(sampling_result.tracestate.c_str(), dt_tracestate_ignored); + // sampling decision from parent should be respected + EXPECT_FALSE(sampling_result.isRecording()); + EXPECT_FALSE(sampling_result.isSampled()); +} + +// Verify sampler being invoked with Dynatrace trace parent from a different tenant +TEST_F(DynatraceSamplerTest, TestWithDynatraceParentContextFromDifferentTenant) { + SpanContext parent_context("00", trace_id, parent_span_id, true, + dt_tracestate_ignored_different_tenant); + + auto sampling_result = + sampler_->shouldSample(parent_context, trace_id, "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {}); + // sampling decision on tracestate should be ignored because it is from a different tenant. + EXPECT_EQ(sampling_result.decision, Decision::RecordAndSample); + EXPECT_EQ(sampling_result.attributes->size(), 1); + EXPECT_STREQ( + sampling_result.attributes->find("supportability.atm_sampling_ratio")->second.c_str(), "1"); + // new Dynatrace tag should be prepended, already existing tag should be kept + const char* exptected = + "5b3f9fed-980df25c@dt=fw4;0;0;0;0;0;0;95,6666ad40-980df25c@dt=fw4;4;4af38366;0;0;1;2;123;" + "8eae;2h01;3h4af38366;4h00;5h01;6h67a9a23155e1741b5b35368e08e6ece5;7h9d83def9a4939b7b"; + EXPECT_STREQ(sampling_result.tracestate.c_str(), exptected); + EXPECT_TRUE(sampling_result.isRecording()); + EXPECT_TRUE(sampling_result.isSampled()); +} + +// Verify sampler being called during warm up phase (no recent top_k available) +TEST_F(DynatraceSamplerTest, TestWarmup) { + // config should allow 200 root spans per minute + sampler_config_.parse("{\n \"rootSpansPerMinute\" : 200 \n }"); + + Tracing::TestTraceContextImpl trace_context_1{}; + trace_context_1.context_method_ = "GET"; + trace_context_1.context_path_ = "/path"; + + // timer is not invoked, because we want to test warm up phase. + // we use 200 as threshold. As long as number of requests is < (threshold/2), exponent should be 0 + uint32_t ignored = 0; + uint32_t sampled = 0; + for (int i = 0; i < 99; i++) { + auto result = sampler_->shouldSample({}, std::to_string(1000 + i), "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, + trace_context_1, {}); + result.isSampled() ? sampled++ : ignored++; + } + EXPECT_EQ(ignored, 0); + EXPECT_EQ(sampled, 99); + + // next (threshold/2) spans will get exponent 1, every second span will be sampled + for (int i = 0; i < 100; i++) { + auto result = sampler_->shouldSample({}, std::to_string(1000 + i), "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, + trace_context_1, {}); + result.isSampled() ? sampled++ : ignored++; + } + // should be 50 ignored, but the used "random" in shouldSample does not produce the same odd/even + // numbers. + EXPECT_EQ(ignored, 41); + EXPECT_EQ(sampled, 158); + + // send more requests + for (int i = 0; i < 100; i++) { + auto result = sampler_->shouldSample({}, std::to_string(1000 + i), "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, + trace_context_1, {}); + result.isSampled() ? sampled++ : ignored++; + } + // exponent should be 2, with a perfect random we would get 25 sampled and 75 ignored. + EXPECT_EQ(ignored, 113); + EXPECT_EQ(sampled, 186); + + // send more requests. + for (int i = 0; i < 700; i++) { + auto result = sampler_->shouldSample({}, std::to_string(1000 + i), "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, + trace_context_1, {}); + result.isSampled() ? sampled++ : ignored++; + } + // with a perfect random, the number of sampled paths would be lower than threshold (200) + // We don't care about exceeding the threshold because it is not a hard limit + EXPECT_EQ(ignored, 791); + EXPECT_EQ(sampled, 208); +} + +// Verify sampling if number of configured spans per minute is exceeded. +TEST_F(DynatraceSamplerTest, TestSampling) { + // config should allow 200 root spans per minute + sampler_config_.parse("{\n \"rootSpansPerMinute\" : 200 \n }"); + + Tracing::TestTraceContextImpl trace_context_1{}; + trace_context_1.context_method_ = "GET"; + trace_context_1.context_path_ = "/path"; + Tracing::TestTraceContextImpl trace_context_2{}; + trace_context_2.context_method_ = "POST"; + trace_context_2.context_path_ = "/path"; + Tracing::TestTraceContextImpl trace_context_3{}; + trace_context_3.context_method_ = "POST"; + trace_context_3.context_path_ = "/another_path"; + + // simulate requests + for (int i = 0; i < 180; i++) { + sampler_->shouldSample({}, trace_id, "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, + trace_context_1, {}); + sampler_->shouldSample({}, trace_id, "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, + trace_context_2, {}); + } + + sampler_->shouldSample({}, trace_id, "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, trace_context_3, + {}); + + // sampler should update sampling exponents based on number of requests in the previous period + timer_->invokeCallback(); + + // the sampler should not sample every span for 'trace_context_1' + // we call it again 10 times. This should be enough to get at least one ignored span + // 'i' is used as 'random trace_id' + bool ignored = false; + for (int i = 0; i < 10; i++) { + auto result = sampler_->shouldSample({}, std::to_string(i), "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, + trace_context_1, {}); + if (!result.isSampled()) { + ignored = true; + break; + } + } + EXPECT_TRUE(ignored); + + // trace_context_3 should always be sampled. + for (int i = 0; i < 10; i++) { + auto result = sampler_->shouldSample({}, std::to_string(i), "operation_name", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, + trace_context_2, {}); + EXPECT_TRUE(result.isSampled()); + } +} + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider_test.cc b/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider_test.cc new file mode 100644 index 000000000000..d5a8c68d71b8 --- /dev/null +++ b/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider_test.cc @@ -0,0 +1,95 @@ +#include +#include +#include + +#include "envoy/config/core/v3/http_uri.pb.h" + +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.h" +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h" + +#include "test/mocks/common.h" +#include "test/mocks/http/mocks.h" +#include "test/mocks/server/tracer_factory_context.h" +#include "test/mocks/tracing/mocks.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +using testing::NiceMock; +using testing::Return; +using testing::ReturnRef; + +class SamplerConfigProviderTest : public testing::Test { +public: +protected: + NiceMock tracer_factory_context_; +}; + +TEST_F(SamplerConfigProviderTest, TestValueConfigured) { + const std::string yaml_string = R"EOF( + tenant: "abc12345" + cluster_id: -1743916452 + token: "tokenval" + http_uri: + cluster: "cluster_name" + uri: "https://testhost.com/otlp/v1/traces" + timeout: 0.250s + root_spans_per_minute: 3456 + + )EOF"; + + envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig proto_config; + TestUtility::loadFromYaml(yaml_string, proto_config); + + SamplerConfigProviderImpl config_provider(tracer_factory_context_, proto_config); + EXPECT_EQ(config_provider.getSamplerConfig().getRootSpansPerMinute(), 3456); +} + +TEST_F(SamplerConfigProviderTest, TestNoValueConfigured) { + const std::string yaml_string = R"EOF( + tenant: "abc12345" + cluster_id: -1743916452 + token: "tokenval" + http_uri: + cluster: "cluster_name" + uri: "https://testhost.com/otlp/v1/traces" + timeout: 0.250s + + )EOF"; + + envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig proto_config; + TestUtility::loadFromYaml(yaml_string, proto_config); + + SamplerConfigProviderImpl config_provider(tracer_factory_context_, proto_config); + EXPECT_EQ(config_provider.getSamplerConfig().getRootSpansPerMinute(), + SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); +} + +TEST_F(SamplerConfigProviderTest, TestValueZeroConfigured) { + const std::string yaml_string = R"EOF( + tenant: "abc12345" + cluster_id: -1743916452 + token: "tokenval" + http_uri: + cluster: "cluster_name" + uri: "https://testhost.com/otlp/v1/traces" + timeout: 0.250s + root_spans_per_minute: 0 + )EOF"; + + envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig proto_config; + TestUtility::loadFromYaml(yaml_string, proto_config); + + SamplerConfigProviderImpl config_provider(tracer_factory_context_, proto_config); + EXPECT_EQ(config_provider.getSamplerConfig().getRootSpansPerMinute(), + SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); +} + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_test.cc b/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_test.cc new file mode 100644 index 000000000000..8cb45a8ca3e6 --- /dev/null +++ b/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_test.cc @@ -0,0 +1,64 @@ +#include +#include + +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +// Test sampler config json parsing +TEST(SamplerConfigTest, TestParsing) { + // default_root_spans_per_minute not set, ROOT_SPANS_PER_MINUTE_DEFAULT should be used + SamplerConfig config(0); + config.parse("{\n \"rootSpansPerMinute\" : 2000 \n }"); + EXPECT_EQ(config.getRootSpansPerMinute(), 2000u); + config.parse("{\n \"rootSpansPerMinute\" : 10000 \n }"); + EXPECT_EQ(config.getRootSpansPerMinute(), 10000u); + + // unexpected json, default value should be used + config.parse("{}"); + EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); + + config.parse(""); + EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); + + config.parse("\\"); + EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); + + config.parse(" { "); + EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); + + config.parse("{\n \"rootSpansPerMinute\" : 10000 "); // closing } is missing + EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); +} + +// Test sampler config default root spans per minute +TEST(SamplerConfigTest, TestDefaultConfig) { + { + SamplerConfig config(0); + EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); + config.parse(" { "); // parse invalid json, default value should still be used + EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); + } + { + SamplerConfig config(900); + EXPECT_EQ(config.getRootSpansPerMinute(), 900); + config.parse(" { "); + EXPECT_EQ(config.getRootSpansPerMinute(), 900); + } + { + SamplerConfig config(SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); + EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); + config.parse(" { "); + EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); + } +} + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampling_controller_test.cc b/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampling_controller_test.cc new file mode 100644 index 000000000000..f56f2c86d4a3 --- /dev/null +++ b/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampling_controller_test.cc @@ -0,0 +1,321 @@ +#include +#include +#include +#include +#include +#include + +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler.h" +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/sampling_controller.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +namespace { + +// helper to offer a value multiple times to SamplingController +void offerEntry(SamplingController& sc, const std::string& value, int count) { + for (int i = 0; i < count; i++) { + sc.offer(value); + } +} + +} // namespace + +class TestSamplerConfigProvider : public SamplerConfigProvider { +public: + TestSamplerConfigProvider( + uint32_t root_spans_per_minute = SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT) + : config(root_spans_per_minute) {} + const SamplerConfig& getSamplerConfig() const override { return config; } + SamplerConfig config; +}; + +// Test with multiple different sampling keys (StreamSummary size exceeded) +TEST(SamplingControllerTest, TestStreamSummarySizeExceeded) { + auto scf = std::make_unique(); + SamplingController sc(std::move(scf)); + + offerEntry(sc, "1", 2000); + offerEntry(sc, "2", 1000); + offerEntry(sc, "3", 750); + offerEntry(sc, "4", 100); + offerEntry(sc, "5", 50); + // add unique sampling keys + for (int64_t i = 0; i < 2100; i++) { + sc.offer(std::to_string(i + 1000000)); + } + + sc.update(); + + EXPECT_EQ(sc.getEffectiveCount(), 1110); + EXPECT_EQ(sc.getSamplingState("1").getMultiplicity(), 128); + EXPECT_EQ(sc.getSamplingState("2").getMultiplicity(), 64); + EXPECT_EQ(sc.getSamplingState("3").getMultiplicity(), 64); + EXPECT_EQ(sc.getSamplingState("4").getMultiplicity(), 8); + EXPECT_EQ(sc.getSamplingState("5").getMultiplicity(), 4); + EXPECT_EQ(sc.getSamplingState("1000000").getMultiplicity(), 2); + EXPECT_EQ(sc.getSamplingState("1000001").getMultiplicity(), 2); + EXPECT_EQ(sc.getSamplingState("1000002").getMultiplicity(), 2); +} + +// Test with 0 root span per minute +TEST(SamplingControllerTest, TestWithZeroAllowedSpan) { + // using 0 does not make sense, but let's ensure there is divide by zero + auto scf = std::make_unique(0); + SamplingController sc(std::move(scf)); + EXPECT_EQ(sc.getSamplingState("1").getMultiplicity(), 1); + sc.update(); + EXPECT_EQ(sc.getSamplingState("1").getMultiplicity(), 1); + offerEntry(sc, "1", 1); + sc.update(); + EXPECT_EQ(sc.getSamplingState("1").getMultiplicity(), 1); +} + +// Test with 1 root span per minute +TEST(SamplingControllerTest, TestWithOneAllowedSpan) { + auto scf = std::make_unique(1); + SamplingController sc(std::move(scf)); + sc.update(); + EXPECT_EQ(sc.getSamplingState("1").getExponent(), SamplingController::MAX_SAMPLING_EXPONENT); + offerEntry(sc, "1", 1); + EXPECT_EQ(sc.getSamplingState("1").getExponent(), SamplingController::MAX_SAMPLING_EXPONENT); + sc.update(); + EXPECT_EQ(sc.getSamplingState("1").getMultiplicity(), 1); +} + +// Test with StreamSummary size not exceeded +TEST(SamplingControllerTest, TestStreamSummarySizeNotExceeded) { + auto scf = std::make_unique(); + SamplingController sc(std::move(scf)); + + offerEntry(sc, "1", 8600); + offerEntry(sc, "2", 5000); + offerEntry(sc, "3", 4000); + offerEntry(sc, "4", 4000); + offerEntry(sc, "5", 3000); + offerEntry(sc, "6", 30); + offerEntry(sc, "7", 3); + offerEntry(sc, "8", 1); + + sc.update(); + + EXPECT_EQ(sc.getEffectiveCount(), 1074); + EXPECT_EQ(sc.getSamplingState("1").getMultiplicity(), 64); + EXPECT_EQ(sc.getSamplingState("2").getMultiplicity(), 32); + EXPECT_EQ(sc.getSamplingState("3").getMultiplicity(), 32); + EXPECT_EQ(sc.getSamplingState("4").getMultiplicity(), 16); + EXPECT_EQ(sc.getSamplingState("5").getMultiplicity(), 8); + EXPECT_EQ(sc.getSamplingState("6").getMultiplicity(), 1); + EXPECT_EQ(sc.getSamplingState("7").getMultiplicity(), 1); + EXPECT_EQ(sc.getSamplingState("8").getMultiplicity(), 1); +} + +// Test with StreamSummary size not exceeded +TEST(SamplingControllerTest, TestStreamSummarySizeNotExceeded1) { + auto scf = std::make_unique(); + SamplingController sc(std::move(scf)); + + offerEntry(sc, "1", 7500); + offerEntry(sc, "2", 1000); + offerEntry(sc, "3", 1); + offerEntry(sc, "4", 1); + offerEntry(sc, "5", 1); + for (int64_t i = 0; i < 11; i++) { + sc.offer(std::to_string(i + 1000000)); + } + + sc.update(); + + EXPECT_EQ(sc.getEffectiveCount(), 1451); + EXPECT_EQ(sc.getSamplingState("1").getMultiplicity(), 8); + EXPECT_EQ(sc.getSamplingState("2").getMultiplicity(), 2); + EXPECT_EQ(sc.getSamplingState("3").getMultiplicity(), 1); + EXPECT_EQ(sc.getSamplingState("4").getMultiplicity(), 1); + EXPECT_EQ(sc.getSamplingState("5").getMultiplicity(), 1); + EXPECT_EQ(sc.getSamplingState("1000000").getMultiplicity(), 1); + EXPECT_EQ(sc.getSamplingState("1000001").getMultiplicity(), 1); + EXPECT_EQ(sc.getSamplingState("1000002").getMultiplicity(), 1); + EXPECT_EQ(sc.getSamplingState("1000003").getMultiplicity(), 1); +} + +// Test using a sampler config having non-default root spans per minute +TEST(SamplingControllerTest, TestNonDefaultRootSpansPerMinute) { + auto scf = std::make_unique(); + scf->config.parse("{\n \"rootSpansPerMinute\" : 100 \n }"); + SamplingController sc(std::move(scf)); + + offerEntry(sc, "GET_xxxx", 300); + offerEntry(sc, "POST_asdf", 200); + offerEntry(sc, "GET_asdf", 100); + + sc.update(); + + EXPECT_EQ(sc.getSamplingState("GET_xxxx").getExponent(), 3); + EXPECT_EQ(sc.getSamplingState("GET_xxxx").getMultiplicity(), 8); + + EXPECT_EQ(sc.getSamplingState("POST_asdf").getExponent(), 2); + EXPECT_EQ(sc.getSamplingState("POST_asdf").getMultiplicity(), 4); + + EXPECT_EQ(sc.getSamplingState("GET_asdf").getExponent(), 1); + EXPECT_EQ(sc.getSamplingState("GET_asdf").getMultiplicity(), 2); +} + +// Test warm up phase (no SamplingState available) +TEST(SamplingControllerTest, TestWarmup) { + auto scf = std::make_unique(); + SamplingController sc(std::move(scf)); + + // offer entries, but don't call update(); + // sampling exponents table will be empty + // exponent will be calculated based on total count. + // same exponent for both existing and non-existing keys. + + offerEntry(sc, "GET_0", 10); + EXPECT_EQ(sc.getSamplingState("GET_0").getExponent(), 0); + EXPECT_EQ(sc.getSamplingState("GET_1").getExponent(), 0); + EXPECT_EQ(sc.getSamplingState("GET_2").getExponent(), 0); + + offerEntry(sc, "GET_1", 540); + // threshold/2 reached, sampling exponent is set to 1 + EXPECT_EQ(sc.getSamplingState("GET_1").getExponent(), 1); + EXPECT_EQ(sc.getSamplingState("GET_2").getExponent(), 1); + EXPECT_EQ(sc.getSamplingState("GET_3").getExponent(), 1); + + offerEntry(sc, "GET_2", 300); + EXPECT_EQ(sc.getSamplingState("GET_1").getExponent(), 1); + EXPECT_EQ(sc.getSamplingState("GET_2").getExponent(), 1); + EXPECT_EQ(sc.getSamplingState("GET_123").getExponent(), 1); + + offerEntry(sc, "GET_4", 550); + EXPECT_EQ(sc.getSamplingState("GET_1").getExponent(), 2); + EXPECT_EQ(sc.getSamplingState("GET_4").getExponent(), 2); + EXPECT_EQ(sc.getSamplingState("GET_234").getExponent(), 2); + + offerEntry(sc, "GET_5", 1000); + EXPECT_EQ(sc.getSamplingState("GET_1").getExponent(), 4); + EXPECT_EQ(sc.getSamplingState("GET_5").getExponent(), 4); + EXPECT_EQ(sc.getSamplingState("GET_456").getExponent(), 4); + + offerEntry(sc, "GET_6", 2000); + EXPECT_EQ(sc.getSamplingState("GET_1").getExponent(), 8); + EXPECT_EQ(sc.getSamplingState("GET_6").getExponent(), 8); + EXPECT_EQ(sc.getSamplingState("GET_789").getExponent(), 8); +} + +// Test getting sampling state from an empty SamplingController +TEST(SamplingControllerTest, TestEmpty) { + auto scf = std::make_unique(); + SamplingController sc(std::move(scf)); + + sc.update(); + // default SamplingState is expected + EXPECT_EQ(sc.getSamplingState("GET_something").getExponent(), 0); + EXPECT_EQ(sc.getSamplingState("GET_something").getMultiplicity(), 1); +} + +// Test getting sampling state for an unknown key from a non-empty SamplingController +TEST(SamplingControllerTest, TestUnknown) { + auto scf = std::make_unique(); + SamplingController sc(std::move(scf)); + + sc.offer("key1"); + sc.update(); + + EXPECT_EQ(sc.getSamplingState("key2").getExponent(), 0); + EXPECT_EQ(sc.getSamplingState("key2").getMultiplicity(), 1); + + // Exceed capacity, + for (uint32_t i = 0; i < SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT * 2; i++) { + sc.offer("key1"); + } + sc.update(); + // "key1" will get exponent 1 + EXPECT_EQ(sc.getSamplingState("key1").getExponent(), 1); + // unknown "key2" will get the same exponent + EXPECT_EQ(sc.getSamplingState("key2").getExponent(), 1); +} + +// Test increasing and decreasing sampling exponent +TEST(SamplingStateTest, TestIncreaseDecrease) { + SamplingState sst{}; + EXPECT_EQ(sst.getExponent(), 0); + EXPECT_EQ(sst.getMultiplicity(), 1); + + sst.increaseExponent(); + EXPECT_EQ(sst.getExponent(), 1); + EXPECT_EQ(sst.getMultiplicity(), 2); + + sst.increaseExponent(); + EXPECT_EQ(sst.getExponent(), 2); + EXPECT_EQ(sst.getMultiplicity(), 4); + + for (int i = 0; i < 6; i++) { + sst.increaseExponent(); + } + EXPECT_EQ(sst.getExponent(), 8); + EXPECT_EQ(sst.getMultiplicity(), 256); + + sst.decreaseExponent(); + EXPECT_EQ(sst.getExponent(), 7); + EXPECT_EQ(sst.getMultiplicity(), 128); +} + +// Test SamplingState shouldSample() +TEST(SamplingStateTest, TestShouldSample) { + // default sampling state should sample every request + SamplingState sst{}; + EXPECT_TRUE(sst.shouldSample(1234)); + EXPECT_TRUE(sst.shouldSample(2345)); + EXPECT_TRUE(sst.shouldSample(3456)); + EXPECT_TRUE(sst.shouldSample(4567)); + + // exponent 1, multiplicity 2, + // we are using % for sampling decision, so even (=not odd) random numbers should be sampled + sst.increaseExponent(); + EXPECT_TRUE(sst.shouldSample(22)); + EXPECT_TRUE(sst.shouldSample(4444444)); + EXPECT_FALSE(sst.shouldSample(21)); + EXPECT_FALSE(sst.shouldSample(111111)); + + for (int i = 0; i < 9; i++) { + sst.increaseExponent(); + } + // exponent 10, multiplicity 1024, + EXPECT_TRUE(sst.shouldSample(1024)); + EXPECT_TRUE(sst.shouldSample(2048)); + EXPECT_TRUE(sst.shouldSample(4096)); + EXPECT_TRUE(sst.shouldSample(10240000000)); + EXPECT_FALSE(sst.shouldSample(1023)); + EXPECT_FALSE(sst.shouldSample(1025)); + EXPECT_FALSE(sst.shouldSample(2047)); + EXPECT_FALSE(sst.shouldSample(2049)); +} + +// Test creating sampling key used to identify a request +TEST(SamplingControllerTest, TestGetSamplingKey) { + std::string key = SamplingController::getSamplingKey("somepath", "GET"); + EXPECT_STREQ(key.c_str(), "GET_somepath"); + + key = SamplingController::getSamplingKey("somepath?withquery", "POST"); + EXPECT_STREQ(key.c_str(), "POST_somepath"); + + key = SamplingController::getSamplingKey("anotherpath", "PUT"); + EXPECT_STREQ(key.c_str(), "PUT_anotherpath"); + + key = SamplingController::getSamplingKey("", "PUT"); + EXPECT_STREQ(key.c_str(), "PUT_"); + + key = SamplingController::getSamplingKey("anotherpath", ""); + EXPECT_STREQ(key.c_str(), "_anotherpath"); +} + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/tracers/opentelemetry/samplers/dynatrace/stream_summary_test.cc b/test/extensions/tracers/opentelemetry/samplers/dynatrace/stream_summary_test.cc new file mode 100644 index 000000000000..f7f80cf68b93 --- /dev/null +++ b/test/extensions/tracers/opentelemetry/samplers/dynatrace/stream_summary_test.cc @@ -0,0 +1,148 @@ +#include +#include +#include +#include +#include +#include +#include + +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/stream_summary.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +namespace { + +// helper function to compare counters +template +void compareCounter(typename std::list>::iterator counter, T item, uint64_t value, + uint64_t error, int line_num) { + SCOPED_TRACE(absl::StrCat(__FUNCTION__, " called from line ", line_num)); + EXPECT_EQ(counter->getValue(), value); + EXPECT_EQ(counter->getItem(), item); + EXPECT_EQ(counter->getError(), error); +} + +} // namespace + +// Test an empty StreamSummary +TEST(StreamSummaryTest, TestEmpty) { + StreamSummary summary(4); + EXPECT_EQ(summary.getN(), 0); + auto top_k = summary.getTopK(); + EXPECT_EQ(top_k.size(), 0); + EXPECT_EQ(top_k.begin(), top_k.end()); + EXPECT_TRUE(summary.validate().ok()); +} + +// Test adding values, capacity not exceeded +TEST(StreamSummaryTest, TestSimple) { + StreamSummary summary(4); + summary.offer('a'); + summary.offer('a'); + summary.offer('b'); + summary.offer('a'); + summary.offer('c'); + summary.offer('b'); + summary.offer('a'); + summary.offer('d'); + + EXPECT_TRUE(summary.validate().ok()); + EXPECT_EQ(summary.getN(), 8); + + auto top_k = summary.getTopK(); + EXPECT_EQ(top_k.size(), 4); + auto it = top_k.begin(); + compareCounter(it, 'a', 4, 0, __LINE__); + compareCounter(++it, 'b', 2, 0, __LINE__); + compareCounter(++it, 'c', 1, 0, __LINE__); + compareCounter(++it, 'd', 1, 0, __LINE__); +} + +// Test adding values, capacity exceeded +TEST(StreamSummaryTest, TestExceedCapacity) { + StreamSummary summary(3); + EXPECT_TRUE(summary.validate().ok()); + summary.offer('d'); + summary.offer('a'); + summary.offer('b'); + summary.offer('a'); + summary.offer('a'); + summary.offer('a'); + summary.offer('b'); + summary.offer('c'); // 'd' will be dropped + summary.offer('b'); + summary.offer('c'); + EXPECT_TRUE(summary.validate().ok()); + + { + auto top_k = summary.getTopK(); + auto it = top_k.begin(); + EXPECT_EQ(top_k.size(), 3); + compareCounter(it, 'a', 4, 0, __LINE__); + compareCounter(++it, 'b', 3, 0, __LINE__); + compareCounter(++it, 'c', 3, 1, __LINE__); + } + + // add item 'e', 'c' should be removed. value for 'c' will be added to error for 'e' + summary.offer('e'); + { + auto top_k = summary.getTopK(); + auto it = top_k.begin(); + EXPECT_EQ(top_k.size(), 3); + compareCounter(it, 'a', 4, 0, __LINE__); + compareCounter(++it, 'e', 4, 3, __LINE__); + compareCounter(++it, 'b', 3, 0, __LINE__); + } +} + +// Test inserting items in random order. topK should not depend on the insert order. +TEST(StreamSummaryTest, TestRandomInsertOrder) { + std::vector v{'a', 'a', 'a', 'a', 'a', 'a', 'b', 'b', 'b', 'b', 'b', + 'c', 'c', 'c', 'c', 'd', 'd', 'd', 'e', 'e', 'f'}; + for (int i = 0; i < 5; ++i) { + // insert order should not matter if all items have a different count in input stream + std::shuffle(v.begin(), v.end(), std::default_random_engine()); + StreamSummary summary(10); + for (auto const c : v) { + summary.offer(c); + } + auto top_k = summary.getTopK(); + auto it = top_k.begin(); + compareCounter(it, 'a', 6, 0, __LINE__); + compareCounter(++it, 'b', 5, 0, __LINE__); + compareCounter(++it, 'c', 4, 0, __LINE__); + compareCounter(++it, 'd', 3, 0, __LINE__); + compareCounter(++it, 'e', 2, 0, __LINE__); + compareCounter(++it, 'f', 1, 0, __LINE__); + } +} + +// Test getTopK size parameter is handled as expected +TEST(StreamSummaryTest, TestGetTopKSize) { + std::vector v{'a', 'a', 'a', 'a', 'a', 'a', 'b', 'b', 'b', 'b', 'b', + 'c', 'c', 'c', 'c', 'd', 'd', 'd', 'e', 'e', 'f'}; + std::shuffle(v.begin(), v.end(), std::default_random_engine()); + StreamSummary summary(20); + for (auto const c : v) { + summary.offer(c); + } + EXPECT_EQ(summary.getTopK().size(), 6); + EXPECT_EQ(summary.getTopK(1).size(), 1); + EXPECT_EQ(summary.getTopK(2).size(), 2); + EXPECT_EQ(summary.getTopK(3).size(), 3); + EXPECT_EQ(summary.getTopK(4).size(), 4); + EXPECT_EQ(summary.getTopK(5).size(), 5); + EXPECT_EQ(summary.getTopK(6).size(), 6); + EXPECT_EQ(summary.getTopK(7).size(), 6); +} + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/tracers/opentelemetry/samplers/dynatrace/tenant_id_test.cc b/test/extensions/tracers/opentelemetry/samplers/dynatrace/tenant_id_test.cc new file mode 100644 index 000000000000..17f9d47d6d38 --- /dev/null +++ b/test/extensions/tracers/opentelemetry/samplers/dynatrace/tenant_id_test.cc @@ -0,0 +1,31 @@ +#include "source/extensions/tracers/opentelemetry/samplers/dynatrace/tenant_id.h" + +#include "absl/strings/str_cat.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +// Test calculation using an empty string +TEST(TenantIdTest, TestEmpty) { EXPECT_EQ(absl::StrCat(calculateTenantId("")), "0"); } + +// Test calculation using strings with unexpected characters +TEST(TenantIdTest, TestUnexpected) { + EXPECT_EQ(absl::StrCat(calculateTenantId("abc 1234")), "182ccac"); + EXPECT_EQ(absl::StrCat(calculateTenantId(" ")), "b173ef2e"); + EXPECT_EQ(absl::StrCat(calculateTenantId("€someth")), "17bd71ec"); +} + +// Test calculation using some expected strings +TEST(TenantIdTest, TestValues) { + EXPECT_EQ(absl::StrCat(calculateTenantId("jmw13303")), "4d10bede"); + EXPECT_EQ(absl::StrCat(calculateTenantId("abc12345")), "5b3f9fed"); + EXPECT_EQ(absl::StrCat(calculateTenantId("?pfel")), "7712d29d"); +} + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc b/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc index 687e63617050..ee5a93679d4e 100644 --- a/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc @@ -56,6 +56,12 @@ class SamplerFactoryTest : public testing::Test { NiceMock context; }; +// Test tracer category() +TEST_F(SamplerFactoryTest, TestGetName) { + TestSamplerFactory factory; + EXPECT_STREQ(factory.category().c_str(), "envoy.tracers.opentelemetry.samplers"); +} + // Test OTLP tracer without a sampler TEST_F(SamplerFactoryTest, TestWithoutSampler) { // using StrictMock, calls to SamplerFactory would cause a test failure diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 37e1296ac3d3..7f66087cef02 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -1324,6 +1324,7 @@ suf superclass superroot superset +supportability svc symlink symlinked From 44d4d8291708dca34e953d77d4b4b9382571870c Mon Sep 17 00:00:00 2001 From: Zhewei Hu Date: Thu, 29 Feb 2024 08:57:19 -0800 Subject: [PATCH 09/12] [ZK filter] Add missing operation supports for multi opcode (#32456) Signed-off-by: Zhewei Hu --- .../network/zookeeper_proxy/decoder.cc | 30 ++++++-- .../network/zookeeper_proxy/filter_test.cc | 72 ++++++++++++------- 2 files changed, 70 insertions(+), 32 deletions(-) diff --git a/source/extensions/filters/network/zookeeper_proxy/decoder.cc b/source/extensions/filters/network/zookeeper_proxy/decoder.cc index ec5edd68ea0c..58303e75cd31 100644 --- a/source/extensions/filters/network/zookeeper_proxy/decoder.cc +++ b/source/extensions/filters/network/zookeeper_proxy/decoder.cc @@ -137,8 +137,11 @@ absl::StatusOr> DecoderImpl::decodeOnData(Buffer::Instan status, fmt::format("parseGetDataRequest: {}", status.message())); break; case OpCodes::Create: + ABSL_FALLTHROUGH_INTENDED; case OpCodes::Create2: + ABSL_FALLTHROUGH_INTENDED; case OpCodes::CreateContainer: + ABSL_FALLTHROUGH_INTENDED; case OpCodes::CreateTtl: status = parseCreateRequest(data, offset, len.value(), opcode); RETURN_INVALID_ARG_ERR_IF_STATUS_NOT_OK( @@ -642,22 +645,37 @@ absl::Status DecoderImpl::parseMultiRequest(Buffer::Instance& data, uint64_t& of break; } - switch (static_cast(opcode.value())) { + const auto op = static_cast(opcode.value()); + switch (op) { case OpCodes::Create: - status = parseCreateRequest(data, offset, len, OpCodes::Create); - EMIT_DECODER_ERR_AND_RETURN_IF_STATUS_NOT_OK(status, OpCodes::Create); + ABSL_FALLTHROUGH_INTENDED; + case OpCodes::Create2: + ABSL_FALLTHROUGH_INTENDED; + case OpCodes::CreateContainer: + ABSL_FALLTHROUGH_INTENDED; + case OpCodes::CreateTtl: + status = parseCreateRequest(data, offset, len, op); + EMIT_DECODER_ERR_AND_RETURN_IF_STATUS_NOT_OK(status, op); break; case OpCodes::SetData: status = parseSetRequest(data, offset, len); - EMIT_DECODER_ERR_AND_RETURN_IF_STATUS_NOT_OK(status, OpCodes::SetData); + EMIT_DECODER_ERR_AND_RETURN_IF_STATUS_NOT_OK(status, op); break; case OpCodes::Check: status = parseCheckRequest(data, offset, len); - EMIT_DECODER_ERR_AND_RETURN_IF_STATUS_NOT_OK(status, OpCodes::Check); + EMIT_DECODER_ERR_AND_RETURN_IF_STATUS_NOT_OK(status, op); break; case OpCodes::Delete: status = parseDeleteRequest(data, offset, len); - EMIT_DECODER_ERR_AND_RETURN_IF_STATUS_NOT_OK(status, OpCodes::Delete); + EMIT_DECODER_ERR_AND_RETURN_IF_STATUS_NOT_OK(status, op); + break; + case OpCodes::GetChildren: + status = parseGetChildrenRequest(data, offset, len, false); + EMIT_DECODER_ERR_AND_RETURN_IF_STATUS_NOT_OK(status, op); + break; + case OpCodes::GetData: + status = parseGetDataRequest(data, offset, len); + EMIT_DECODER_ERR_AND_RETURN_IF_STATUS_NOT_OK(status, op); break; default: callbacks_.onDecodeError(absl::nullopt); diff --git a/test/extensions/filters/network/zookeeper_proxy/filter_test.cc b/test/extensions/filters/network/zookeeper_proxy/filter_test.cc index 72f8bba27954..823f1e43a721 100644 --- a/test/extensions/filters/network/zookeeper_proxy/filter_test.cc +++ b/test/extensions/filters/network/zookeeper_proxy/filter_test.cc @@ -223,15 +223,18 @@ class ZooKeeperFilterTest : public testing::Test { return buffer; } - Buffer::OwnedImpl - encodePathWatch(const std::string& path, const bool watch, const int32_t xid = 1000, - const int32_t opcode = enumToSignedInt(OpCodes::GetData)) const { + Buffer::OwnedImpl encodePathWatch(const std::string& path, const bool watch, + const int32_t xid = 1000, + const int32_t opcode = enumToSignedInt(OpCodes::GetData), + const bool txn = false) const { Buffer::OwnedImpl buffer; - buffer.writeBEInt(13 + path.length()); - buffer.writeBEInt(xid); - // Opcode. - buffer.writeBEInt(opcode); + if (!txn) { + buffer.writeBEInt(13 + path.length()); + buffer.writeBEInt(xid); + buffer.writeBEInt(opcode); + } + // Path. addString(buffer, path); // Watch. @@ -311,7 +314,7 @@ class ZooKeeperFilterTest : public testing::Test { return buffer; } - Buffer::OwnedImpl encodeCreateRequestWithNegativeDataLen( + Buffer::OwnedImpl encodeCreateRequestWithoutZnodeData( const std::string& path, const int32_t create_flag_val, const bool txn = false, const int32_t opcode = enumToSignedInt(OpCodes::Create)) const { Buffer::OwnedImpl buffer; @@ -596,11 +599,11 @@ class ZooKeeperFilterTest : public testing::Test { store_.counter(absl::StrCat("test.zookeeper.", opname, "_decoder_error")).value()); } - void testCreateWithNegativeDataLen(CreateFlags flag, const int32_t flag_val, - const OpCodes opcode = OpCodes::Create) { + void testCreateWithoutZnodeData(CreateFlags flag, const int32_t flag_val, + const OpCodes opcode = OpCodes::Create) { initialize(); Buffer::OwnedImpl data = - encodeCreateRequestWithNegativeDataLen("/foo", flag_val, false, enumToSignedInt(opcode)); + encodeCreateRequestWithoutZnodeData("/foo", flag_val, false, enumToSignedInt(opcode)); std::string opname = "create"; switch (opcode) { @@ -1088,8 +1091,8 @@ TEST_F(ZooKeeperFilterTest, CreateRequestPersistent) { testResponse({{{"opname", "create_resp"}, {"zxid", "2000"}, {"error", "0"}}, {{"bytes", "20"}}}); } -TEST_F(ZooKeeperFilterTest, CreateRequestPersistentWithNegativeDataLen) { - testCreateWithNegativeDataLen(CreateFlags::Persistent, 0); +TEST_F(ZooKeeperFilterTest, CreateRequestPersistentWithoutZnodeData) { + testCreateWithoutZnodeData(CreateFlags::Persistent, 0); testResponse({{{"opname", "create_resp"}, {"zxid", "2000"}, {"error", "0"}}, {{"bytes", "20"}}}); } @@ -1283,33 +1286,50 @@ TEST_F(ZooKeeperFilterTest, CheckRequest) { TEST_F(ZooKeeperFilterTest, MultiRequest) { initialize(); - Buffer::OwnedImpl create1 = encodeCreateRequest("/foo", "1", 0, true); - Buffer::OwnedImpl create2 = encodeCreateRequest("/bar", "1", 0, true); - Buffer::OwnedImpl create3 = encodeCreateRequestWithNegativeDataLen("/baz", 0, true); - Buffer::OwnedImpl check1 = encodePathVersion("/foo", 100, enumToSignedInt(OpCodes::Check), true); - Buffer::OwnedImpl set1 = encodeSetRequest("/bar", "2", -1, true); + Buffer::OwnedImpl create1 = + encodeCreateRequest("/foo", "1", 0, true, 1000, enumToSignedInt(OpCodes::Create)); + Buffer::OwnedImpl create2 = + encodeCreateRequest("/bar", "1", 0, true, 1000, enumToSignedInt(OpCodes::Create2)); + Buffer::OwnedImpl create3 = + encodeCreateRequest("/baz", "1", 0, true, 1000, enumToSignedInt(OpCodes::CreateContainer)); + Buffer::OwnedImpl create4 = + encodeCreateRequestWithoutZnodeData("/qux", 0, true, enumToSignedInt(OpCodes::CreateTtl)); + Buffer::OwnedImpl check = encodePathVersion("/foo", 100, enumToSignedInt(OpCodes::Check), true); + Buffer::OwnedImpl set = encodeSetRequest("/bar", "2", -1, true); Buffer::OwnedImpl delete1 = encodeDeleteRequest("/abcd", 1, true); Buffer::OwnedImpl delete2 = encodeDeleteRequest("/efg", 2, true); + Buffer::OwnedImpl getchildren = + encodePathWatch("/foo", false, 1000, enumToSignedInt(OpCodes::GetChildren), true); + Buffer::OwnedImpl getdata = + encodePathWatch("/bar", true, 1000, enumToSignedInt(OpCodes::GetData), true); std::vector> ops; ops.push_back(std::make_pair(enumToSignedInt(OpCodes::Create), std::move(create1))); - ops.push_back(std::make_pair(enumToSignedInt(OpCodes::Create), std::move(create2))); - ops.push_back(std::make_pair(enumToSignedInt(OpCodes::Create), std::move(create3))); - ops.push_back(std::make_pair(enumToSignedInt(OpCodes::Check), std::move(check1))); - ops.push_back(std::make_pair(enumToSignedInt(OpCodes::SetData), std::move(set1))); + ops.push_back(std::make_pair(enumToSignedInt(OpCodes::Create2), std::move(create2))); + ops.push_back(std::make_pair(enumToSignedInt(OpCodes::CreateContainer), std::move(create3))); + ops.push_back(std::make_pair(enumToSignedInt(OpCodes::CreateTtl), std::move(create4))); + ops.push_back(std::make_pair(enumToSignedInt(OpCodes::Check), std::move(check))); + ops.push_back(std::make_pair(enumToSignedInt(OpCodes::SetData), std::move(set))); ops.push_back(std::make_pair(enumToSignedInt(OpCodes::Delete), std::move(delete1))); ops.push_back(std::make_pair(enumToSignedInt(OpCodes::Delete), std::move(delete2))); + ops.push_back(std::make_pair(enumToSignedInt(OpCodes::GetChildren), std::move(getchildren))); + ops.push_back(std::make_pair(enumToSignedInt(OpCodes::GetData), std::move(getdata))); Buffer::OwnedImpl data = encodeMultiRequest(ops); EXPECT_EQ(Envoy::Network::FilterStatus::Continue, filter_->onData(data, false)); EXPECT_EQ(1UL, config_->stats().multi_rq_.value()); - EXPECT_EQ(200UL, config_->stats().request_bytes_.value()); - EXPECT_EQ(200UL, config_->stats().multi_rq_bytes_.value()); - EXPECT_EQ(3UL, config_->stats().create_rq_.value()); - EXPECT_EQ(1UL, config_->stats().setdata_rq_.value()); + EXPECT_EQ(266UL, config_->stats().request_bytes_.value()); + EXPECT_EQ(266UL, config_->stats().multi_rq_bytes_.value()); + EXPECT_EQ(1UL, config_->stats().create_rq_.value()); + EXPECT_EQ(1UL, config_->stats().create2_rq_.value()); + EXPECT_EQ(1UL, config_->stats().createcontainer_rq_.value()); + EXPECT_EQ(1UL, config_->stats().createttl_rq_.value()); EXPECT_EQ(1UL, config_->stats().check_rq_.value()); + EXPECT_EQ(1UL, config_->stats().setdata_rq_.value()); EXPECT_EQ(2UL, config_->stats().delete_rq_.value()); + EXPECT_EQ(1UL, config_->stats().getchildren_rq_.value()); + EXPECT_EQ(1UL, config_->stats().getdata_rq_.value()); EXPECT_EQ(0UL, config_->stats().decoder_error_.value()); EXPECT_EQ(0UL, config_->stats().multi_decoder_error_.value()); From 88cc3025c7fa23725029e1b7cea76910c7e86a80 Mon Sep 17 00:00:00 2001 From: Kuat Date: Thu, 29 Feb 2024 09:26:10 -0800 Subject: [PATCH 10/12] oauth2: refactor SDS secret provider to remove a data race (#32625) Commit Message: Refactor SDS secret provider to use TLV for values and make it a common utility to prevent code duplication. Additional Description: Risk Level: low Testing: done Docs Changes: none Release Notes: none Fixes: #21273 Signed-off-by: Kuat Yessenov --- contrib/sxg/filters/http/source/BUILD | 1 + contrib/sxg/filters/http/source/config.cc | 3 +- .../sxg/filters/http/source/filter_config.h | 41 +++++------------- contrib/sxg/filters/http/test/filter_test.cc | 4 +- source/common/secret/BUILD | 3 ++ source/common/secret/secret_provider_impl.cc | 29 +++++++++++++ source/common/secret/secret_provider_impl.h | 25 +++++++++++ source/extensions/filters/http/oauth2/BUILD | 2 +- .../extensions/filters/http/oauth2/config.cc | 6 +-- .../extensions/filters/http/oauth2/filter.h | 42 +++++-------------- .../filters/http/oauth2/filter_test.cc | 4 +- tools/code_format/config.yaml | 1 + 12 files changed, 91 insertions(+), 70 deletions(-) diff --git a/contrib/sxg/filters/http/source/BUILD b/contrib/sxg/filters/http/source/BUILD index a6bb7cec6708..1d3b5b9231a6 100644 --- a/contrib/sxg/filters/http/source/BUILD +++ b/contrib/sxg/filters/http/source/BUILD @@ -28,6 +28,7 @@ envoy_cc_library( "//source/common/http:codes_lib", "//source/common/stats:symbol_table_lib", "//source/common/stats:utility_lib", + "//source/common/secret:secret_provider_impl_lib", "//source/extensions/filters/http/common:pass_through_filter_lib", "@envoy_api//contrib/envoy/extensions/filters/http/sxg/v3alpha:pkg_cc_proto", # use boringssl alias to select fips vs non-fips version. diff --git a/contrib/sxg/filters/http/source/config.cc b/contrib/sxg/filters/http/source/config.cc index 68b7d2292a72..541216f8454f 100644 --- a/contrib/sxg/filters/http/source/config.cc +++ b/contrib/sxg/filters/http/source/config.cc @@ -57,7 +57,8 @@ Http::FilterFactoryCb FilterFactory::createFilterFactoryFromProtoTyped( } auto secret_reader = std::make_shared( - secret_provider_certificate, secret_provider_private_key, server_context.api()); + std::move(secret_provider_certificate), std::move(secret_provider_private_key), + server_context.threadLocal(), server_context.api()); auto config = std::make_shared(proto_config, server_context.timeSource(), secret_reader, stat_prefix, context.scope()); return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { diff --git a/contrib/sxg/filters/http/source/filter_config.h b/contrib/sxg/filters/http/source/filter_config.h index ca3cbfdb5300..a54f51794259 100644 --- a/contrib/sxg/filters/http/source/filter_config.h +++ b/contrib/sxg/filters/http/source/filter_config.h @@ -4,7 +4,7 @@ #include "envoy/stats/scope.h" #include "envoy/stats/stats_macros.h" -#include "source/common/config/datasource.h" +#include "source/common/secret/secret_provider_impl.h" #include "source/extensions/filters/http/common/pass_through_filter.h" #include "contrib/envoy/extensions/filters/http/sxg/v3alpha/sxg.pb.h" @@ -37,39 +37,18 @@ class SecretReader { class SDSSecretReader : public SecretReader { public: - SDSSecretReader(Secret::GenericSecretConfigProviderSharedPtr certificate_provider, - Secret::GenericSecretConfigProviderSharedPtr private_key_provider, Api::Api& api) - : update_callback_client_(readAndWatchSecret(certificate_, certificate_provider, api)), - update_callback_token_(readAndWatchSecret(private_key_, private_key_provider, api)) {} - + SDSSecretReader(Secret::GenericSecretConfigProviderSharedPtr&& certificate_provider, + Secret::GenericSecretConfigProviderSharedPtr&& private_key_provider, + ThreadLocal::SlotAllocator& tls, Api::Api& api) + : certificate_(std::move(certificate_provider), tls, api), + private_key_(std::move(private_key_provider), tls, api) {} // SecretReader - const std::string& certificate() const override { return certificate_; } - const std::string& privateKey() const override { return private_key_; } + const std::string& certificate() const override { return certificate_.secret(); } + const std::string& privateKey() const override { return private_key_.secret(); } private: - Envoy::Common::CallbackHandlePtr - readAndWatchSecret(std::string& value, - Secret::GenericSecretConfigProviderSharedPtr& secret_provider, Api::Api& api) { - const auto* secret = secret_provider->secret(); - if (secret != nullptr) { - value = - THROW_OR_RETURN_VALUE(Config::DataSource::read(secret->secret(), true, api), std::string); - } - - return secret_provider->addUpdateCallback([secret_provider, &api, &value]() { - const auto* secret = secret_provider->secret(); - if (secret != nullptr) { - value = THROW_OR_RETURN_VALUE(Config::DataSource::read(secret->secret(), true, api), - std::string); - } - }); - } - - std::string certificate_; - std::string private_key_; - - Envoy::Common::CallbackHandlePtr update_callback_client_; - Envoy::Common::CallbackHandlePtr update_callback_token_; + Secret::ThreadLocalGenericSecretProvider certificate_; + Secret::ThreadLocalGenericSecretProvider private_key_; }; class FilterConfig : public Logger::Loggable { diff --git a/contrib/sxg/filters/http/test/filter_test.cc b/contrib/sxg/filters/http/test/filter_test.cc index 933198e6f6c7..a8ba5c027e18 100644 --- a/contrib/sxg/filters/http/test/filter_test.cc +++ b/contrib/sxg/filters/http/test/filter_test.cc @@ -380,7 +380,9 @@ TEST_F(FilterTest, SdsDynamicGenericSecret) { config_source, "private_key", secret_context, init_manager); auto private_key_callback = secret_context.cluster_manager_.subscription_factory_.callbacks_; - SDSSecretReader secret_reader(certificate_secret_provider, private_key_secret_provider, *api); + NiceMock tls; + SDSSecretReader secret_reader(std::move(certificate_secret_provider), + std::move(private_key_secret_provider), tls, *api); EXPECT_TRUE(secret_reader.certificate().empty()); EXPECT_TRUE(secret_reader.privateKey().empty()); diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index b7b25aedbe5b..d6e61c39531f 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -32,6 +32,9 @@ envoy_cc_library( hdrs = ["secret_provider_impl.h"], deps = [ "//envoy/secret:secret_provider_interface", + "//envoy/thread_local:thread_local_interface", + "//envoy/thread_local:thread_local_object", + "//source/common/config:datasource_lib", "//source/common/ssl:certificate_validation_context_config_impl_lib", "//source/common/ssl:tls_certificate_config_impl_lib", "@envoy_api//envoy/extensions/transport_sockets/tls/v3:pkg_cc_proto", diff --git a/source/common/secret/secret_provider_impl.cc b/source/common/secret/secret_provider_impl.cc index 6aed80d0107c..ea2ff427ec19 100644 --- a/source/common/secret/secret_provider_impl.cc +++ b/source/common/secret/secret_provider_impl.cc @@ -3,6 +3,7 @@ #include "envoy/extensions/transport_sockets/tls/v3/cert.pb.h" #include "source/common/common/assert.h" +#include "source/common/config/datasource.h" #include "source/common/ssl/certificate_validation_context_config_impl.h" #include "source/common/ssl/tls_certificate_config_impl.h" @@ -36,5 +37,33 @@ GenericSecretConfigProviderImpl::GenericSecretConfigProviderImpl( std::make_unique( generic_secret)) {} +ThreadLocalGenericSecretProvider::ThreadLocalGenericSecretProvider( + GenericSecretConfigProviderSharedPtr&& provider, ThreadLocal::SlotAllocator& tls, Api::Api& api) + : provider_(provider), api_(api), + tls_(std::make_unique>(tls)), + cb_(provider_->addUpdateCallback([this] { update(); })) { + std::string value; + if (const auto* secret = provider_->secret(); secret != nullptr) { + value = + THROW_OR_RETURN_VALUE(Config::DataSource::read(secret->secret(), true, api_), std::string); + } + tls_->set([value = std::move(value)](Event::Dispatcher&) { + return std::make_shared(value); + }); +} + +const std::string& ThreadLocalGenericSecretProvider::secret() const { return (*tls_)->value_; } + +// This function is executed on the main during xDS update and can throw. +void ThreadLocalGenericSecretProvider::update() { + std::string value; + if (const auto* secret = provider_->secret(); secret != nullptr) { + value = + THROW_OR_RETURN_VALUE(Config::DataSource::read(secret->secret(), true, api_), std::string); + } + tls_->runOnAllThreads( + [value = std::move(value)](OptRef tls) { tls->value_ = value; }); +} + } // namespace Secret } // namespace Envoy diff --git a/source/common/secret/secret_provider_impl.h b/source/common/secret/secret_provider_impl.h index 566f53bf2a86..981f1c84cf41 100644 --- a/source/common/secret/secret_provider_impl.h +++ b/source/common/secret/secret_provider_impl.h @@ -6,6 +6,8 @@ #include "envoy/secret/secret_provider.h" #include "envoy/ssl/certificate_validation_context_config.h" #include "envoy/ssl/tls_certificate_config.h" +#include "envoy/thread_local/thread_local.h" +#include "envoy/thread_local/thread_local_object.h" namespace Envoy { namespace Secret { @@ -108,5 +110,28 @@ class GenericSecretConfigProviderImpl : public GenericSecretConfigProvider { Secret::GenericSecretPtr generic_secret_; }; +/** + * A utility secret provider that uses thread local values to share the updates to the secrets from + * the main to the workers. + **/ +class ThreadLocalGenericSecretProvider { +public: + ThreadLocalGenericSecretProvider(GenericSecretConfigProviderSharedPtr&& provider, + ThreadLocal::SlotAllocator& tls, Api::Api& api); + const std::string& secret() const; + +private: + struct ThreadLocalSecret : public ThreadLocal::ThreadLocalObject { + explicit ThreadLocalSecret(const std::string& value) : value_(value) {} + std::string value_; + }; + void update(); + GenericSecretConfigProviderSharedPtr provider_; + Api::Api& api_; + ThreadLocal::TypedSlotPtr tls_; + // Must be last since it has a non-trivial de-registering destructor. + Common::CallbackHandlePtr cb_; +}; + } // namespace Secret } // namespace Envoy diff --git a/source/extensions/filters/http/oauth2/BUILD b/source/extensions/filters/http/oauth2/BUILD index 936579f59266..a236dfa7583f 100644 --- a/source/extensions/filters/http/oauth2/BUILD +++ b/source/extensions/filters/http/oauth2/BUILD @@ -49,10 +49,10 @@ envoy_cc_library( "//envoy/server:filter_config_interface", "//source/common/common:assert_lib", "//source/common/common:empty_string", - "//source/common/config:datasource_lib", "//source/common/crypto:utility_lib", "//source/common/formatter:substitution_formatter_lib", "//source/common/protobuf:utility_lib", + "//source/common/secret:secret_provider_impl_lib", "//source/extensions/filters/http/common:pass_through_filter_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/http/oauth2/v3:pkg_cc_proto", diff --git a/source/extensions/filters/http/oauth2/config.cc b/source/extensions/filters/http/oauth2/config.cc index 98fd1f7dd6d8..1a7878b92f26 100644 --- a/source/extensions/filters/http/oauth2/config.cc +++ b/source/extensions/filters/http/oauth2/config.cc @@ -64,9 +64,9 @@ Http::FilterFactoryCb OAuth2Config::createFilterFactoryFromProtoTyped( throw EnvoyException("invalid HMAC secret configuration"); } - auto secret_reader = - std::make_shared(secret_provider_token_secret, secret_provider_hmac_secret, - context.serverFactoryContext().api()); + auto secret_reader = std::make_shared( + std::move(secret_provider_token_secret), std::move(secret_provider_hmac_secret), + context.serverFactoryContext().threadLocal(), context.serverFactoryContext().api()); auto config = std::make_shared(proto_config, cluster_manager, secret_reader, context.scope(), stats_prefix); diff --git a/source/extensions/filters/http/oauth2/filter.h b/source/extensions/filters/http/oauth2/filter.h index 715cd3230cdd..d2a594241a35 100644 --- a/source/extensions/filters/http/oauth2/filter.h +++ b/source/extensions/filters/http/oauth2/filter.h @@ -16,11 +16,11 @@ #include "source/common/common/assert.h" #include "source/common/common/matchers.h" -#include "source/common/config/datasource.h" #include "source/common/formatter/substitution_formatter.h" #include "source/common/http/header_map_impl.h" #include "source/common/http/header_utility.h" #include "source/common/http/utility.h" +#include "source/common/secret/secret_provider_impl.h" #include "source/extensions/filters/http/common/pass_through_filter.h" #include "source/extensions/filters/http/oauth2/oauth.h" #include "source/extensions/filters/http/oauth2/oauth_client.h" @@ -45,39 +45,17 @@ class SecretReader { class SDSSecretReader : public SecretReader { public: - SDSSecretReader(Secret::GenericSecretConfigProviderSharedPtr client_secret_provider, - Secret::GenericSecretConfigProviderSharedPtr token_secret_provider, Api::Api& api) - : update_callback_client_(readAndWatchSecret(client_secret_, client_secret_provider, api)), - update_callback_token_(readAndWatchSecret(token_secret_, token_secret_provider, api)) {} - - const std::string& clientSecret() const override { return client_secret_; } - - const std::string& tokenSecret() const override { return token_secret_; } + SDSSecretReader(Secret::GenericSecretConfigProviderSharedPtr&& client_secret_provider, + Secret::GenericSecretConfigProviderSharedPtr&& token_secret_provider, + ThreadLocal::SlotAllocator& tls, Api::Api& api) + : client_secret_(std::move(client_secret_provider), tls, api), + token_secret_(std::move(token_secret_provider), tls, api) {} + const std::string& clientSecret() const override { return client_secret_.secret(); } + const std::string& tokenSecret() const override { return token_secret_.secret(); } private: - Envoy::Common::CallbackHandlePtr - readAndWatchSecret(std::string& value, - Secret::GenericSecretConfigProviderSharedPtr& secret_provider, Api::Api& api) { - const auto* secret = secret_provider->secret(); - if (secret != nullptr) { - value = - THROW_OR_RETURN_VALUE(Config::DataSource::read(secret->secret(), true, api), std::string); - } - - return secret_provider->addUpdateCallback([secret_provider, &api, &value]() { - const auto* secret = secret_provider->secret(); - if (secret != nullptr) { - value = THROW_OR_RETURN_VALUE(Config::DataSource::read(secret->secret(), true, api), - std::string); - } - }); - } - - std::string client_secret_; - std::string token_secret_; - - Envoy::Common::CallbackHandlePtr update_callback_client_; - Envoy::Common::CallbackHandlePtr update_callback_token_; + Secret::ThreadLocalGenericSecretProvider client_secret_; + Secret::ThreadLocalGenericSecretProvider token_secret_; }; /** diff --git a/test/extensions/filters/http/oauth2/filter_test.cc b/test/extensions/filters/http/oauth2/filter_test.cc index 3ac4819bc635..231f899fd611 100644 --- a/test/extensions/filters/http/oauth2/filter_test.cc +++ b/test/extensions/filters/http/oauth2/filter_test.cc @@ -247,7 +247,9 @@ TEST_F(OAuth2Test, SdsDynamicGenericSecret) { config_source, "token", secret_context, init_manager); auto token_callback = secret_context.cluster_manager_.subscription_factory_.callbacks_; - SDSSecretReader secret_reader(client_secret_provider, token_secret_provider, *api); + NiceMock tls; + SDSSecretReader secret_reader(std::move(client_secret_provider), std::move(token_secret_provider), + tls, *api); EXPECT_TRUE(secret_reader.clientSecret().empty()); EXPECT_TRUE(secret_reader.tokenSecret().empty()); diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml index 0e96847c58dd..7da3e054be32 100644 --- a/tools/code_format/config.yaml +++ b/tools/code_format/config.yaml @@ -162,6 +162,7 @@ paths: - source/common/upstream/health_discovery_service.cc - source/common/secret/sds_api.h - source/common/secret/sds_api.cc + - source/common/secret/secret_provider_impl.cc - source/common/router/router.cc - source/common/config/config_provider_impl.h - source/common/common/logger_delegates.cc From f7352b3237d5b16cf7e078733b32ba158e66d086 Mon Sep 17 00:00:00 2001 From: Raven Black Date: Thu, 29 Feb 2024 12:37:43 -0500 Subject: [PATCH 11/12] QUIC hot restart part 6 - child instance pauses listening until parent is drained (#31130) QUIC hot restart part 6 - child instance pauses listening until parent is drained (#31130) --------- Signed-off-by: Raven Black --- envoy/network/BUILD | 6 ++ .../parent_drained_callback_registrar.h | 29 +++++ envoy/network/socket.h | 7 ++ envoy/server/hot_restart.h | 11 ++ .../listener_manager/listener_manager_impl.cc | 5 +- source/common/network/BUILD | 1 + source/common/network/listen_socket_impl.h | 19 +++- source/common/network/udp_listener_impl.cc | 40 ++++++- source/common/network/udp_listener_impl.h | 6 ++ source/server/BUILD | 1 + source/server/hot_restart_impl.cc | 4 + source/server/hot_restart_impl.h | 1 + source/server/hot_restart_nop_impl.h | 3 + source/server/hot_restarting_child.cc | 53 ++++++--- source/server/hot_restarting_child.h | 17 ++- test/common/network/BUILD | 1 + .../udp_listener_impl_batch_writer_test.cc | 1 + test/common/network/udp_listener_impl_test.cc | 102 ++++++++++++++++++ .../network/udp_listener_impl_test_base.h | 27 ++++- .../python/hotrestart_handoff_test.py | 2 +- test/mocks/network/BUILD | 6 ++ .../mock_parent_drained_callback_registrar.h | 18 ++++ test/mocks/server/hot_restart.h | 2 + test/server/hot_restart_impl_test.cc | 8 ++ test/server/hot_restarting_child_test.cc | 35 ++++++ 25 files changed, 376 insertions(+), 29 deletions(-) create mode 100644 envoy/network/parent_drained_callback_registrar.h create mode 100644 test/mocks/network/mock_parent_drained_callback_registrar.h diff --git a/envoy/network/BUILD b/envoy/network/BUILD index 3e7d51a07e90..230f7c065d58 100644 --- a/envoy/network/BUILD +++ b/envoy/network/BUILD @@ -58,6 +58,12 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "parent_drained_callback_registrar_interface", + hdrs = ["parent_drained_callback_registrar.h"], + deps = [":address_interface"], +) + envoy_cc_library( name = "udp_packet_writer_handler_interface", hdrs = ["udp_packet_writer_handler.h"], diff --git a/envoy/network/parent_drained_callback_registrar.h b/envoy/network/parent_drained_callback_registrar.h new file mode 100644 index 000000000000..d0ce7c9a191e --- /dev/null +++ b/envoy/network/parent_drained_callback_registrar.h @@ -0,0 +1,29 @@ +#pragma once + +#include "envoy/network/address.h" + +#include "absl/functional/any_invocable.h" + +namespace Envoy { +namespace Network { + +/** + * An interface through which a UDP listen socket, especially a QUIC socket, can + * postpone reading during hot restart until the parent instance is drained. + */ +class ParentDrainedCallbackRegistrar { +public: + /** + * @param address is the address of the listener. + * @param callback the function to call when the listener matching address is + * drained on the parent instance. + */ + virtual void registerParentDrainedCallback(const Address::InstanceConstSharedPtr& address, + absl::AnyInvocable callback) PURE; + +protected: + virtual ~ParentDrainedCallbackRegistrar() = default; +}; + +} // namespace Network +} // namespace Envoy diff --git a/envoy/network/socket.h b/envoy/network/socket.h index b83d0f213047..6091cf9fe922 100644 --- a/envoy/network/socket.h +++ b/envoy/network/socket.h @@ -542,6 +542,13 @@ class Socket { * @return the socket options stored earlier with addOption() and addOptions() calls, if any. */ virtual const OptionsSharedPtr& options() const PURE; + + /** + * @return a ParentDrainedCallbackRegistrar for UDP listen sockets during hot restart. + */ + virtual OptRef parentDrainedCallbackRegistrar() const { + return absl::nullopt; + } }; using SocketPtr = std::unique_ptr; diff --git a/envoy/server/hot_restart.h b/envoy/server/hot_restart.h index a1ce1663cde4..8e201dd65e08 100644 --- a/envoy/server/hot_restart.h +++ b/envoy/server/hot_restart.h @@ -62,6 +62,17 @@ class HotRestart { virtual void registerUdpForwardingListener(Network::Address::InstanceConstSharedPtr address, std::shared_ptr listener_config) PURE; + + /** + * @return An interface on which registerParentDrainedCallback can be called during + * creation of a listener, or nullopt if there is no parent instance. + * + * If this is set, any UDP listener should start paused and only begin listening + * when the parent instance is drained; this allows draining QUIC listeners to + * catch their own packets and forward unrecognized packets to the child instance. + */ + virtual OptRef parentDrainedCallbackRegistrar() PURE; + /** * Initialize the parent logic of our restarter. Meant to be called after initialization of a * new child has begun. The hot restart implementation needs to be created early to deal with diff --git a/source/common/listener_manager/listener_manager_impl.cc b/source/common/listener_manager/listener_manager_impl.cc index 853087d0b1bf..930378a94b94 100644 --- a/source/common/listener_manager/listener_manager_impl.cc +++ b/source/common/listener_manager/listener_manager_impl.cc @@ -321,7 +321,10 @@ Network::SocketSharedPtr ProdListenerComponentFactory::createListenSocket( if (socket_type == Network::Socket::Type::Stream) { return std::make_shared(std::move(io_handle), address, options); } else { - return std::make_shared(std::move(io_handle), address, options); + auto socket = std::make_shared( + std::move(io_handle), address, options, + server_.hotRestart().parentDrainedCallbackRegistrar()); + return socket; } } } diff --git a/source/common/network/BUILD b/source/common/network/BUILD index c563ca21428b..53ca953779a4 100644 --- a/source/common/network/BUILD +++ b/source/common/network/BUILD @@ -344,6 +344,7 @@ envoy_cc_library( "//envoy/event:file_event_interface", "//envoy/network:exception_interface", "//envoy/network:listener_interface", + "//envoy/network:parent_drained_callback_registrar_interface", "//envoy/runtime:runtime_interface", "//envoy/stats:stats_interface", "//envoy/stats:stats_macros", diff --git a/source/common/network/listen_socket_impl.h b/source/common/network/listen_socket_impl.h index 1dd9a6f668d7..e17e3178637d 100644 --- a/source/common/network/listen_socket_impl.h +++ b/source/common/network/listen_socket_impl.h @@ -68,12 +68,19 @@ template class NetworkListenSocket : public ListenSocketImpl { } } - NetworkListenSocket(IoHandlePtr&& io_handle, const Address::InstanceConstSharedPtr& address, - const Network::Socket::OptionsSharedPtr& options) - : ListenSocketImpl(std::move(io_handle), address) { + NetworkListenSocket( + IoHandlePtr&& io_handle, const Address::InstanceConstSharedPtr& address, + const Network::Socket::OptionsSharedPtr& options, + OptRef parent_drained_callback_registrar = absl::nullopt) + : ListenSocketImpl(std::move(io_handle), address), + parent_drained_callback_registrar_(parent_drained_callback_registrar) { setListenSocketOptions(options); } + OptRef parentDrainedCallbackRegistrar() const override { + return parent_drained_callback_registrar_; + } + Socket::Type socketType() const override { return T::type; } SocketPtr duplicate() override { @@ -110,6 +117,12 @@ template class NetworkListenSocket : public ListenSocketImpl { } protected: + // Usually a socket when initialized starts listening for ready-to-read or ready-to-write events; + // for a QUIC socket during hot restart this is undesirable as the parent instance needs to + // receive all packets; in that case this interface is set, and listening won't begin until the + // callback is called. + OptRef parent_drained_callback_registrar_; + void setPrebindSocketOptions() { // On Windows, SO_REUSEADDR does not restrict subsequent bind calls when there is a listener as // on Linux and later BSD socket stacks. diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index 62c5b273db96..a184eaae7036 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -9,6 +9,7 @@ #include "envoy/common/platform.h" #include "envoy/config/core/v3/base.pb.h" #include "envoy/network/exception.h" +#include "envoy/network/parent_drained_callback_registrar.h" #include "source/common/api/os_sys_calls_impl.h" #include "source/common/common/assert.h" @@ -34,9 +35,34 @@ UdpListenerImpl::UdpListenerImpl(Event::Dispatcher& dispatcher, SocketSharedPtr : BaseListenerImpl(dispatcher, std::move(socket)), cb_(cb), time_source_(time_source), // Default prefer_gro to false for downstream server traffic. config_(config, false) { + parent_drained_callback_registrar_ = socket_->parentDrainedCallbackRegistrar(); socket_->ioHandle().initializeFileEvent( dispatcher, [this](uint32_t events) -> void { onSocketEvent(events); }, - Event::PlatformDefaultTriggerType, Event::FileReadyType::Read | Event::FileReadyType::Write); + Event::PlatformDefaultTriggerType, paused() ? 0 : events_when_unpaused_); + if (paused()) { + parent_drained_callback_registrar_->registerParentDrainedCallback( + socket_->connectionInfoProvider().localAddress(), + [this, &dispatcher, alive = std::weak_ptr(destruction_checker_)]() { + dispatcher.post([this, alive = std::move(alive)]() { + auto still_alive = alive.lock(); + if (still_alive != nullptr) { + unpause(); + } + }); + }); + } +} + +void UdpListenerImpl::unpause() { + // Remove the paused state so enable will actually start listening to events. + parent_drained_callback_registrar_ = absl::nullopt; + if (events_when_unpaused_ != 0) { + // Start listening to events. + enable(); + // There may have already been events while this instance was ignoring them, + // so try reading immediately. + activateRead(); + } } UdpListenerImpl::~UdpListenerImpl() { socket_->ioHandle().resetFileEvents(); } @@ -44,10 +70,18 @@ UdpListenerImpl::~UdpListenerImpl() { socket_->ioHandle().resetFileEvents(); } void UdpListenerImpl::disable() { disableEvent(); } void UdpListenerImpl::enable() { - socket_->ioHandle().enableFileEvents(Event::FileReadyType::Read | Event::FileReadyType::Write); + events_when_unpaused_ = Event::FileReadyType::Read | Event::FileReadyType::Write; + if (!paused()) { + socket_->ioHandle().enableFileEvents(events_when_unpaused_); + } } -void UdpListenerImpl::disableEvent() { socket_->ioHandle().enableFileEvents(0); } +void UdpListenerImpl::disableEvent() { + events_when_unpaused_ = 0; + if (!paused()) { + socket_->ioHandle().enableFileEvents(0); + } +} void UdpListenerImpl::onSocketEvent(short flags) { ASSERT((flags & (Event::FileReadyType::Read | Event::FileReadyType::Write))); diff --git a/source/common/network/udp_listener_impl.h b/source/common/network/udp_listener_impl.h index 723c3c74de75..244f93f3923b 100644 --- a/source/common/network/udp_listener_impl.h +++ b/source/common/network/udp_listener_impl.h @@ -26,6 +26,8 @@ class UdpListenerImpl : public BaseListenerImpl, TimeSource& time_source, const envoy::config::core::v3::UdpSocketConfig& config); ~UdpListenerImpl() override; uint32_t packetsDropped() { return packets_dropped_; } + bool paused() const { return parent_drained_callback_registrar_ != absl::nullopt; } + void unpause(); // Network::Listener void disable() override; @@ -63,6 +65,10 @@ class UdpListenerImpl : public BaseListenerImpl, TimeSource& time_source_; const ResolvedUdpSocketConfig config_; + OptRef parent_drained_callback_registrar_; + // Taking a weak_ptr to this lets us detect if the listener has been destroyed. + std::shared_ptr destruction_checker_ = std::make_shared(true); + uint32_t events_when_unpaused_ = Event::FileReadyType::Read | Event::FileReadyType::Write; }; class UdpListenerWorkerRouterImpl : public UdpListenerWorkerRouter { diff --git a/source/server/BUILD b/source/server/BUILD index 7ee402750e1f..f656303df03e 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -187,6 +187,7 @@ envoy_cc_library( hdrs = envoy_select_hot_restart(["hot_restarting_child.h"]), deps = [ ":hot_restarting_base", + "//envoy/network:parent_drained_callback_registrar_interface", "//source/common/stats:stat_merger_lib", ], ) diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index 417bb4a5f2f3..6e2377c9c6f0 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -124,6 +124,10 @@ void HotRestartImpl::registerUdpForwardingListener( as_child_.registerUdpForwardingListener(address, listener_config); } +OptRef HotRestartImpl::parentDrainedCallbackRegistrar() { + return as_child_; +} + void HotRestartImpl::initialize(Event::Dispatcher& dispatcher, Server::Instance& server) { as_parent_.initialize(dispatcher, server); as_child_.initialize(dispatcher); diff --git a/source/server/hot_restart_impl.h b/source/server/hot_restart_impl.h index ace7d41321d9..9a22b6f3ec13 100644 --- a/source/server/hot_restart_impl.h +++ b/source/server/hot_restart_impl.h @@ -106,6 +106,7 @@ class HotRestartImpl : public HotRestart { void registerUdpForwardingListener( Network::Address::InstanceConstSharedPtr address, std::shared_ptr listener_config) override; + OptRef parentDrainedCallbackRegistrar() override; void initialize(Event::Dispatcher& dispatcher, Server::Instance& server) override; absl::optional sendParentAdminShutdownRequest() override; void sendParentTerminateRequest() override; diff --git a/source/server/hot_restart_nop_impl.h b/source/server/hot_restart_nop_impl.h index 99f006083937..031cf1e4613b 100644 --- a/source/server/hot_restart_nop_impl.h +++ b/source/server/hot_restart_nop_impl.h @@ -20,6 +20,9 @@ class HotRestartNopImpl : public Server::HotRestart { int duplicateParentListenSocket(const std::string&, uint32_t) override { return -1; } void registerUdpForwardingListener(Network::Address::InstanceConstSharedPtr, std::shared_ptr) override {} + OptRef parentDrainedCallbackRegistrar() override { + return absl::nullopt; + } void initialize(Event::Dispatcher&, Server::Instance&) override {} absl::optional sendParentAdminShutdownRequest() override { return absl::nullopt; diff --git a/source/server/hot_restarting_child.cc b/source/server/hot_restarting_child.cc index 0d842a2755eb..0bb33e650db9 100644 --- a/source/server/hot_restarting_child.cc +++ b/source/server/hot_restarting_child.cc @@ -46,9 +46,12 @@ HotRestartingChild::UdpForwardingContext::getListenerForDestination( return it->second; } +// If restart_epoch is 0 there is no parent, so it's effectively already +// drained and terminated. HotRestartingChild::HotRestartingChild(int base_id, int restart_epoch, const std::string& socket_path, mode_t socket_mode) - : HotRestartingBase(base_id), restart_epoch_(restart_epoch) { + : HotRestartingBase(base_id), restart_epoch_(restart_epoch), + parent_terminated_(restart_epoch == 0), parent_drained_(restart_epoch == 0) { main_rpc_stream_.initDomainSocketAddress(&parent_address_); std::string socket_path_udp = socket_path + "_udp"; udp_forwarding_rpc_stream_.initDomainSocketAddress(&parent_address_udp_forwarding_); @@ -102,7 +105,7 @@ void HotRestartingChild::onForwardedUdpPacket(uint32_t worker_index, Network::Ud int HotRestartingChild::duplicateParentListenSocket(const std::string& address, uint32_t worker_index) { - if (restart_epoch_ == 0 || parent_terminated_) { + if (parent_terminated_) { return -1; } @@ -121,7 +124,7 @@ int HotRestartingChild::duplicateParentListenSocket(const std::string& address, } std::unique_ptr HotRestartingChild::getParentStats() { - if (restart_epoch_ == 0 || parent_terminated_) { + if (parent_terminated_) { return nullptr; } @@ -138,7 +141,7 @@ std::unique_ptr HotRestartingChild::getParentStats() { } void HotRestartingChild::drainParentListeners() { - if (restart_epoch_ == 0 || parent_terminated_) { + if (parent_terminated_) { return; } // No reply expected. @@ -154,9 +157,27 @@ void HotRestartingChild::registerUdpForwardingListener( udp_forwarding_context_.registerListener(address, listener_config); } +void HotRestartingChild::registerParentDrainedCallback( + const Network::Address::InstanceConstSharedPtr& address, absl::AnyInvocable callback) { + if (parent_drained_) { + callback(); + } else { + on_drained_actions_.emplace(address->asString(), std::move(callback)); + } +} + +void HotRestartingChild::allDrainsImplicitlyComplete() { + for (auto& drain_action : on_drained_actions_) { + // Call the callback. + std::move(drain_action.second)(); + } + on_drained_actions_.clear(); + parent_drained_ = true; +} + absl::optional HotRestartingChild::sendParentAdminShutdownRequest() { - if (restart_epoch_ == 0 || parent_terminated_) { + if (parent_terminated_) { return absl::nullopt; } @@ -176,9 +197,11 @@ HotRestartingChild::sendParentAdminShutdownRequest() { } void HotRestartingChild::sendParentTerminateRequest() { - if (restart_epoch_ == 0 || parent_terminated_) { + if (parent_terminated_) { return; } + allDrainsImplicitlyComplete(); + HotRestartMessage wrapped_request; wrapped_request.mutable_request()->mutable_terminate(); main_rpc_stream_.sendHotRestartMessage(parent_address_, wrapped_request); @@ -186,15 +209,17 @@ void HotRestartingChild::sendParentTerminateRequest() { // Note that the 'generation' counter needs to retain the contribution from // the parent. - stat_merger_->retainParentGaugeValue(hot_restart_generation_stat_name_); + if (stat_merger_ != nullptr) { + stat_merger_->retainParentGaugeValue(hot_restart_generation_stat_name_); - // Now it is safe to forget our stat transferral state. - // - // This destruction is actually important far beyond memory efficiency. The - // scope-based temporary counter logic relies on the StatMerger getting - // destroyed once hot restart's stat merging is all done. (See stat_merger.h - // for details). - stat_merger_.reset(); + // Now it is safe to forget our stat transferral state. + // + // This destruction is actually important far beyond memory efficiency. The + // scope-based temporary counter logic relies on the StatMerger getting + // destroyed once hot restart's stat merging is all done. (See stat_merger.h + // for details). + stat_merger_.reset(); + } } void HotRestartingChild::mergeParentStats(Stats::Store& stats_store, diff --git a/source/server/hot_restarting_child.h b/source/server/hot_restarting_child.h index 7f61dfc59f55..7f485511b3d4 100644 --- a/source/server/hot_restarting_child.h +++ b/source/server/hot_restarting_child.h @@ -1,5 +1,6 @@ #pragma once +#include "envoy/network/parent_drained_callback_registrar.h" #include "envoy/server/instance.h" #include "source/common/stats/stat_merger.h" @@ -11,7 +12,8 @@ namespace Server { /** * The child half of hot restarting. Issues requests and commands to the parent. */ -class HotRestartingChild : public HotRestartingBase { +class HotRestartingChild : public HotRestartingBase, + public Network::ParentDrainedCallbackRegistrar { public: // A structure to record the set of registered UDP listeners keyed on their addresses, // to support QUIC packet forwarding. @@ -42,7 +44,7 @@ class HotRestartingChild : public HotRestartingBase { HotRestartingChild(int base_id, int restart_epoch, const std::string& socket_path, mode_t socket_mode); - ~HotRestartingChild() = default; + ~HotRestartingChild() override = default; void initialize(Event::Dispatcher& dispatcher); void shutdown(); @@ -50,6 +52,9 @@ class HotRestartingChild : public HotRestartingBase { int duplicateParentListenSocket(const std::string& address, uint32_t worker_index); void registerUdpForwardingListener(Network::Address::InstanceConstSharedPtr address, std::shared_ptr listener_config); + // From Network::ParentDrainedCallbackRegistrar. + void registerParentDrainedCallback(const Network::Address::InstanceConstSharedPtr& addr, + absl::AnyInvocable action) override; std::unique_ptr getParentStats(); void drainParentListeners(); absl::optional sendParentAdminShutdownRequest(); @@ -60,15 +65,21 @@ class HotRestartingChild : public HotRestartingBase { protected: void onSocketEventUdpForwarding(); void onForwardedUdpPacket(uint32_t worker_index, Network::UdpRecvData&& data); + // When call to terminate parent is sent, or parent is already terminated, + void allDrainsImplicitlyComplete(); private: friend class HotRestartUdpForwardingTestHelper; const int restart_epoch_; - bool parent_terminated_{}; + bool parent_terminated_; + bool parent_drained_; sockaddr_un parent_address_; sockaddr_un parent_address_udp_forwarding_; std::unique_ptr stat_merger_{}; Stats::StatName hot_restart_generation_stat_name_; + // There are multiple listener instances per address that must all be reactivated + // when the parent is drained, so a multimap is used to contain them. + std::unordered_multimap> on_drained_actions_; Event::FileEventPtr socket_event_udp_forwarding_; UdpForwardingContext udp_forwarding_context_; }; diff --git a/test/common/network/BUILD b/test/common/network/BUILD index ea59472db9b6..9e4c1c2e8205 100644 --- a/test/common/network/BUILD +++ b/test/common/network/BUILD @@ -249,6 +249,7 @@ envoy_cc_test( "//source/common/network:utility_lib", "//source/common/stats:stats_lib", "//test/common/network:listener_impl_test_base_lib", + "//test/mocks/network:mock_parent_drained_callback_registrar", "//test/mocks/network:network_mocks", "//test/mocks/server:server_mocks", "//test/test_common:environment_lib", diff --git a/test/common/network/udp_listener_impl_batch_writer_test.cc b/test/common/network/udp_listener_impl_batch_writer_test.cc index 39b69e86c02a..35156dbd6a83 100644 --- a/test/common/network/udp_listener_impl_batch_writer_test.cc +++ b/test/common/network/udp_listener_impl_batch_writer_test.cc @@ -61,6 +61,7 @@ size_t getPacketLength(const msghdr* msg) { class UdpListenerImplBatchWriterTest : public UdpListenerImplTestBase { public: void SetUp() override { + UdpListenerImplTestBase::setup(); // Set listening socket options and set UdpGsoBatchWriter server_socket_->addOptions(SocketOptionFactory::buildIpPacketInfoOptions()); server_socket_->addOptions(SocketOptionFactory::buildRxQueueOverFlowOptions()); diff --git a/test/common/network/udp_listener_impl_test.cc b/test/common/network/udp_listener_impl_test.cc index 18810ca7467a..6df91c150355 100644 --- a/test/common/network/udp_listener_impl_test.cc +++ b/test/common/network/udp_listener_impl_test.cc @@ -16,6 +16,7 @@ #include "test/common/network/udp_listener_impl_test_base.h" #include "test/mocks/api/mocks.h" +#include "test/mocks/network/mock_parent_drained_callback_registrar.h" #include "test/mocks/network/mocks.h" #include "test/test_common/environment.h" #include "test/test_common/network_utility.h" @@ -52,6 +53,7 @@ class OverrideOsSysCallsImpl : public Api::OsSysCallsImpl { class UdpListenerImplTest : public UdpListenerImplTestBase { public: void setup(bool prefer_gro = false) { + UdpListenerImplTestBase::setup(); ON_CALL(override_syscall_, supportsUdpGro()).WillByDefault(Return(false)); // Return the real version by default. ON_CALL(override_syscall_, supportsMmsg()) @@ -385,6 +387,106 @@ TEST_P(UdpListenerImplTest, UdpListenerEnableDisable) { dispatcher_->run(Event::Dispatcher::RunType::Block); } +class HotRestartedUdpListenerImplTest : public UdpListenerImplTest { +public: + void SetUp() override { +#ifdef WIN32 + GTEST_SKIP() << "Hot restart is not supported on Windows."; +#endif + } + void setup() { + io_handle_ = &useHotRestartSocket(registrar_); + // File event should be created listening to no events (i.e. disabled). + EXPECT_CALL(*io_handle_, createFileEvent_(_, _, _, 0)); + // Parent drained callback should be registered when the listener is created. + // We capture the callback so we can simulate "drain complete". + EXPECT_CALL(registrar_, registerParentDrainedCallback(_, _)) + .WillOnce( + [this](const Address::InstanceConstSharedPtr&, absl::AnyInvocable callback) { + parent_drained_callback_ = std::move(callback); + }); + UdpListenerImplTest::setup(); + testing::Mock::VerifyAndClearExpectations(®istrar_); + } + +protected: + MockParentDrainedCallbackRegistrar registrar_; + MockIoHandle* io_handle_; + absl::AnyInvocable parent_drained_callback_; +}; + +INSTANTIATE_TEST_SUITE_P(IpVersions, HotRestartedUdpListenerImplTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + +/** + * During hot restart, while the parent instance is draining, a quic udp + * listener (created with a parent_drained_callback_registrar) should not + * be reading packets, regardless of enable/disable calls. + * It should begin reading packets after drain completes. + */ +TEST_P(HotRestartedUdpListenerImplTest, EnableAndDisableDuringParentDrainShouldDoNothing) { + setup(); + // Enabling and disabling listener should *not* trigger any + // event actions on the io_handle, because of listener being paused + // while draining. + EXPECT_CALL(*io_handle_, enableFileEvents(_)).Times(0); + listener_->disable(); + listener_->enable(); + testing::Mock::VerifyAndClearExpectations(io_handle_); + // Ending parent drain should cause io_handle to go into reading mode. + EXPECT_CALL(*io_handle_, + enableFileEvents(Event::FileReadyType::Read | Event::FileReadyType::Write)); + EXPECT_CALL(*io_handle_, activateFileEvents(Event::FileReadyType::Read)); + std::move(parent_drained_callback_)(); + dispatcher_->run(Event::Dispatcher::RunType::Block); + testing::Mock::VerifyAndClearExpectations(io_handle_); + // Enabling and disabling once unpaused should update io_handle. + EXPECT_CALL(*io_handle_, enableFileEvents(0)); + listener_->disable(); + testing::Mock::VerifyAndClearExpectations(io_handle_); + EXPECT_CALL(*io_handle_, + enableFileEvents(Event::FileReadyType::Read | Event::FileReadyType::Write)); + listener_->enable(); + testing::Mock::VerifyAndClearExpectations(io_handle_); +} + +/** + * Mostly the same as EnableAndDisableDuringParentDrainShouldDoNothing, but in disabled state when + * drain ends. + */ +TEST_P(HotRestartedUdpListenerImplTest, EndingParentDrainedWhileDisabledShouldNotStartReading) { + setup(); + // Enabling and disabling listener should *not* trigger any + // event actions on the io_handle, because of listener being paused + // while draining. + EXPECT_CALL(*io_handle_, enableFileEvents(_)).Times(0); + listener_->enable(); + listener_->disable(); + testing::Mock::VerifyAndClearExpectations(io_handle_); + // Ending drain should not trigger any event changes because the last state + // of the listener was disabled. + std::move(parent_drained_callback_)(); + dispatcher_->run(Event::Dispatcher::RunType::Block); + testing::Mock::VerifyAndClearExpectations(io_handle_); + // Enabling after unpaused should set io_handle to reading/writing. + EXPECT_CALL(*io_handle_, + enableFileEvents(Event::FileReadyType::Read | Event::FileReadyType::Write)); + listener_->enable(); + testing::Mock::VerifyAndClearExpectations(io_handle_); +} + +TEST_P(HotRestartedUdpListenerImplTest, + ParentDrainedCallbackAfterListenerDestroyedShouldDoNothing) { + setup(); + EXPECT_CALL(*io_handle_, enableFileEvents(_)).Times(0); + listener_ = nullptr; + // Signaling end-of-drain after the listener was destroyed should do nothing. + std::move(parent_drained_callback_)(); + dispatcher_->run(Event::Dispatcher::RunType::Block); + // At this point io_handle should be an invalid reference. +} + /** * Tests UDP listener's error callback. */ diff --git a/test/common/network/udp_listener_impl_test_base.h b/test/common/network/udp_listener_impl_test_base.h index 112f89d68dc3..9b7636e13ae8 100644 --- a/test/common/network/udp_listener_impl_test_base.h +++ b/test/common/network/udp_listener_impl_test_base.h @@ -31,13 +31,24 @@ namespace Envoy { namespace Network { class UdpListenerImplTestBase : public ListenerImplTestBase { -public: - UdpListenerImplTestBase() - : server_socket_(createServerSocket(true)), send_to_addr_(getServerLoopbackAddress()) { +protected: + MockIoHandle& + useHotRestartSocket(OptRef parent_drained_callback_registrar) { + auto io_handle = std::make_unique>(); + MockIoHandle& ret = *io_handle; + server_socket_ = createServerSocketFromExistingHandle(std::move(io_handle), + parent_drained_callback_registrar); + return ret; + } + + void setup() { + if (server_socket_ == nullptr) { + server_socket_ = createServerSocket(true); + } + send_to_addr_ = Address::InstanceConstSharedPtr(getServerLoopbackAddress()); time_system_.advanceTimeWait(std::chrono::milliseconds(100)); } -protected: Address::Instance* getServerLoopbackAddress() { if (version_ == Address::IpVersion::v4) { return new Address::Ipv4Instance( @@ -60,6 +71,14 @@ class UdpListenerImplTestBase : public ListenerImplTestBase { bind); } + SocketSharedPtr createServerSocketFromExistingHandle( + IoHandlePtr&& io_handle, + OptRef parent_drained_callback_registrar) { + return std::make_shared( + std::move(io_handle), Network::Test::getCanonicalLoopbackAddress(version_), + SocketOptionFactory::buildIpFreebindOptions(), parent_drained_callback_registrar); + } + Address::InstanceConstSharedPtr getNonDefaultSourceAddress() { // Use a self address that is unlikely to be picked by source address discovery // algorithm if not specified in recvmsg/recvmmsg. Port is not taken into diff --git a/test/integration/python/hotrestart_handoff_test.py b/test/integration/python/hotrestart_handoff_test.py index 7d975bd7dda7..dcbbb5a8ba1e 100644 --- a/test/integration/python/hotrestart_handoff_test.py +++ b/test/integration/python/hotrestart_handoff_test.py @@ -304,7 +304,7 @@ async def _wait_for_envoy_epoch(i: int): pass await asyncio.sleep(0.2) # Envoy instance with expected restart_epoch should have started up - assert expected_substring in response, f"server_info={response}" + assert expected_substring in response, f"expected_substring={expected_substring}, server_info={response}" class IntegrationTest(unittest.IsolatedAsyncioTestCase): diff --git a/test/mocks/network/BUILD b/test/mocks/network/BUILD index 82b5ef8c79f1..73c9854193a3 100644 --- a/test/mocks/network/BUILD +++ b/test/mocks/network/BUILD @@ -42,6 +42,12 @@ envoy_cc_mock( ], ) +envoy_cc_mock( + name = "mock_parent_drained_callback_registrar", + hdrs = ["mock_parent_drained_callback_registrar.h"], + deps = ["//envoy/network:parent_drained_callback_registrar_interface"], +) + envoy_cc_mock( name = "network_mocks", srcs = ["mocks.cc"], diff --git a/test/mocks/network/mock_parent_drained_callback_registrar.h b/test/mocks/network/mock_parent_drained_callback_registrar.h new file mode 100644 index 000000000000..ae82b52d31f6 --- /dev/null +++ b/test/mocks/network/mock_parent_drained_callback_registrar.h @@ -0,0 +1,18 @@ +#pragma once + +#include "envoy/network/parent_drained_callback_registrar.h" + +#include "gmock/gmock.h" + +namespace Envoy { +namespace Network { + +class MockParentDrainedCallbackRegistrar : public ParentDrainedCallbackRegistrar { +public: + MOCK_METHOD(void, registerParentDrainedCallback, + (const Address::InstanceConstSharedPtr& address, + absl::AnyInvocable callback)); +}; + +} // namespace Network +} // namespace Envoy diff --git a/test/mocks/server/hot_restart.h b/test/mocks/server/hot_restart.h index c83142692c06..99bfa3ccbb0c 100644 --- a/test/mocks/server/hot_restart.h +++ b/test/mocks/server/hot_restart.h @@ -20,6 +20,8 @@ class MockHotRestart : public HotRestart { MOCK_METHOD(void, registerUdpForwardingListener, (Network::Address::InstanceConstSharedPtr address, std::shared_ptr listener_config)); + MOCK_METHOD(OptRef, parentDrainedCallbackRegistrar, ()); + MOCK_METHOD(void, whenDrainComplete, (absl::string_view addr, absl::AnyInvocable action)); MOCK_METHOD(void, initialize, (Event::Dispatcher & dispatcher, Server::Instance& server)); MOCK_METHOD(absl::optional, sendParentAdminShutdownRequest, ()); MOCK_METHOD(void, sendParentTerminateRequest, ()); diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index 81baf88181b5..28e1427564e1 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -87,6 +87,14 @@ class HotRestartImplTest : public testing::Test { std::unique_ptr hot_restart_; }; +TEST_F(HotRestartImplTest, ParentDrainedCallbackRegistrarIsSetAndCanBeCalled) { + setup(); + OptRef registrar = + hot_restart_->parentDrainedCallbackRegistrar(); + ASSERT_TRUE(registrar.has_value()); + registrar->registerParentDrainedCallback(test_addresses_.ipv4_test_addr_, []() {}); +} + TEST_F(HotRestartImplTest, VersionString) { // Tests that the version-string will be consistent and HOT_RESTART_VERSION, // between multiple instantiations. diff --git a/test/server/hot_restarting_child_test.cc b/test/server/hot_restarting_child_test.cc index f2455034f054..d16653d6454a 100644 --- a/test/server/hot_restarting_child_test.cc +++ b/test/server/hot_restarting_child_test.cc @@ -67,6 +67,11 @@ class FakeHotRestartingParent : public HotRestartingBase { }); udp_forwarding_rpc_stream_.sendHotRestartMessage(child_address_udp_forwarding_, message); } + void expectParentTerminateMessages() { + EXPECT_CALL(os_sys_calls_, sendmsg(_, _, _)).WillOnce([](int, const msghdr* msg, int) { + return Api::SysCallSizeResult{static_cast(msg->msg_iov[0].iov_len), 0}; + }); + } Api::MockOsSysCalls& os_sys_calls_; Event::FileReadyCb udp_file_ready_callback_; sockaddr_un child_address_udp_forwarding_; @@ -100,6 +105,36 @@ class HotRestartingChildTest : public testing::Test { std::unique_ptr hot_restarting_child_; }; +TEST_F(HotRestartingChildTest, ParentDrainedCallbacksAreCalled) { + auto test_listener_addr = Network::Utility::resolveUrl("udp://127.0.0.1:1234"); + auto test_listener_addr2 = Network::Utility::resolveUrl("udp://127.0.0.1:1235"); + testing::MockFunction callback1; + testing::MockFunction callback2; + hot_restarting_child_->registerParentDrainedCallback(test_listener_addr, + callback1.AsStdFunction()); + hot_restarting_child_->registerParentDrainedCallback(test_listener_addr2, + callback2.AsStdFunction()); + EXPECT_CALL(callback1, Call()); + EXPECT_CALL(callback2, Call()); + fake_parent_->expectParentTerminateMessages(); + hot_restarting_child_->sendParentTerminateRequest(); +} + +TEST_F(HotRestartingChildTest, ParentDrainedCallbacksAreCalledImmediatelyWhenAlreadyDrained) { + auto test_listener_addr = Network::Utility::resolveUrl("udp://127.0.0.1:1234"); + auto test_listener_addr2 = Network::Utility::resolveUrl("udp://127.0.0.1:1235"); + testing::MockFunction callback1; + testing::MockFunction callback2; + fake_parent_->expectParentTerminateMessages(); + hot_restarting_child_->sendParentTerminateRequest(); + EXPECT_CALL(callback1, Call()); + EXPECT_CALL(callback2, Call()); + hot_restarting_child_->registerParentDrainedCallback(test_listener_addr, + callback1.AsStdFunction()); + hot_restarting_child_->registerParentDrainedCallback(test_listener_addr2, + callback2.AsStdFunction()); +} + TEST_F(HotRestartingChildTest, LogsErrorOnReplyMessageInUdpStream) { envoy::HotRestartMessage msg; msg.mutable_reply(); From 572860489074d1a47b05de307dec0f275f3c1410 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 29 Feb 2024 09:40:39 -0800 Subject: [PATCH 12/12] network: fix tsan test flake (#32631) Fixes #32527 Signed-off-by: Greg Greenway --- source/common/network/address_impl.cc | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/source/common/network/address_impl.cc b/source/common/network/address_impl.cc index cb6b233cdc7a..40dfb793261c 100644 --- a/source/common/network/address_impl.cc +++ b/source/common/network/address_impl.cc @@ -213,18 +213,18 @@ std::string Ipv4Instance::sockaddrToString(const sockaddr_in& addr) { } namespace { -bool force_ipv4_unsupported_for_test = false; +std::atomic force_ipv4_unsupported_for_test = false; } Cleanup Ipv4Instance::forceProtocolUnsupportedForTest(bool new_val) { - bool old_val = force_ipv4_unsupported_for_test; - force_ipv4_unsupported_for_test = new_val; - return {[old_val]() { force_ipv4_unsupported_for_test = old_val; }}; + bool old_val = force_ipv4_unsupported_for_test.load(); + force_ipv4_unsupported_for_test.store(new_val); + return {[old_val]() { force_ipv4_unsupported_for_test.store(old_val); }}; } absl::Status Ipv4Instance::validateProtocolSupported() { static const bool supported = SocketInterfaceSingleton::get().ipFamilySupported(AF_INET); - if (supported && !force_ipv4_unsupported_for_test) { + if (supported && !force_ipv4_unsupported_for_test.load(std::memory_order_relaxed)) { return absl::OkStatus(); } return absl::FailedPreconditionError("IPv4 addresses are not supported on this machine"); @@ -335,18 +335,18 @@ Ipv6Instance::Ipv6Instance(absl::Status& status, const sockaddr_in6& address, bo } namespace { -bool force_ipv6_unsupported_for_test = false; +std::atomic force_ipv6_unsupported_for_test = false; } Cleanup Ipv6Instance::forceProtocolUnsupportedForTest(bool new_val) { - bool old_val = force_ipv6_unsupported_for_test; - force_ipv6_unsupported_for_test = new_val; - return {[old_val]() { force_ipv6_unsupported_for_test = old_val; }}; + bool old_val = force_ipv6_unsupported_for_test.load(); + force_ipv6_unsupported_for_test.store(new_val); + return {[old_val]() { force_ipv6_unsupported_for_test.store(old_val); }}; } absl::Status Ipv6Instance::validateProtocolSupported() { static const bool supported = SocketInterfaceSingleton::get().ipFamilySupported(AF_INET6); - if (supported && !force_ipv6_unsupported_for_test) { + if (supported && !force_ipv6_unsupported_for_test.load(std::memory_order_relaxed)) { return absl::OkStatus(); } return absl::FailedPreconditionError("IPv6 addresses are not supported on this machine");