Repository navigation
fix(logs): log discarded attributes once per record - #2463
naveedsajjdawan wants to merge 7 commits into
Conversation
kaylareopelle
left a comment
kaylareopelle
left a comment
There was a problem hiding this comment.
Hi @naveedsajjdawan! Thanks for opening this PR. I have a few comments. It also looks like there's a Rubocop offense to address.
| OpenTelemetry.handle_error(message: "Invalid log record attribute key type #{k.class} for " \ | ||
| "key #{k.inspect} on record: '#{body}'. Attribute keys must be Strings. Dropping attribute.") | ||
| log_attribute_discard("Invalid log record attribute key type #{k.class} for " \ | ||
| "key #{k.inspect} on record: '#{body}'. Attribute keys must be Strings. Dropping attribute.") |
There was a problem hiding this comment.
We've had some feedback that body shouldn't be included here.
| "key #{k.inspect} on record: '#{body}'. Attribute keys must be Strings. Dropping attribute.") | |
| "key #{k.inspect}. Attribute keys must be Strings. Dropping attribute.") |
There was a problem hiding this comment.
We're also missing a test that makes sure that attribute key warnings are only logged once per log record.
| OpenTelemetry.handle_error(message: "Invalid log record attribute value type #{v.class} for key '#{k}' " \ | ||
| "on record: '#{body}'. Dropping attribute.") | ||
| log_attribute_discard("Invalid log record attribute value type #{v.class} for key '#{k}' " \ | ||
| "on record: '#{body}'. Dropping attribute.") |
There was a problem hiding this comment.
We've had some feedback that body shouldn't be included in the message. This will keep things less verbose.
| "on record: '#{body}'. Dropping attribute.") | |
| ". Dropping attribute.") |
| return if @attribute_discard_logged | ||
|
|
||
| @attribute_discard_logged = true | ||
| OpenTelemetry.handle_error(message: message) |
There was a problem hiding this comment.
Though this is what was there before, other language implementations use OpenTelemetry.logger.warn here instead. This seems more appropriate since there isn't an error with the Logs SDK, and rather a user mismatch with our limits. Let's change this to a warn-level log.
| OpenTelemetry.handle_error(message: message) | |
| OpenTelemetry.logger.warn(message) |
| attributes: { 'a' => Class.new, 'b' => Class.new, 'c' => 'c' } | ||
| ) | ||
|
|
||
| assert_equal(1, log_stream.string.lines.length) |
There was a problem hiding this comment.
Could we add an assertion to make sure the right message is emitted?
Summary
Fixes #2351 by ensuring attribute-discard diagnostics are emitted at most once for each
LogRecord.Changes
Testing
The regression tests have been added but have not been run locally in this environment. Please check CI before merging.