feat: Create Logs SDK LoggerProvider #2
Conversation
kaylareopelle
changed the title
| if @stopped | ||
| OpenTelemetry.logger.warn('LoggerProvider#shutdown called multiple times.') | ||
| return OpenTelemetry::SDK::Logs::Export::FAILURE | ||
| end |
There was a problem hiding this comment.
The code for LoggerProvider#shutdown and LoggerProvider#force_flush are drawn mainly from TracerProvider methods of the same names.
Why does #shutdown return a failure status code if called when @stopped is true, but #force_flush returns a success status code?
JS, for example, does similar things for both #force_flush and #shutdown.
The specs on #force_flush (very similar for Logs and Traces), don't describe the desired behavior in cases where the method is called, but the provider has stopped/shutdown.
There was a problem hiding this comment.
Also, the spec states:
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
This is also true for TracerProvider#shutdown (source). Should I update this code in the Logs SDK to return OpenTelemetry::Logs::LoggerProvider::NOOP_LOGGER? If so, should action be taken on the Trace SDK?
I think we should keep the Logs SDK and the Trace SDK as similar as possible. Some things like this might be better handled as issues to tackle in both libraries after the initial implementation. I'm open to keeping things in sync for now and making dual changes later on.
| rescue StandardError => e | ||
| OpenTelemetry.handle_error(exception: e, message: FORCE_FLUSH_ERROR) | ||
| Export::FAILURE | ||
| end |
There was a problem hiding this comment.
There is not a rescue in the TracerProvider#force_flush method, but the method documentation states "Export::FAILURE should be returned if a non-specific failure occurred." I'm assuming a non-specific failure would be any StandardError instance.
- Is there a condition to return
Export::FAILUREon non-specific errors inTracerProvider#force_flushthat I'm missing? - Similar documentation exists for the
LoggerProvider/TracerProvider#shutdownmethods. If we choose to add arescuehere, should the#shutdownmethods also have a similarrescue?
| # LogRecordProcessor to add. | ||
| def add_log_record_processor(log_record_processor) | ||
| @mutex.synchronize do | ||
| @log_record_processors = @log_record_processors.dup.push(log_record_processor) |
There was a problem hiding this comment.
This is copied from TracerProvider#add_span_processor. @fallwith and I were chatting about this LOC. He was asking about the use of .dup.push here.
Does dup.push provide atomicity in a way that the push method alone couldn't deliver? Or is there a different reason dup.push was used?
|
Closed in favor of otel-ruby 1517 |
Fulfills the Logs SDK specs for Logger Provider
The Logger Provider has a lot of similarities with the already implemented Tracer Provider, as they have very similar specs.