Feedback needed :)

[aserafin]

Hi,

I've just merged almost all PR's for the grape_logging and I want to thank you all for your hard work! I will be running master version of gem in my projects during next week to make sure it's stable before pushing to rubygems and it would be really great if you could do the same and post your findings here :)

@diguliu @guizmaii @marshall-lee @987poiuytrewq @yalongzhang @jtmarmon @holstvoogd @scauglog @ayanko @rngtng

[guizmaii]

It's in production, with the Rails instrumentation mecanism, at home since a long time ;)

[aserafin]

Great - glad to hear :) I did some refactoring though and fixed one bug with appending db times :) Do you think you can try using master?

[guizmaii]

I'll look at your commits and will use the master ;)

[guizmaii]

Ok for me. I don't really know if the += instead of the = in the GrapeLogging::Timings.append_db_runtime changes something.

I pushed you a small PR for code lisibility : #22

[aserafin]

It actually does - without it we are only keeping track of last query timing ;)

[TobiG77]

master is usable over latest release, good work!

[guizmaii]

When do you plan to release the v1.2.0 ?

[aserafin]

I will be pushing it today evening

[rngtng]

First, thanks for processing the PRs, great job! Second, sorry I still havn't found time to look into it in detail. I just ran a quick update and figured it includes breaking changes, e.g. .formatter is on Logger required. We use https://github.com/TwP/logging where this method is not available.

But nevertheless, go on with releasing a new version, I'd only bump it up to 2.0.0 to follow SemVer correctly. Cheers!

[aserafin]

@rngtng it should be pretty easy to make this optional - I will push update today to master and check it with your gem :)

[rngtng]

cool thx

[guizmaii]

I aslo use https://github.com/TwP/logging. .formatter is required for Logger since v1.1.2 at least. That's why I added the Rails instrumentation mecanism. So IMHO, it's not a breaking changes of this version.

If you're in a Rails app, the simplest solution is that you use the Rails instrumentation mecanism, see https://github.com/aserafin/grape_logging#logging-via-rails-instrumentation

What version of grape_logging are you using ?

[rngtng]

@guizmaii I saw that section in README. But well, we don't use rails.... Logging is such a simple, dependency less instruction, it shouldn't depend on Rails instrumentation at all. Let's keep coupling to the minimum.

We're currently on version 1.1.2. And use it successful with:

use GrapeLogging::Middleware::RequestLogger, logger: Logging.logger.root

After upgrade to latest branch this fails. When I grep the 1.1.2 code I don't see any mention of formatter besides in README.

[aserafin]

Created new issue for loggers that don't expose .formatter #23 - expect fix soon :)

[guizmaii]

@rngtng You're right, my bad. Sorry.

Can you test with my PR please ?

[rngtng]

tested, looks good thx

[987poiuytrewq]

Thanks @aserafin, @guizmaii and everyone else for getting this merged in - great job! I've been running 876ddf37ff712afd2ab60eba2da88ab6966e8ac1 for the past week in production at 75 requests per minute and it's been working great. :+1:

[guizmaii]

Thanks to you @987poiuytrewq ! A lot of the work come from you ! And thanks for the feedback :+1:

[aserafin]

Just pushed 1.2.1 to RubyGems! Thanks again for all your hard work and support :clap:

[rngtng]

nice thx

[guizmaii]

:+1: