-
Notifications
You must be signed in to change notification settings - Fork 247
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: Update Logger#on_emit to create LogRecords #1611
Changes from 9 commits
d4af81c
ff82039
75f6875
34538d9
1f9f902
89bf4fc
7dfc712
647e0c9
0f0e3eb
d360759
0fb4606
ab57071
be2034d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,36 +12,45 @@ class Logger | |
|
||
# Emit a {LogRecord} to the processing pipeline. | ||
# | ||
# @param timestamp [optional Float, Time] Time in nanoseconds since Unix | ||
# @param [optional Float, Time] timestamp Time in nanoseconds since Unix | ||
# epoch when the event occurred measured by the origin clock, i.e. the | ||
# time at the source. | ||
# @param observed_timestamp [optional Float, Time] Time in nanoseconds | ||
# @param [optional Float, Time] observed_timestamp Time in nanoseconds | ||
# since Unix epoch when the event was observed by the collection system. | ||
# Intended default: Process.clock_gettime(Process::CLOCK_REALTIME) | ||
# @param context [optional Context] The Context to associate with the | ||
# LogRecord. Intended default: OpenTelemetry::Context.current | ||
# @param severity_number [optional Integer] Numerical value of the | ||
# @param [optional Integer] severity_number Numerical value of the | ||
# severity. Smaller numerical values correspond to less severe events | ||
# (such as debug events), larger numerical values correspond to more | ||
# severe events (such as errors and critical events). | ||
# @param severity_text [optional String] Original string representation of | ||
# @param [optional String] severity_text Original string representation of | ||
# the severity as it is known at the source. Also known as log level. | ||
# @param body [optional String, Numeric, Boolean, Array<String, Numeric, | ||
# @param [optional String, Numeric, Boolean, Array<String, Numeric, | ||
# Boolean>, Hash{String => String, Numeric, Boolean, Array<String, | ||
# Numeric, Boolean>}] A value containing the body of the log record. | ||
# @param attributes [optional Hash{String => String, Numeric, Boolean, | ||
# Array<String, Numeric, Boolean>}] Additional information about the | ||
# event. | ||
# Numeric, Boolean>}] body A value containing the body of the log record. | ||
# @param [optional String] trace_id The trace ID associated with the | ||
# current context. | ||
# @param [optional String] span_id The span ID associated with the | ||
# current context. | ||
# @param [optional TraceFlags] trace_flags The trace flags associated | ||
# with the current context. | ||
# @param [optional Hash{String => String, Numeric, Boolean, | ||
# Array<String, Numeric, Boolean>}] attributes Additional information | ||
# about the event. | ||
# @param [optional Context] context The Context to associate with the | ||
# LogRecord. Intended default: OpenTelemetry::Context.current | ||
# | ||
# @api public | ||
def emit( | ||
def on_emit( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When we renamed the method in #1517, I forgot to go back and change the references in the API. |
||
timestamp: nil, | ||
observed_timestamp: nil, | ||
context: nil, | ||
severity_number: nil, | ||
severity_text: nil, | ||
body: nil, | ||
attributes: nil | ||
trace_id: nil, | ||
span_id: nil, | ||
trace_flags: nil, | ||
attributes: nil, | ||
context: nil | ||
) | ||
end | ||
# rubocop:enable Style/EmptyMethod | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
# frozen_string_literal: true | ||
|
||
# Copyright The OpenTelemetry Authors | ||
# | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
module OpenTelemetry | ||
module SDK | ||
module Logs | ||
# Implementation of OpenTelemetry::Logs::LogRecord that records log events. | ||
class LogRecord < OpenTelemetry::Logs::LogRecord | ||
attr_accessor :timestamp, | ||
:observed_timestamp, | ||
:severity_text, | ||
:severity_number, | ||
:body, | ||
:attributes, | ||
:trace_id, | ||
:span_id, | ||
:trace_flags, | ||
:resource, | ||
:instrumentation_scope | ||
|
||
# Creates a new {LogRecord}. | ||
# | ||
# @param [optional Float, Time] timestamp Time when the event occurred. | ||
# @param [optional Float, Time] observed_timestamp Time when the event | ||
# was observed by the collection system. If nil, will first attempt | ||
# to set to `timestamp`. If `timestamp` is nil, will set to | ||
# `Process.clock_gettime(Process::CLOCK_REALTIME, :nanosecond)`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do these accept There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I got a little too overzealous when trying to adhere to the spec. The datatype in the Logs Data Model was listed as "Type: Timestamp, uint64 nanoseconds since Unix epoch." Since the timestamp could be efficiently accessed using Based on these questions, I decided to remove the Fixes across two commits: |
||
# @param [optional String] severity_text The log severity, also known as | ||
# log level. | ||
# @param [optional Integer] severity_number The numerical value of the | ||
# log severity. | ||
# @param [optional String, Numeric, Boolean, Array<String, Numeric, | ||
# Boolean>, Hash{String => String, Numeric, Boolean, Array<String, | ||
# Numeric, Boolean>}] body The body of the {LogRecord}. | ||
# @param [optional Hash{String => String, Numeric, Boolean, | ||
# Array<String, Numeric, Boolean>}] attributes Attributes to associate | ||
# with the {LogRecord}. | ||
# @param [optional String] trace_id The trace ID associated with the | ||
# current context. | ||
# @param [optional String] span_id The span ID associated with the | ||
# current context. | ||
# @param [optional TraceFlags] trace_flags The trace flags associated | ||
# with the current context. | ||
# @param [optional OpenTelemetry::SDK::Logs::Logger] logger The logger that | ||
# created the {LogRecord}. Used to set `resource` and | ||
# `instrumentation_scope`. | ||
# | ||
# @return [LogRecord] | ||
def initialize( | ||
timestamp: nil, | ||
observed_timestamp: nil, | ||
severity_text: nil, | ||
severity_number: nil, | ||
body: nil, | ||
attributes: nil, | ||
trace_id: nil, | ||
span_id: nil, | ||
trace_flags: nil, | ||
logger: nil | ||
) | ||
@timestamp = timestamp | ||
@observed_timestamp = observed_timestamp || timestamp || Process.clock_gettime(Process::CLOCK_REALTIME, :nanosecond) | ||
@severity_text = severity_text | ||
@severity_number = severity_number | ||
@body = body | ||
@attributes = attributes.nil? ? nil : Hash[attributes] # We need a mutable copy of attributes | ||
@trace_id = trace_id | ||
@span_id = span_id | ||
@trace_flags = trace_flags | ||
@resource = logger&.resource | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know there's a method added to the sdk logger here to support this but it still reads like I wonder if there's any merit in following the same pattern we did with the tracer https://github.com/open-telemetry/opentelemetry-ruby/blob/main/sdk/lib/opentelemetry/sdk/trace/tracer.rb#L35 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a great point, following that approach cleans up the code nicely. Fixed in ab57071 |
||
@instrumentation_scope = logger&.instrumentation_scope | ||
kaylareopelle marked this conversation as resolved.
Show resolved
Hide resolved
|
||
@total_recorded_attributes = @attributes&.size || 0 | ||
end | ||
|
||
def to_log_record_data | ||
LogRecordData.new( | ||
@timestamp, | ||
@observed_timestamp, | ||
@severity_text, | ||
@severity_number, | ||
@body, | ||
@attributes, | ||
@trace_id, | ||
@span_id, | ||
@trace_flags, | ||
@resource, | ||
@instrumentation_scope, | ||
@total_recorded_attributes | ||
) | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
# frozen_string_literal: true | ||
|
||
# Copyright The OpenTelemetry Authors | ||
# | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
module OpenTelemetry | ||
module SDK | ||
module Logs | ||
# LogRecordData is a Struct containing {LogRecord} data for export. | ||
LogRecordData = Struct.new(:timestamp, # optional Integer nanoseconds since Epoch | ||
:observed_timestamp, # Integer nanoseconds since Epoch | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great point. Fixed in 0fb4606 |
||
:severity_text, # optional String | ||
:severity_number, # optional Integer | ||
:body, # optional String, Numeric, Boolean, Array<String, Numeric, | ||
# Boolean>, Hash{String => String, Numeric, Boolean, | ||
# Array<String, Numeric, Boolean>} | ||
:attributes, # optional Hash{String => String, Numeric, Boolean, Array<String, Numeric, Boolean>} | ||
:trace_id, # optional String (16-byte binary) | ||
:span_id, # optional String (8-byte binary) | ||
:trace_flags, # optional Integer (8-bit byte of bit flags) | ||
:resource, # optional OpenTelemetry::SDK::Resources::Resource | ||
:instrumentation_scope, # OpenTelemetry::SDK::InstrumentationScope | ||
:total_recorded_attributes) do # Integer | ||
def unix_nano_timestamp | ||
kaylareopelle marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if timestamp.is_a?(Time) | ||
(timestamp.to_r * 1_000_000_000).to_i | ||
else | ||
timestamp | ||
end | ||
end | ||
|
||
def unix_nano_observed_timestamp | ||
if timestamp.is_a?(Time) | ||
(timestamp.to_r * 1_000_000_000).to_i | ||
else | ||
timestamp | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
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 units are unclear here. If we accept nanoseconds, I'd expect that to be Integer rather than Float, and if we accept
Time
, then the units are not relevant. IMO we should only acceptTime
, and we should convert that to "Integer nanoseconds since Epoch" internally.Related: documenting the
Process.clock_gettime(Process::CLOCK_REALTIME)
as the default increases confusion about the units (and precision).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 points, I agree. Addressed here: #1611 (comment)