From 412fbada2ab24afcd1429b347f2e14db924712fb Mon Sep 17 00:00:00 2001
From: Kayla Reopelle <kreopelle@newrelic.com>
Date: Wed, 28 Aug 2024 13:35:22 -0700
Subject: [PATCH] feat: Add instrument registry for logs

LoggerProvider now has an instrument registry to track loggers and
prevent the creation of duplicates.

This also prevents new loggers from being created if the provider
is stopped
---
 logs_api/lib/opentelemetry/logs/logger.rb     |  5 +++
 .../lib/opentelemetry/logs/logger_provider.rb |  4 ++-
 .../opentelemetry/sdk/logs/logger_provider.rb | 25 +++++++++----
 .../sdk/logs/logger_provider_test.rb          | 36 +++++++++++++++++++
 4 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/logs_api/lib/opentelemetry/logs/logger.rb b/logs_api/lib/opentelemetry/logs/logger.rb
index c6caa34a7..6374b296b 100644
--- a/logs_api/lib/opentelemetry/logs/logger.rb
+++ b/logs_api/lib/opentelemetry/logs/logger.rb
@@ -8,6 +8,11 @@ module OpenTelemetry
   module Logs
     # No-op implementation of logger.
     class Logger
+      def initialize
+        @mutex = Mutex.new
+        @instrument_registry = {}
+      end
+
       # rubocop:disable Style/EmptyMethod
 
       # Emit a {LogRecord} to the processing pipeline.
diff --git a/logs_api/lib/opentelemetry/logs/logger_provider.rb b/logs_api/lib/opentelemetry/logs/logger_provider.rb
index 13343407d..9c9b9c79e 100644
--- a/logs_api/lib/opentelemetry/logs/logger_provider.rb
+++ b/logs_api/lib/opentelemetry/logs/logger_provider.rb
@@ -9,7 +9,9 @@ module Logs
     # No-op implementation of a logger provider.
     class LoggerProvider
       NOOP_LOGGER = OpenTelemetry::Logs::Logger.new
-      private_constant :NOOP_LOGGER
+      # TODO: Make NOOP_LOGGER private again
+      # NOOP_LOGGER is used in the SDK logger at this time until the ProxyLogger has been created
+      # private_constant :NOOP_LOGGER
 
       # Returns an {OpenTelemetry::Logs::Logger} instance.
       #
diff --git a/logs_sdk/lib/opentelemetry/sdk/logs/logger_provider.rb b/logs_sdk/lib/opentelemetry/sdk/logs/logger_provider.rb
index 99e7df10e..3a3c10856 100644
--- a/logs_sdk/lib/opentelemetry/sdk/logs/logger_provider.rb
+++ b/logs_sdk/lib/opentelemetry/sdk/logs/logger_provider.rb
@@ -9,6 +9,9 @@ module SDK
     module Logs
       # The SDK implementation of OpenTelemetry::Logs::LoggerProvider.
       class LoggerProvider < OpenTelemetry::Logs::LoggerProvider
+        Key = Struct.new(:name, :version)
+        private_constant(:Key)
+
         UNEXPECTED_ERROR_MESSAGE = 'unexpected error in ' \
           'OpenTelemetry::SDK::Logs::LoggerProvider#%s'
 
@@ -25,6 +28,8 @@ def initialize(resource: OpenTelemetry::SDK::Resources::Resource.create)
           @mutex = Mutex.new
           @resource = resource
           @stopped = false
+          @registry = {}
+          @registry_mutex = Mutex.new
         end
 
         # Returns an {OpenTelemetry::SDK::Logs::Logger} instance.
@@ -34,14 +39,22 @@ def initialize(resource: OpenTelemetry::SDK::Resources::Resource.create)
         #
         # @return [OpenTelemetry::SDK::Logs::Logger]
         def logger(name:, version: nil)
-          version ||= ''
+          if @stopped
+            # TODO: Return a ProxyLogger instead once that's available
+            OpenTelemetry.logger.warn('calling LoggerProvider#logger after shutdown, a noop logger will be returned')
+            OpenTelemetry::Logs::LoggerProvider::NOOP_LOGGER
+          else
+            version ||= ''
+
+            if !name.is_a?(String) || name.empty?
+              OpenTelemetry.logger.warn('LoggerProvider#logger called with an ' \
+                "invalid name. Name provided: #{name.inspect}")
+            end
 
-          if !name.is_a?(String) || name.empty?
-            OpenTelemetry.logger.warn('LoggerProvider#logger called with an ' \
-              "invalid name. Name provided: #{name.inspect}")
+            @registry_mutex.synchronize do
+              @registry[Key.new(name, version)] ||= Logger.new(name, version, self)
+            end
           end
-
-          Logger.new(name, version, self)
         end
 
         # Adds a new log record processor to this LoggerProvider's
diff --git a/logs_sdk/test/opentelemetry/sdk/logs/logger_provider_test.rb b/logs_sdk/test/opentelemetry/sdk/logs/logger_provider_test.rb
index 023a9a67c..e9b5380da 100644
--- a/logs_sdk/test/opentelemetry/sdk/logs/logger_provider_test.rb
+++ b/logs_sdk/test/opentelemetry/sdk/logs/logger_provider_test.rb
@@ -215,4 +215,40 @@ def mock_context.value(key); OpenTelemetry::Trace::Span::INVALID; end # rubocop:
       end
     end
   end
+
+  describe 'instrument registry' do
+    # On the first call, create a logger with an empty string for name and
+    # version and add to the registry. The second call returns that logger
+    # from the registry instead of creating a new instance.
+    it 'reuses the same logger if called twice when name and version are nil' do
+      logger = logger_provider.logger(name: nil, version: nil)
+      logger2 = logger_provider.logger(name: nil, version: nil)
+
+      assert_instance_of(Logs::Logger, logger)
+      assert_same(logger, logger2)
+    end
+
+    describe 'when stopped' do
+      it 'logs a warning' do
+        logger_provider.instance_variable_set(:@stopped, true)
+
+        OpenTelemetry::TestHelpers.with_test_logger do |log_stream|
+          logger_provider.logger(name: '')
+          assert_match(
+            /calling LoggerProvider#logger after shutdown/,
+            log_stream.string
+          )
+        end
+      end
+
+      it 'does not add a new logger to the registry' do
+        before_stopped_size = logger_provider.instance_variable_get(:@registry).keys.size
+        logger_provider.instance_variable_set(:@stopped, true)
+        logger_provider.logger(name: 'new_logger')
+        after_stopped_size = logger_provider.instance_variable_get(:@registry).keys.size
+
+        assert_equal(before_stopped_size, after_stopped_size)
+      end
+    end
+  end
 end