tracing: use weak references when caching HttpTracers inside HttpTracerManager

source/common/tracing/http_tracer_manager_impl.cc

@@ -18,11 +18,14 @@ HttpTracerManagerImpl::getOrCreateHttpTracer(const envoy::config::trace::v3::Tra
   const auto cache_key = MessageUtil::hash(*config);
   const auto it = http_tracers_.find(cache_key);
   if (it != http_tracers_.end()) {
-    return it->second;
+    auto http_tracer = it->second.lock();
+    if (http_tracer) { // HttpTracer might have been released since it's a weak reference
+      return http_tracer;
+    }
   }
 
-  // Initialize tracing driver.
-  ENVOY_LOG(info, "instantiating tracing driver: {}", config->name());
+  // Initialize a new tracer.
+  ENVOY_LOG(info, "instantiating a new tracer: {}", config->name());
 
   // Now see if there is a factory that will accept the config.
   auto& factory =
@@ -31,10 +34,7 @@ HttpTracerManagerImpl::getOrCreateHttpTracer(const envoy::config::trace::v3::Tra
       *config, factory_context_->messageValidationVisitor(), factory);
 
   HttpTracerSharedPtr http_tracer = factory.createHttpTracer(*message, *factory_context_);
-  // TODO(yskopets): In the initial implementation HttpTracers are never removed from the cache.
-  // Once HttpTracer implementations have been revised to support removal and cleanup,
-  // this cache must be reworked to release HttpTracers as soon as they are no longer in use.
-  http_tracers_.emplace(cache_key, http_tracer);
+  http_tracers_.insert_or_assign(cache_key, http_tracer); // cache a weak reference
   return http_tracer;
 }
 

source/common/tracing/http_tracer_manager_impl.h

@@ -25,7 +25,7 @@ class HttpTracerManagerImpl : public HttpTracerManager,
   const HttpTracerSharedPtr null_tracer_{std::make_shared<Tracing::HttpNullTracer>()};
 
   // HttpTracers indexed by the hash of their configuration.
-  absl::flat_hash_map<std::size_t, HttpTracerSharedPtr> http_tracers_;
+  absl::flat_hash_map<std::size_t, std::weak_ptr<HttpTracer>> http_tracers_;
 };
 
 } // namespace Tracing

test/common/tracing/BUILD

@@ -46,6 +46,7 @@ envoy_cc_test(
         "//source/common/tracing:http_tracer_lib",
         "//source/common/tracing:http_tracer_manager_lib",
         "//test/mocks/server:server_mocks",
+        "//test/mocks/tracing:tracing_mocks",
         "//test/test_common:registry_lib",
     ],
 )

test/common/tracing/http_tracer_manager_impl_test.cc

@@ -3,13 +3,16 @@
 #include "common/tracing/http_tracer_manager_impl.h"
 
 #include "test/mocks/server/mocks.h"
+#include "test/mocks/tracing/mocks.h"
 #include "test/test_common/registry.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+using testing::InvokeWithoutArgs;
 using testing::NiceMock;
 using testing::NotNull;
+using testing::Return;
 using testing::WhenDynamicCastTo;
 
 namespace Envoy {
@@ -121,6 +124,64 @@ TEST_F(HttpTracerManagerImplTest, ShouldFailIfProviderSpecificConfigIsNotValid)
 )");
 }
 
+class HttpTracerManagerImplCacheTest : public testing::Test {
+public:
+  NiceMock<Server::Configuration::MockServerFactoryContext> server_factory_context_;
+  HttpTracerManagerImpl http_tracer_manager_{std::make_unique<TracerFactoryContextImpl>(
+      server_factory_context_, ProtobufMessage::getStrictValidationVisitor())};
+
+  NiceMock<Server::Configuration::MockTracerFactory> tracer_factory_{"envoy.tracers.mock"};
+
+private:
+  Registry::InjectFactory<Server::Configuration::TracerFactory> registered_tracer_factory_{
+      tracer_factory_};
+};
+
+TEST_F(HttpTracerManagerImplCacheTest, ShouldCacheHttpTracersUsingWeakReferences) {
+  envoy::config::trace::v3::Tracing_Http tracing_config;
+  tracing_config.set_name("envoy.tracers.mock");
+
+  HttpTracer* expected_tracer = new NiceMock<MockHttpTracer>();
+
+  // Expect HttpTracerManager to create a new HttpTracer.
+  EXPECT_CALL(tracer_factory_, createHttpTracer(_, _))
+      .WillOnce(InvokeWithoutArgs(
+          [expected_tracer] { return std::unique_ptr<HttpTracer>(expected_tracer); }));
+
+  auto actual_tracer_one = http_tracer_manager_.getOrCreateHttpTracer(&tracing_config);
+
+  EXPECT_EQ(actual_tracer_one.get(), expected_tracer);
+
+  // Expect HttpTracerManager to re-use cached value.
+  auto actual_tracer_two = http_tracer_manager_.getOrCreateHttpTracer(&tracing_config);
+
+  EXPECT_EQ(actual_tracer_two.get(), expected_tracer);
+
+  // Expect HttpTracerManager to use weak references under the hood and release HttpTracer as soon
+  // as it's no longer in use.
+  std::weak_ptr<HttpTracer> weak_pointer{actual_tracer_one};
+
+  actual_tracer_one.reset();
+  // There is still one strong reference left.
+  EXPECT_NE(weak_pointer.lock(), nullptr);
+
+  actual_tracer_two.reset();
+  // There are no more strong references left.
+  EXPECT_EQ(weak_pointer.lock(), nullptr);
+
+  HttpTracer* expected_another_tracer = new NiceMock<MockHttpTracer>();
+
+  // Expect HttpTracerManager to create a new HttpTracer once again.
+  EXPECT_CALL(tracer_factory_, createHttpTracer(_, _))
+      .WillOnce(InvokeWithoutArgs([expected_another_tracer] {
+        return std::unique_ptr<HttpTracer>(expected_another_tracer);
+      }));
+
+  auto actual_tracer_three = http_tracer_manager_.getOrCreateHttpTracer(&tracing_config);
+
+  EXPECT_EQ(actual_tracer_three.get(), expected_another_tracer);
+}
+
 } // namespace
 } // namespace Tracing
 } // namespace Envoy

test/mocks/server/mocks.cc

@@ -275,6 +275,13 @@ MockHealthCheckerFactoryContext::~MockHealthCheckerFactoryContext() = default;
 MockFilterChainFactoryContext::MockFilterChainFactoryContext() = default;
 MockFilterChainFactoryContext::~MockFilterChainFactoryContext() = default;
 
+MockTracerFactory::MockTracerFactory(const std::string& name) : name_(name) {
+  ON_CALL(*this, createEmptyConfigProto()).WillByDefault(Invoke([] {
+    return std::make_unique<ProtobufWkt::Struct>();
+  }));
+}
+MockTracerFactory::~MockTracerFactory() = default;
+
 MockTracerFactoryContext::MockTracerFactoryContext() {
   ON_CALL(*this, serverFactoryContext()).WillByDefault(ReturnRef(server_factory_context_));
   ON_CALL(*this, messageValidationVisitor())

test/mocks/server/mocks.h

@@ -636,6 +636,21 @@ class MockFilterChainFactoryContext : public MockFactoryContext, public FilterCh
   ~MockFilterChainFactoryContext() override;
 };
 
+class MockTracerFactory : public TracerFactory {
+public:
+  explicit MockTracerFactory(const std::string& name);
+  ~MockTracerFactory() override;
+
+  std::string name() const override { return name_; }
+
+  MOCK_METHOD(ProtobufTypes::MessagePtr, createEmptyConfigProto, ());
+  MOCK_METHOD(Tracing::HttpTracerPtr, createHttpTracer,
+              (const Protobuf::Message& config, TracerFactoryContext& context));
+
+private:
+  std::string name_;
+};
+
 class MockTracerFactoryContext : public TracerFactoryContext {
 public:
   MockTracerFactoryContext();