Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: OpenTelemetry.logger_provider API, ProxyLoggers, Configuration, and Instrument Registry #1725

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

kaylareopelle
Copy link
Contributor

@kaylareopelle kaylareopelle commented Sep 10, 2024

This PR has a few different features that are all somewhat related:

  • Adds the APIs to set a global logger provider using OpenTelemetry.logger_provider
    • Related to this, introduces OpenTelemetry::Internal::ProxyLoggerProvider and OpenTelemetry::Internal::ProxyLogger classes for defaults.
    • Like in the main SDK with traces, these proxies are upgraded to real LoggerProviders/Loggers when set.
  • Adds the ability to configure the logs exporter using OTEL_LOGS_EXPORTER. The options available are console, none, and otlp. The OTLP exporter option will fail gracefully unless the yet to be released opentelemetry-exporter-otlp-logs gem is available. This takes the ConfigurationPatch approach, similar to feat: Update metrics configuration patch #1548
  • Adds a thread-safe registry for loggers to access the same logger based on name and version

Closes #1699
Closes #1702

@kaylareopelle kaylareopelle changed the title feat: Logs APIs, ProxyLoggers, Configuration, and Instrument Registry feat: OpenTelemetry.logger_provider API, ProxyLoggers, Configuration, and Instrument Registry Sep 12, 2024
@kaylareopelle kaylareopelle marked this pull request as ready for review September 12, 2024 05:42
@kaylareopelle kaylareopelle self-assigned this Sep 27, 2024
# @return [ProxyLogger]
def initialize
super
@mutex = Mutex.new
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a mutex here? We don't have it for the proxy tracer. I believe we can remove this because the delegate is set from proxy logger provider, which synchronizes access using its mutex.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question! I don't think we need it here. I was copying the setup in the ProxyMeter and didn't cross-reference what was going on with the ProxyTracer.

Should we remove the mutex from the ProxyMeter too?

def delegate=(meter)
@mutex.synchronize do
if @delegate.nil?
@delegate = meter
@instrument_registry.each_value { |instrument| instrument.upgrade_with(meter) }
else
OpenTelemetry.logger.warn 'Attempt to reset delegate in ProxyMeter ignored.'
end
end
end

@@ -34,14 +39,21 @@ def initialize(resource: OpenTelemetry::SDK::Resources::Resource.create)
#
# @return [OpenTelemetry::SDK::Logs::Logger]
def logger(name:, version: nil)
version ||= ''
if @stopped
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this case necessary? I ask because it differs from what we're doing in the TracerProvider.

Copy link
Contributor Author

@kaylareopelle kaylareopelle Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a minute since I wrote this, but I believe I did it to adhere to this part of the shutdown protocol:

Shutdown MUST be called only once for each LoggerProvider instance. After
the call to Shutdown, subsequent attempts to get a Logger are not allowed.
SDKs SHOULD return a valid no-op Logger for these calls, if possible.

(source)

There's similar verbiage for the Trace SDK.

I'm not sure if we adhere to this part of the Trace spec elsewhere. Maybe this part does the job well enough? But I'm not sure if that would sufficiently covers an attempt to get a tracer after shutdown.

if @stopped
OpenTelemetry.logger.warn('calling Tracer#shutdown multiple times.')
return Export::FAILURE
end

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logs SDK - Add Instrumentation Registry Logs SDK - Overall Logs Configuration
2 participants