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: Update Logger#on_emit to create LogRecords #1611

Merged
merged 13 commits into from
Jul 25, 2024

Conversation

kaylareopelle
Copy link
Contributor

This PR creates a LogRecord class that is read/writeable to start with, and becomes read-only when it transitions to LogRecordData.

Additionally, it provides the SDK implementation of the Logs Bridge API Logger#on_emit method to emit LogRecords to the processing pipeline.

Relates to the following specs:

Resolves: #1484

Copy link
Contributor

github-actions bot commented May 5, 2024

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

@github-actions github-actions bot added the stale label May 5, 2024
Previously, trace_id, span_id, and trace_flags were assigned based on
the OpenTelemetry::Trace.current_span's SpanContext. The specification
requires a context be accepted as an argument on emit.

If no context is provided, OpenTelemetry::Context.current will be used.

If there isn't a current span on OpenTelemetry::Context.current
trace_id, span_id, and trace_flags will be nil unless they are passed
as arguments.

Also, remaining #emit methods are renamed to #on_emit.
#
# @api public
def emit(
def on_emit(
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

This code was leftover and should have been removed.
Now, the specfic `trace_id`, `span_id`, and `trace_flags`
attributes are used instead.
@trace_id = trace_id
@span_id = span_id
@trace_flags = trace_flags
@resource = logger&.resource
Copy link
Contributor

Choose a reason for hiding this comment

The 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 logger.logger_provider.resource to me.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Comment on lines 11 to 12
LogRecordData = Struct.new(:timestamp, # optional Integer nanoseconds since Epoch
:observed_timestamp, # Integer nanoseconds since Epoch
Copy link
Contributor

Choose a reason for hiding this comment

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

The timestamp and observed_timestamp are documented here to be "Integer nanoseconds since Epoch", so there should be no need for the unix_nano_timestamp and unix_nano_observed_timestamp methods below.

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 point. Fixed in 0fb4606

Comment on lines 26 to 30
# @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)`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do these accept Float? They should only accept Time, like corresponding methods in the tracing API. This makes it easier to ensure the correctness of conversion to "Integer nanoseconds since Epoch" internally, and reduces confusion about the units.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Process.clock_gettime(Process::CLOCK_REALTIME, :nanosecond), I decided to add it and make it the default.

Based on these questions, I decided to remove the Float option, go all in on the Time object and transform it into a nanosecond Integer in LogRecord#to_log_record_data.

Fixes across two commits:

Comment on lines 15 to 20
# @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)
Copy link
Contributor

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 accept Time, 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).

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 points, I agree. Addressed here: #1611 (comment)

Copy link
Contributor

github-actions bot commented Jul 5, 2024

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

Convert Time to the appropriate nanosecond format in LogRecordData.
Remove Float option from argument declaration.
The LogRecordData definition says the time is already
Integer nanoseconds since epoch, so move the logic to convert
the timestamp to LogRecord#to_log_record_data
* Follow the pattern in TracerProvider#internal_start_span
* Update spacing in LogRecordData
* Remove resource references outside LoggerProvider
* Remove Logger attr_readers for logger_provider and
  instrumentation_scope
@kaylareopelle
Copy link
Contributor Author

@robertlaurin, @fbogsany - Thanks for your feedback! I've been able to return to this project and addressed your comments. This PR is ready for another review.

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

LGTM. @fbogsany can you confirm that your changes have been addressed?

@mwear
Copy link
Member

mwear commented Jul 23, 2024

I'll plan to merge this EOD Thursday if there aren't any objections.

@mwear mwear merged commit 309de20 into open-telemetry:main Jul 25, 2024
56 checks passed
@kaylareopelle kaylareopelle deleted the logs-sdk-log-record branch October 11, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logs SDK - LogRecord
5 participants