Logger improvements #1423
Open
Logger improvements #1423
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `#<<` method is meant to use the `INFO` log level. Since we do some extra transformations on `add_with_attributes` when the `#info` method is called, we can use the same code path and make it easier to understand that it's just an alias.
When the `#add` method is used instead of the level-specific helper methods, log the default attributes as well.
Before refusing to log a message because it is not a string, attempt to call `#to_s` on the message. Fixes #1418.
When broadcasting log messages, do so regardless of the log level of the AppSignal broadcaster. This allows loggers with less strict levels to log messages according to their own level thresholds. To avoid executing the block passed as a message several times, which would run into the same issues as `ActiveSupport::BroadcastLogger`, wrap the given message block with an object that can be cast into a proc and be called more than once, but that only call the underlying block once, with the same return value each time. This also ensures that we don't call the underlying block if the message is not logged by any of the broadcasted loggers. When the logger is silenced, however, do not broadcast any logs over the silenced log threshold. This makes sense because silencing is about the application code that performs the logging, not about its destination. Move the `Exception` formatting to the `#add` method so that all error levels, not just `#error`, can format exceptions. Fixes #1422.
Name the members of the permutations.
|
Hi @unflxw, We've found some issues with your Pull Request.
|
tombruijn
approved these changes
Jun 18, 2025
| @@ -73,6 +73,8 @@ def add(severity, message = nil, group = nil) | |||
| nil | |||
| end | |||
|
|
|||
| message = message.to_s if message.respond_to?(:to_s) | |||
There was a problem hiding this comment.
Most, if not all, objects respond to to_s, it's implemented on Object. It might cause us to log things like this:
>> Object.new.to_s
=> "#<Object:0x0000000128de9940>"Is that what we want? How do other loggers handle this?
There was a problem hiding this comment.
Yeah, to_s is too blunt of a tool. It's likely we should leave it up to the formatter, as mentioned in #1418 (comment) (and in the issue itself, which I misunderstood)
|
|
||
| return if message.nil? | ||
|
|
||
| message = "#{message.class}: #{message.message}" if message.is_a?(Exception) |
There was a problem hiding this comment.
Nit: Could be useful to include the first backtrace line if we're logging exceptions.
There was a problem hiding this comment.
The existing implementation explicitly chose not to do that (like, there's a test for it and everything!) but I agree that makes sense.
|
In addition to the comments above, address #1418 (comment) before merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Have #<< and #info use the same implementation
The
#<<method is meant to use theINFOlog level. Since we dosome extra transformations on
add_with_attributeswhen the#infomethod is called, we can use the same code path and make it easier
to understand that it's just an alias.
Log default attributes when #add is used
When the
#addmethod is used instead of the level-specific helpermethods, log the default attributes as well.
Call #to_s on the message
Before refusing to log a message because it is not a string, attempt
to call
#to_son the message.Fixes #1418.
Broadcast log messages regardless of level
When broadcasting log messages, do so regardless of the log level
of the AppSignal broadcaster. This allows loggers with less strict
levels to log messages according to their own level thresholds.
To avoid executing the block passed as a message several times, which
would run into the same issues as
ActiveSupport::BroadcastLogger,wrap the given message block with an object that can be cast into a
proc and be called more than once, but that only call the underlying
block once, with the same return value each time. This also ensures
that we don't call the underlying block if the message is not logged
by any of the broadcasted loggers.
When the logger is silenced, however, do not broadcast any logs over
the silenced log threshold. This makes sense because silencing is
about the application code that performs the logging, not about its
destination.
Move the
Exceptionformatting to the#addmethod so that allerror levels, not just
#error, can format exceptions.Fixes #1422.
Improve readability of logger method tests
Name the members of the permutations.