Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Create Logs SDK LoggerProvider #1517
feat: Create Logs SDK LoggerProvider #1517
Changes from 1 commit
de7f36c
d63a4e9
5528277
37befe6
ca7c15f
a9f9b77
4313b3e
a619d36
336ca7d
5e2d8a3
f2486f7
75792a6
aba959f
d72c982
5f7d674
da89235
b7e6d8c
6c23382
44bba16
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This matches the pattern in OpenTelemetry::SDK::Trace::Export
The ExportError class is not present because it was not needed for the LoggerProvider. It will be added once it's used in subsequent PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should also be result codes for exporters - their use shouldn't just relate to these two methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point. Updated to be more general in: d63a4e9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is a boilerplate/placeholder so the LogRecordProcessor methods can be called. They will be implemented in a later PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the "resolved Context" here? Who does the resolution? (Note that I haven't read this part of the spec or all of this PR, or the logs API, so maybe this is answered clearly somewhere else - just asking as a naive reviewer.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "resolved Context" here is coming from the Logs SDK spec description for the method arguments.
https://opentelemetry.io/docs/specs/otel/logs/sdk/#onemit
The description has a bit more detail to include that might clear things up. I've updated the description to include the rest of the text in: 5528277
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure who does the resolution. I'm also not quite sure how we'll use this argument.
I'll consult some of the implementations in other languages and come back with more info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fbogsany - I can't find any examples where the
context
argument is used in other languages.onEmit
functions.So I don't have a great answer about who does the resolution. I didn't end up needing this argument in the Simple or Batch processors because I used the SpanContext to pull
trace_id
,span_id
, andtrace_flags
. Those are the only context-related attributes in the Logs Data Model.Do you have a sense of what approach you'd prefer Ruby take?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Logger SDK will be fully written in a separate PR. The code here represents what's necessary to fulfill the LoggerProvider SDK spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why we expose these attributes. In the
Tracer
andMeter
, they're private, so the inconsistency is a little weird.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
attr_reader
call has been removed. I'm not sure why I initially broke fromTracer
andMeter
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works toward the SDK Logger Provider => Logger Creation spec fulfillment
https://opentelemetry.io/docs/specs/otel/logs/sdk/#logger-creation
I didn't put any restrictions on initializing a new Logger in the code, but they are present in the documentation. I'm open to suggestions on how to limit Logger creation to only the
LoggerProvider#logger
method.Name validation logic is handled in
LoggerProvider#logger
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
@api private
comment is our standard way of marking "internal" methods. There is no practical way to enforce it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specs for SDK LoggerProvider and SDK TracerProvider are extremely similar.
Given this, the code is very similar to the already implemented TracerProvider in the SDK gem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log_record_processors
need safe concurrent access. By exposing this as a reader, external callers may gain access to the array in an unsafe way.This attribute should not be exposed as a public value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also not exposed in the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might notice that limits are not included in the parameters to initialize a LoggerProvider. This is because the limits are implemented in issue #1516.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Providing the
log_record_processors
at creation time is MAY in the spec. We don't do that in theMeterProvider
(formetric_readers
) or theTracerProvider
(forspan_processors
). Instead, we rely on theadd_...
methods to add them individually. I would start with not exposing it during creation. We can always add it later if that's a common pattern (and then consider adding toTracerProvider
andMeterProvider
as well).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to remove the arg from the initialize method. I'll double-check about its commonality in other languages. I've also removed the public reader. See: 37befe6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Resource can be passed as an arg. If none is supplied, a new Resource will be created on initialization.
The Log Record Definition includes
Resource
as an attribute. The LogRecord class and the act of emitting log records from a logger are not in the scope of this PR.The statement "it SHOULD be associated with all the LogRecords produced by any Logger from the LoggerProvider" will be addressed in a subsequent PR. The groundwork laid here enables a logger to access the logger provider's resource:
logger.instance_variable_get(:@logger_provider).instance_variable_get(:@resource)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This signature is inconsistent between
TracerProvider
andMeterProvider
. In the latter, we used:so
name
is required andversion
is a keyword arg. IIRC we're going to have to add one or two more args at some point 🔜 , so keywords for optional args are vastly preferred.We should go back and fix this for
TracerProvider
. I think that can be done in a backwards-compatible fashion.For
MeterProvider
, we opted to requirename
. If a user explicitly passesnil
or""
, we can handle it, but those are both considered invalid - I'd rather have an explicitly invalid required arg than an invalid default arg.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great. I prefer the keyword args, I just defaulted to what was in TracerProvider since that was already stable.
Updated in: ca7c15f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source
Every logger instance has a reference to the logger provider that created it. (see L50) This gives loggers access to the log record processors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thepush
method alone couldn't deliver? Or is there a different reasondup.push
was used?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm uncertain, but @fbogsany probably knows. I think it has to do with mutating the
processors
array. Looking atTracerProvider
a span gets a reference toprocessors
on start. If a span processor is added after a span is started, and theprocessors
array is mutated, a span will see a processor that wasn't there when it was started. I'm not sure if this would happen in the real world, or if this is what we're protecting against, but that is a reason why we might preferdup.push
overpush
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's exactly the reason - each SDK
Span
instance has a reference to the list of span processors, and that list should not change between the span callingon_start
andon_finish
. IDK if that applies in your case - I need to read the rest of the PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth considering whether it should be valid to add log record processors after
@stopped
. In theTracerProvider
, we warn and don't allow that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: https://opentelemetry.io/docs/specs/otel/logs/sdk/#shutdown
@stopped
totrue
L92OpenTelemetry::SDK::Logs::EXPORT
constants (L81, L87, L93)OpenTelemetry::Common::Utilities.timeout_timestamp
andOpenTelemetry::Common::Utilities.maybe_timeout
(L84, L86)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the
TracerProvider
andMeterProvider
we expect the processors/readers to notraise
from#shutdown
or#force_flush
and instead only return one of the 3 supported result codes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. Fixed in: a619d36
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating dependencies to more recent versions. This came about when I realized the SimpleCov version that was installed didn't include a feature I wanted to use.