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

Add ContextLogger that accepts data as second argument to log method #79

Closed
wants to merge 1 commit into from

Conversation

bliof
Copy link
Contributor

@bliof bliof commented Aug 29, 2016

This PR tries to resolve #77

ContextLogger.info('test')
ContextLogger.info('test', user_id: 123, test_arg: 12)
ContextLogger.info('test') { 'test message' }
ContextLogger.info('test') { ['test message', test_arg: 1235] }

In rails the ContextLogger is returned by Rails.logger. I think that this is not ok and we should return that logger when doing Loga.logger and return a normal logger when doing Rails.logger. Comments :) ?

@bliof bliof added the 🚧 wip label Aug 29, 2016
@bliof
Copy link
Contributor Author

bliof commented Aug 29, 2016

@timstott @FundingCircle/prod-ops 👀

@bliof bliof changed the title Add ContextLogger that excepts data as second argument to log method Add ContextLogger that accepts data as second argument to log method Aug 29, 2016
@bliof bliof force-pushed the logging-with-context branch from c414b93 to d9d4d50 Compare August 29, 2016 14:15
@bliof bliof changed the title Add ContextLogger that accepts data as second argument to log method [PO-1925] Add ContextLogger that accepts data as second argument to log method Aug 29, 2016
@@ -46,25 +46,29 @@ def compute_service_version
end

def initialize_logger
unless device
raise 'Please specify device for the Loga logger'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to raise an exception considering we're logging the error below?

Copy link
Contributor Author

@bliof bliof Sep 21, 2016

Choose a reason for hiding this comment

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

Otherwise it will be NoMethodError on the next line.

https://github.com/FundingCircle/loga/blob/master/spec/unit/loga/configuration_spec.rb#L50

Is there a reason not to default to e.g. device ||= $stdout here?

@timstott
Copy link
Contributor

timstott commented Sep 6, 2016

@bliof it's looking good. I'm going to do some additional review locally 🙇

@bliof bliof force-pushed the logging-with-context branch from d9d4d50 to f0521a9 Compare September 15, 2016 08:20
@0mega 0mega mentioned this pull request Sep 15, 2016
@sgerrand sgerrand changed the title [PO-1925] Add ContextLogger that accepts data as second argument to log method Add ContextLogger that accepts data as second argument to log method Sep 15, 2016
@bliof bliof force-pushed the logging-with-context branch from f0521a9 to 6f4ab4b Compare September 17, 2016 14:07
@bliof bliof force-pushed the logging-with-context branch from 6f4ab4b to f80a498 Compare October 10, 2016 08:13
@bliof
Copy link
Contributor Author

bliof commented Oct 10, 2016

@timstott 💭 Logging to $stdout by default?

@bliof bliof force-pushed the logging-with-context branch from 3b754c7 to 8e11ee6 Compare October 10, 2016 10:36
@bliof bliof removed the 🚧 wip label Oct 10, 2016
@bliof bliof force-pushed the logging-with-context branch 2 times, most recently from d1a276f to a6a46fc Compare October 10, 2016 11:07
@bliof bliof force-pushed the logging-with-context branch 4 times, most recently from 7f8f148 to 5bf0458 Compare November 1, 2016 15:55
@bliof
Copy link
Contributor Author

bliof commented Nov 1, 2016

There are some special fields :exception, :timestamp, :type in Loga::Event. At the moment if you write something like Loga.logger.info "this is a test", type: 'my cool test', the type will go under :data. And if you need to modify any of them you'll have to build the Loga::Event manually.

https://github.com/FundingCircle/loga/blob/master/lib/loga/event.rb#L3

ContextLogger.info('test')
ContextLogger.info('test', user_id: 123, test_arg: 12)
ContextLogger.info('test') { 'test message' }
ContextLogger.info('test') { ['test message', test_arg: 1235] }
@bliof bliof force-pushed the logging-with-context branch from f3bf9fe to 76090d1 Compare December 15, 2016 15:10
@bliof
Copy link
Contributor Author

bliof commented Jan 3, 2017

@timstott What are we going to do with this PR? :) 👍 / 👎

@timstott
Copy link
Contributor

timstott commented Jan 4, 2017

@bliof while I understand and support the ambition of this PR, I am not keen on introducing backwards incompatible change. Removing Loga from an application should only be the matter of removing the dependency in the Gemfile and the configuration.
I am in favour of closing the PR and going back to the drawing board.

@bliof bliof closed this Jan 18, 2017
@bliof
Copy link
Contributor Author

bliof commented Jan 18, 2017

@timstott You have to send me a photo of that board :D

Here are 2 ideas:

  1. the gem could provide a more advanced logger which will not be used by default
  2. advanced logger which will be used by default for Loga.logger but not like Rails.logger

@bliof bliof deleted the logging-with-context branch June 8, 2017 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve passing context to logger
2 participants