-
Notifications
You must be signed in to change notification settings - Fork 176
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
Added basic logger/log-device and tests #375
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments inline, the approach makes sense overall, though I didn't understand the reasoning for some of the details.
lib/bugsnag/loggers/logger.rb
Outdated
message = progname | ||
end | ||
if severity >= level | ||
@log_device.write(message, progname, Bugsnag::Loggers::SEVERITIES[severity]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to define your own level
? What happens if its over Logger::UNKNOWN
?
lib/bugsnag/loggers/logger.rb
Outdated
true | ||
end | ||
|
||
def reopen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will this be used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the base class it's meant to close the connection to a file or IO object. While we're not hanging onto those objects I thought it'd be appropriate to cease logging in similar circumstances
lib/bugsnag/loggers/logger.rb
Outdated
def initialize(level=Logger::INFO) | ||
@open = true | ||
@log_device = Bugsnag::Loggers::LogDevice.new | ||
super @log_device, level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the benefit of having the device over nil
here? The add()
method could call Bugsnag.leave_breadcrumb
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, there is no advantage. I'll remove this. For some reason I assumed the logger base class wouldn't be happy being passed nil, but it's quite happy.
lib/bugsnag/loggers/logger.rb
Outdated
require "logger" | ||
require "bugsnag" | ||
|
||
module Bugsnag::Loggers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any benefit to the Loggers module? Bugsnag::Logger
reads nicer. Is there a clash with our internal logger maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's going to be some shared functionality between the Logger
and the Appender
classes (Appender coming soon). I've thought about renaming the module 'logging' as it's what the module deals with, but it will contain 'loggers', so not sure about that
Added the appender in with tests, I'll get some examples added, one using |
- Move logger and appender from Bugsnag::Logging to Bugsnag::Breadcrumbs module
This is becoming part of a separate project |
Additions
Logger
can be assigned to any interface that uses a Logger, such asActiveRecord::Base.logger
orMongoid::Loggable.logger
Appender
includes trackableLogEvent
data includingfile
,line
,method