wagslane / go-rabbitmq

A wrapper of streadway/amqp that provides reconnection logic and sane defaults
https://blog.boot.dev/golang/connecting-to-rabbitmq-in-golang-easy/
MIT License
772 stars 126 forks source link

Add log level to Printf method of the Logger interface #72

Closed metalrex100 closed 2 years ago

metalrex100 commented 2 years ago

Currently it is not possible to define which log-level message has: info or warning or error. Need to extend Printf method with logLevel variable. I can suggest PR.

It's a breaking change, but don't really see how such feature can be added without breaking change.

wagslane commented 2 years ago

You can define your own custom logger, I think that's the thing to do. We send all logs through there, and then the user can choose too suppress them how they wish.

metalrex100 commented 2 years ago

@wagslane for example I want to show only logs with errors. How do I know in my Sprintf implementation if message containing error or just some info?

wagslane commented 2 years ago

To not make breaking API changes, we could just add all the prefixes to the strings we log.

On Sat, Apr 23, 2022, 11:26 AM metalrex100 @.***> wrote:

@wagslane https://github.com/wagslane for example I want to show only logs with errors. How do I know in my Sprintf implementation if message containing error or just some info?

— Reply to this email directly, view it on GitHub https://github.com/wagslane/go-rabbitmq/issues/72#issuecomment-1107538579, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFHZKVOI7TUFEJ2KORAXGCTVGQXFXANCNFSM5UE6PSAQ . You are receiving this because you were mentioned.Message ID: @.***>

metalrex100 commented 2 years ago

@wagslane so you suggest to rewrite logs with prefixes with log level? Before:

chManager.logger.Printf("attempting to reconnect to amqp server from error: %v", err)
chManager.logger.Printf("successfully reconnected to amqp server")

After:

chManager.logger.Printf("[Error]: attempting to reconnect to amqp server from error: %v", err)
chManager.logger.Printf("[Info]: successfully reconnected to amqp server")

?

So in my Printf implementation I will have to define message log-level by searching substring? Possible solution, but guess you also understand that not really good.

wagslane commented 2 years ago

Yeah, I don't love it either. I'll think about it some more

On Sat, Apr 23, 2022, 11:35 AM metalrex100 @.***> wrote:

@wagslane https://github.com/wagslane so you suggest to rewrite logs with prefixes with log level? Before:

chManager.logger.Printf("attempting to reconnect to amqp server from error: %v", err) chManager.logger.Printf("successfully reconnected to amqp server")

After:

chManager.logger.Printf("[Error]: attempting to reconnect to amqp server from error: %v", err) chManager.logger.Printf("[Info]: successfully reconnected to amqp server")

?

So in my Printf implementation I will have to define message log-level by searching substring? Possible solution, but guess you also understand that not really good.

— Reply to this email directly, view it on GitHub https://github.com/wagslane/go-rabbitmq/issues/72#issuecomment-1107540018, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFHZKVPAA2NDVKJGFRLFAWLVGQYGXANCNFSM5UE6PSAQ . You are receiving this because you were mentioned.Message ID: @.***>

metalrex100 commented 2 years ago

@wagslane I see here 2 possible solutions:

  1. Add type LogLevel with constants and change Logger's interface method Sprintf to accept LogLevel type parameter:
    
    type LogLevel string

const ( Debug = "debug" Info = "info" Warning = "warning" Error = "error" )

type Logger interface { Printf(LogLevel, string, ...interface{}) }

The big question here is to have `...interface{}` variadic param, because inside my `Printf` implementation I don't really know what will it contain and how I should handle it.

2. Change Logger interface to have separate methods for each log level:
```go
type Logger interface {
    Debug(string)
    Info(string)
    Warn(string)
    Error(string)
}

type MyLogger struct {}

func (m MyLogger) Debug(s string) {
    // do nothing if Debug logs not needed
}

func (m MyLogger) Info(s string) {
    // implement
}

func (m MyLogger) Warn(s string) {
    // implement
}

func (m MyLogger) Error(s string) {
    // implement
}

I prefer 2-nd option. It is more clear and allow to easy suppress any log level.

metalrex100 commented 2 years ago

@wagslane any updates here? I can suggest PR with implementation of the 2-nd variant of Logger interface.

wagslane commented 2 years ago

Implemented in 0.9

metalrex100 commented 2 years ago

@wagslane so instead of further discussion you decided to close provided PR and just implement how you see it?

wagslane commented 2 years ago

@metalrex100 Hey there! I was waiting for a couple weeks for your PR to be updated, but it was just sitting here with failing tests.

Yeah so I went ahead with the log levels, the variadic params should work just fine (same as the printf). You can see an example in the default standard logger if you're unsure how to use it.

metalrex100 commented 2 years ago

@wagslane we were discussing the logger interface, my message was latest, you didn't answer, but just closed PR and implemented your solution 🫤 Why?