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

Logger dumps error with classname #899

Merged
merged 4 commits into from
Apr 20, 2016

Conversation

tagomoris
Copy link
Member

@tagomoris tagomoris commented Apr 15, 2016

In these days, many plugin authors (and core developers) writes logging code like below many times.

log.error "my message", error_class: e.class, error: e.to_s, other: attributes

Fairly speaking, this is waste of time and key typing.

Fluentd will log errors like below with this change by just specifying error: e:

log.error "my message", error: e
# [error]: my message error=MyErrorClass[message of e]

@tagomoris tagomoris force-pushed the logger-dumps-error-with-classname branch from 0d02974 to a720cde Compare April 19, 2016 08:53
@tagomoris
Copy link
Member Author

@repeatedly @sonots How do you think about this change?

@sonots
Copy link
Member

sonots commented Apr 19, 2016

I am positive for specifying only an error object, but somebody might be watching logs of fluentd by a log monitoring system for alerting. Changing log format would break compatibilities for their system, so I think we should keep compatibilities of log format.

@tagomoris
Copy link
Member Author

@sonots Do you mean that we should print errors in format below?

error_class=MyErrorClass error="message of e"

@sonots
Copy link
Member

sonots commented Apr 19, 2016

@tagomoris yes!

@tagomoris
Copy link
Member Author

@sonots Hmm, it looks a bit verbose for me, and v0.14 may be a good time to do that kind of change.
@repeatedly How do you think?

@repeatedly
Copy link
Member

Keeping compatibility is good for existing users. Maybe, asking it on mailing list is better.

v0.14 may be a good time to do that kind of change.

This change affects v0.12 because we will backport several changes to v0.12 and skipping this patch is hard...
If you want to change error message format, we should suspend this patch until just before v0.14 release.

@tagomoris
Copy link
Member Author

Okay, I'll change format of errors.

@tagomoris
Copy link
Member Author

Done.

@@ -292,7 +292,11 @@ def event(level, args)
}

map.each_pair {|k,v|
message << " #{k}=#{v.inspect}"
if k == "error".freeze
message << " error_class=#{v.class.to_s} error=#{v.to_s.inspect}"
Copy link
Member

Choose a reason for hiding this comment

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

v would be a string like

$log.error "error on output thread", error: $!.to_s
, so need to add and v.is_a?(Exception).

Copy link
Member

Choose a reason for hiding this comment

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

Need to add another condition !map.has_key?('error_class'), otherwirse, error_class= appears twice for old codes such as:

log.error 'message', error_class: e.class, error: e

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right. I fixed so.

@sonots
Copy link
Member

sonots commented Apr 19, 2016

LGTM!

@tagomoris tagomoris merged commit 1b6f9e6 into master Apr 20, 2016
@tagomoris
Copy link
Member Author

Merged.

@tagomoris tagomoris deleted the logger-dumps-error-with-classname branch May 17, 2016 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants