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

Adding a breadcrumb logger #373

Closed
wants to merge 24 commits into from
Closed

Conversation

Cawllec
Copy link
Contributor

@Cawllec Cawllec commented Sep 26, 2017

Additions

  • Adds a logger to track events output to a logger to breadcrumbs
  • BugsnagLogger can be assigned to any interface that uses a Logger, such as ActiveRecord::Base.logger or Mongoid::Loggable.logger
  • Breadcrumb logging threshold can be set during initialisation or at any point afterwards
  • MultiLogger accepts an array of loggers on creation and will output to each of them on message receipt, allowing user to attach other loggers alongside BugsnagLogger

Discussion

  • Considering changing name to BreadcrumbLogger to be more explicit about functionality
  • I created this as a separate PR to the breadcrumbs PR to avoid splitting focus. I think the breadcrumb feature should be completed before this

TODO

  • Needs tests added
  • Needs examples/documenting

@Cawllec Cawllec added needs discussion Requires internal analysis/discussion needs documentation labels Sep 26, 2017
@Cawllec
Copy link
Contributor Author

Cawllec commented Sep 27, 2017

Tests are now added as well as a few updates

@@ -0,0 +1,85 @@
module Bugsnag::Loggers
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think we should implement this. We can recommend another one if we want, but its out of the scope of our lib imo

"fatal"
]

class BugsnagLogger
Copy link
Contributor

Choose a reason for hiding this comment

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

we can call this logger and put it in the bugsnag module if we get rid of multi


attr_reader :level

def initialize(level="info")
Copy link
Contributor

Choose a reason for hiding this comment

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

We havent inherited from logger which is potentially a better idea. I looked at some things. Found log4r which might be a good thing to use? We could also instead build an appender? http://www.rubydoc.info/gems/logging/Logging/Appenders

That way we dont have to even worry about multi loggers. I havent looked into this deeply at all however!

Copy link
Contributor Author

@Cawllec Cawllec Oct 3, 2017

Choose a reason for hiding this comment

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

I'll subclass Appender for the same goal in a different branch. If we'd prefer that I'll close this one out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I didn't notice when I last checked that Appenders doesn't belong to Stdlib. Personally I'd prefer to implement an stdlib solution with examples for other common third parties. I can remove multi-logger and re-tool Bugsnag-Logger into just Logger.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's two sets of users here that we can support:

  • folks using the stdlib logger
  • folks using other loggers (e.g. logging and log4r gems)

For the first group, the simplest solution seems to be subclassing Logger, since it already provides log levels and filtering by level. In that case the implementation only needs to map levels to severities and make breacrumbs/reports from the log() method when its invoked.

For the folks using something other than the stdlib Logger, the logging gem seems to be the most used and maintained, as log4r hasn't seen a release in the last 7ish years. We'd need an appender for this and a check that the Logging class exists to activate it. The appender and the stdlib Logger subclass could share the same logic, potentially, depending on what an appender needs to do.

Copy link
Contributor Author

@Cawllec Cawllec Oct 3, 2017

Choose a reason for hiding this comment

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

stdlib includes a LogDevice which seems to be equivalent to an Appender in the Logging lib. I think targeting these two and allow users to configure the actual logger may be the best solution.

  • Edit -
    LogDevice doesn't seem to support being handed log - levels, making it a non-starter. I'll do what you suggested above @kattrali

@Cawllec
Copy link
Contributor Author

Cawllec commented Dec 21, 2017

I think we're in favour of #375 more than this, so I'll close this for now.

@Cawllec Cawllec closed this Dec 21, 2017
@Cawllec Cawllec deleted the cawllec/db-breadcrumbs branch February 7, 2018 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Requires internal analysis/discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants