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

Added LeveledFormatLogger (Printf-like logger) #101

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

juanfresia
Copy link

Hi everyone!

I was having a hard time trying to use logrus logger interface to work with go-retryablehttp, having no other choice that to implement a wrapper to align the two interfaces. Also, I was not the first to attempt stumble upon this (see #93).

Changes

So I tried to add support in go-retryablehtp for interfaces like logrus logger, which uses the Printf family of functions to output formatted logs (there is one for each level: Infof, Debugf, etc).

The change is generic (it is not tied to logrus), and it only adds a new logger interface called LeveledFormatLogger, which is similar to LeveledLogger but uses the Printf style for formatting and adding arguments to the logs.

A wrapper was provided to use the new interface as a simple Logger (hookFormatLogger), and all log lines have now an alternative with LeveledFormatLogger.

Example

Here is an example of the output with the changes:

DEBUG GET http://localhost:8081/test: performing request 
ERROR GET http://localhost:8081/test: request failed: Get "http://localhost:8081/test": dial tcp [::1]:8081: connect: connection refused 
DEBUG GET http://localhost:8081/test: retrying request in 1s (10 left) 
ERROR GET http://localhost:8081/test: request failed: Get "http://localhost:8081/test": dial tcp [::1]:8081: connect: connection refused 
DEBUG GET http://localhost:8081/test: retrying request in 2s (9 left) 
ERROR GET http://localhost:8081/test: request failed: Get "http://localhost:8081/test": dial tcp [::1]:8081: connect: connection refused 
DEBUG GET http://localhost:8081/test: retrying request in 4s (8 left) 

As opposed to the same output while using logrus as a default Logger

INFO [DEBUG] GET http://localhost:8081/test       
INFO [ERR] GET http://localhost:8081/test request failed: Get "http://localhost:8081/test": dial tcp [::1]:8081: connect: connection refused 
INFO [DEBUG] GET http://localhost:8081/test: retrying in 1s (10 left) 
INFO [ERR] GET http://localhost:8081/test request failed: Get "http://localhost:8081/test": dial tcp [::1]:8081: connect: connection refused 
INFO [DEBUG] GET http://localhost:8081/test: retrying in 2s (9 left) 
INFO [ERR] GET http://localhost:8081/test request failed: Get "http://localhost:8081/test": dial tcp [::1]:8081: connect: connection refused 
INFO [DEBUG] GET http://localhost:8081/test: retrying in 4s (8 left) 

This is what I'm currently using, and given it is a small change I thought it could be interesting to bring it upstream. Let me know what you think!

Signed-off-by: Juan Manuel Fresia juanmanuelfresia@gmail.com

@hashicorp-cla
Copy link

hashicorp-cla commented Jun 14, 2020

CLA assistant check
All committers have signed the CLA.

@jefferai
Copy link
Member

LGTM but needs CLA signed and would like @evanphx to take a look as well since he does more maintaining here.

Signed-off-by: Juan Manuel Fresia <juanmanuelfresia@gmail.com>
Signed-off-by: Juan Manuel Fresia <juanmanuelfresia@gmail.com>
Signed-off-by: Juan Manuel Fresia <juanmanuelfresia@gmail.com>
Signed-off-by: Juan Manuel Fresia <juanmanuelfresia@gmail.com>
Signed-off-by: Juan Manuel Fresia <juanmanuelfresia@gmail.com>
@juanfresia
Copy link
Author

Thanks @jefferai, I've signed the CLA, and rebased the changes to master.

@alexandrevilain
Copy link

Hi :)

Any news on this Pull Request merge ETA ?

@juanfresia
Copy link
Author

Maybe @evanphx could take a look?

@evanphx
Copy link
Contributor

evanphx commented Nov 13, 2020

I suppose this is fine, though I worry about having this massive footprint of different logging interfaces embedded in a simple package.

@greut
Copy link

greut commented Nov 28, 2020

w.r.t logrus we did that to turn it into a LeveledLogger https://play.golang.org/p/yQ1io2eYJWK

many libraries have logr compatible interface, which would be yet another interface.

@martinsirbe
Copy link

Hi, any plans of getting this merged anytime soon? :)

jasoncodes added a commit to ennova/gotenberg that referenced this pull request Feb 15, 2021
jasoncodes added a commit to ennova/gotenberg that referenced this pull request Mar 9, 2021
@sylr
Copy link

sylr commented Dec 8, 2021

I'm also interested in this.

@StarpTech
Copy link

@ryanuber Any chance to merge and publish it?

@achuchev
Copy link

Any news on this?

@evanphx evanphx requested a review from ryanuber February 22, 2022 18:31
@crashiura
Copy link

Hi @ryanuber could take a look?

@akashsinghal
Copy link

Any update on this? Would be very helpful

@VaismanLior
Copy link

Hey, would be helpful to have this support.
Any plans to merge this PR?

@thapabishwa
Copy link

cc @ryanuber any eta on this?

@kedare
Copy link

kedare commented Sep 22, 2023

Same I am also interested, so far we disable completely the logging on this lib as it's unuseable.

@jwlv
Copy link

jwlv commented Nov 15, 2024

This looks like a pretty simple change that would help a lot of people. Any chance of it getting merged?

@greut
Copy link

greut commented Nov 15, 2024

@jwlv @kedare etc. did you try the little adapter posted just above? #101 (comment)

@jwlv
Copy link

jwlv commented Nov 27, 2024

@jwlv @kedare etc. did you try the little adapter posted just above? #101 (comment)

@greut thanks for the pointer. It worked great!

@kedare
Copy link

kedare commented Nov 27, 2024

Unfortunately I don't work with go anymore :(

@alexrojco
Copy link

I am really interested in having this added!

@syzyfus
Copy link

syzyfus commented Dec 3, 2024

Would be helpful if this was eventually merged :)

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.