bufferapp / php-bufflog

php logger for all PHP Buffer services
1 stars 1 forks source link

Mix types for the context argument #12

Open erickhun opened 4 years ago

erickhun commented 4 years ago

Hi @esclapes @colinscape

Wanted to get your thoughts on making BuffLog more flexible and make it "safer" to use.

There was an incident that happen when I've integrated BuffLog with the analyze workers

How it works now is that we call Monolog directly but if the 2nd param type isn't correct, it will throw an exception and crash the script.

I've also noticed some challenges when replacing all the var_dump in the analyze code base by trying to guess what type are the variables.

Here my question/suggestion:

I like the flexibility of a var_dump($param) because we can use it without worrying much about about what $param is. I also believe it's used because it will never crash the code.

So we would have prototype that looks lilke that:

BuffLog::notice(string , mixed); 
BuffLog::debug(string , mixed); 
...

// instead of
BuffLog::notice(string , array); 
BuffLog::debug(string , array); 

Thoughts? :)

colinscape commented 4 years ago

I definitely like the idea of making the logger as safe to use as possible. We definitely don't want processes crashing because of a failed logging call.

I guess the question is then whether we accept the multitude of possible types that could be sent, or whether we reject them, and log the error.

I'm tending towards the latter as a means of helping to make it clear what the logging method needs. Passing numbers or strings as the second parameter can probably be achieved in much the same way by incorporating them into the message, and providing a 'raw' value like "3" doesn't give any information on what that value is (is it the number of profile, or the number of updates or what?) and so having something appear in Datadog like {"value": 3} won't provide a lot of use.

We'd just want to make sure that if we log mis-use of the log method that it doesn't get into an infinite loop!! 😅

Would love to hear other perspectives and thoughts round this though!