feat: Create additional log record interfaces for SDK #3
Conversation
| severity_number: nil, | ||
| body: nil, | ||
| attributes: nil, | ||
| logger: nil |
There was a problem hiding this comment.
- JS uses the logger to get to the instrumentation scope and resource
- I think PHP uses the logger for the resource, but not for the instrumentation scope
- Java seems to set them based on what's passed in
- Python (approx location) seems to accept args for resource and gets the scope through the LogData object
I'm tempted to follow JS here and pass in the logger to get those attributes, but I'm open to other ideas.
| module OpenTelemetry | ||
| module SDK | ||
| module Logs | ||
| # A {LogRecord} interface that can write to the full {LogRecord} and |
There was a problem hiding this comment.
Fix these LogRecord references to link to the API. This may need to wait until the API is published on rubygems/rdoc
| @resource = logger&.resource | ||
| @instrumentation_scope = logger&.instrumentation_scope |
There was a problem hiding this comment.
According to the Logs Data Model, every attribute on a log record is optional, with the exception of observed_timestamp. This makes a logger not required, since resource and instrumentation_scope do not need to be present.
Though one could argue instrumentation_scope is required since the "optional" references in the data model field description are for the name and version attributes on the instrumentation scope.
If we come to the conclusion that instrumentation_scope is required, we should probably make logger required and remove the safe navigation operators.
| # @param [optional Float, Time] timestamp Time when the event occurred. | ||
| # @param [optional Float, Time] observed_timestamp Time when the event |
There was a problem hiding this comment.
Float - The Process.clock_gettime return value's class
Time - The documented param class for timestamp on a Span is the Time class
Is there a preference for the type of object passed here? My guess would be Float/Process.clock_gettime as it seems that many gems replaced their calls to Time.now with Process.clock_gettime in 2021's 1.0 release.
There was a problem hiding this comment.
💡 Remember -- Any changes to ReadWriteLogRecord should be made to ReadableLogRecord too!
| logger: nil | ||
| ) | ||
| @timestamp = timestamp | ||
| @observed_timestamp = observed_timestamp || timestamp || Process.clock_gettime(Process::CLOCK_REALTIME) |
There was a problem hiding this comment.
This one feels a bit tricky...
Observed Timestamp. If unspecified the implementation SHOULD set it equal to the current time.
source
Logs Data Model observed timestamp description for comparison
This leads me to think the order of assignment would be as described above. I could also see a scenario where observed_timestamp is not available as an argument on initialize and can only be created from timestamp or Process.clock_gettime. Thoughts?
| ) | ||
| @timestamp = timestamp | ||
| @observed_timestamp = observed_timestamp || timestamp || Process.clock_gettime(Process::CLOCK_REALTIME) | ||
| @span_context = span_context |
There was a problem hiding this comment.
The Logs Data Model liststrace_id, span_id, and trace_flags as separate attributes. These attributes are all present in an OpenTelemetry::SDK::Trace::SpanContext object.
The Logs SDK spec says:
The trace context fields MUST be populated from the resolved Context (either the explicitly passed Context or the current Context) when emitted."
Each language treats this a little differently:
What makes the most sense for Ruby? I'm leaning toward SpanContext and adding the current span context as the default. Is there a way to get the current span context? I've found a way to get the current Context via Context.current, but Context doesn't seem quite right since it doesn't necessarily have trace_id, span_id, and trace_flags as far as I can tell.
|
Next steps:
|
|
Ultimately went in a different direction for this work. |
Implements the ReadableLogRecord and ReadWriteLogRecord as described in the Logs SDK specs and the Logs Data Model.
It also adds a
SeverityNumberclass to the API. I noticed many other implementations of the logs signal have this and there is reference to this class in the documentation for the additional log record interfaces. The numbers correspond to the references in the Logs Data Model. I'm happy to break this out into a separate PR there's a preference to keep all work on the SDK and API separate.