Skip to content

feat: Use logger.warn for log attribute validation #1825

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: Use logger.warn for log attribute validation #1825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5fbe2cb
feat: Use logger.warn for log attribute validation
kaylareopelle Mar 11, 2025
2e20bc5
test: Update tests for new log message
kaylareopelle Mar 11, 2025
9495553
chore: Rubocop Bundler/OrderedGems
kaylareopelle Mar 11, 2025
f57ebaa
Merge branch 'main' into log-attribute-keys
kaylareopelle Apr 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions logs_sdk/Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ source 'https://rubygems.org'

gemspec

gem 'mutex_m' if RUBY_VERSION >= '3.4'
gem 'opentelemetry-api', path: '../api'
gem 'opentelemetry-logs-api', path: '../logs_api'
gem 'opentelemetry-sdk', path: '../sdk'
Expand Down
4 changes: 2 additions & 2 deletions logs_sdk/lib/opentelemetry/sdk/logs/log_record.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,10 @@ def validate_attributes(attrs)
# Future refactor opportunity: https://github.com/open-telemetry/opentelemetry-ruby/issues/1739
attrs.keep_if do |k, v|
if !Internal.valid_key?(k)
OpenTelemetry.handle_error(message: "invalid log record attribute key type #{k.class} on record: '#{body}'")
OpenTelemetry.logger.warn("Invalid log record attribute key type #{k.class} for key #{k.inspect} on record: '#{body}'. Attribute keys must be Strings. Dropping attribute.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to produce very noisy log output.

Do we log warnings in the SDK for traces?

Copy link
Contributor Author

@kaylareopelle kaylareopelle Apr 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Great point. The Trace SDK does not use the logger to warn, instead, we handle the error like the current behavior in the logger. I wonder if updating the message for the handle_error call would be sufficient to let people know about the problem?

Here's the equivalent Trace SDK code:

opentelemetry-ruby/sdk/lib/opentelemetry/sdk/internal.rb

Lines 51 to 63 in 98c629e

def valid_attributes?(owner, kind, attrs)
attrs.nil? || attrs.each do |k, v|
if !valid_key?(k)
OpenTelemetry.handle_error(message: "invalid #{kind} attribute key type #{k.class} on span '#{owner}'")
return false
elsif !valid_value?(v)
OpenTelemetry.handle_error(message: "invalid #{kind} attribute value type #{v.class} for key '#{k}' on span '#{owner}'")
return false
end
end
true
end

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 think the main issue with the previous message was that the key was missing from the output. That made it more difficult to identify what attribute was the problem. I'm fine with keeping it as a handle_error call or using this PR to move it to a logger.warn.

I might lean more toward the handle_error approach so that people can define their own error handlers to control the output behavior.

return false
elsif !Internal.valid_value?(v)
OpenTelemetry.handle_error(message: "invalid log record attribute value type #{v.class} for key '#{k}' on record: '#{body}'")
OpenTelemetry.logger.warn("Invalid log record attribute value type #{v.class} for key '#{k}' on record: '#{body}'. Dropping attribute.")
return false
end

Expand Down
4 changes: 2 additions & 2 deletions logs_sdk/test/opentelemetry/sdk/logs/log_record_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,14 @@
it 'emits an error message if attribute key is invalid' do
OpenTelemetry::TestHelpers.with_test_logger do |log_stream|
logger.on_emit(attributes: { a: 'a' })
assert_match(/invalid log record attribute key type Symbol/, log_stream.string)
assert_match(/Invalid log record attribute key type Symbol/, log_stream.string)
end
end

it 'emits an error message if the attribute value is invalid' do
OpenTelemetry::TestHelpers.with_test_logger do |log_stream|
logger.on_emit(attributes: { 'a' => Class.new })
assert_match(/invalid log record attribute value type Class/, log_stream.string)
assert_match(/Invalid log record attribute value type Class/, log_stream.string)
end
end

Expand Down
Loading