search

labstack / gommon

Common packages for Go
MIT License
536 stars 100 forks source link

remove atomic operation in the logger #52

Closed lesismal closed 1 year ago

lesismal commented 2 years ago

I think these atomic operations in logger are not useful, reasons as below:

  1. Usually, we should init logger level at the beginning of the process, before the service starts listening.
  2. There are many goroutines calling logger methods, the atomic operations can only lock the level field but can not lock the logging processing steps which do not need to be guaranteed, and that should be a little waste of the performance.
codecov-commenter commented 2 years ago

Codecov Report

Merging #52 (5bcfdb9) into master (6267eb7) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head 5bcfdb9 differs from pull request most recent head 19a197e. Consider uploading reports for the commit 19a197e to get more accurate results

@@           Coverage Diff           @@
##           master      #52   +/-   ##
=======================================
  Coverage   60.70%   60.70%           
=======================================
  Files           6        6           
  Lines         626      626           
=======================================
  Hits          380      380           
  Misses        242      242           
  Partials        4        4
Impacted Files Coverage Ξ”
log/log.go 53.15% <100.00%> (ΓΈ)

Help us with your feedback. Take ten seconds to tell us how you rate us.

aldas commented 2 years ago

about 2. - I assume original creator though process was to guard against data race when level value is changed. Underlying writer that does actual "output" is separate consern.

Just out of curiosity - have you considered using some proper (structured) logging library?

there are "wrappers" for these libraries to Echo logger interface.

I have not benchmarked Echo logger against other loggers but instead straight to go Zerolog (my favorite) and do not bother at all with Echo native logger. This is something that I assumed most of the people would do if they start doing logging more seriously.

lesismal commented 2 years ago

Thank you for your response!

About data race: I think performance is more important for basic frameworks, if really care about data race, //go:norace may be better for the lines which we are sure about safety, I have just made a new commit to disable that race warning by using //go:norace.

Just out of curiosity - have you considered using some proper (structured) logging library?

I use zap in most of my projects, just found this atomic operations when I read the code and make this pr.

This is something that I assumed most of the people would do if they start doing logging more seriously.

Agree!

lesismal commented 2 years ago

If you guys don't think it cares too much, just close this pr, I'm fine with it, really like echo πŸ˜„ .

aldas commented 2 years ago

I am little bit hesitant about this change. It removes guarantee for guarding data race on that field by assuming that no-one wants to change level during runtime for unknown amount of performance improvement.

This seems somewhat local optimization that does not take into consideration that at (larger) scale where usage of atomic.LoadUint32 matters or not you probably will not be using that logger at all.

I personally think that Echo logger has been made obsolete by those other wonderful logging libraries and for example middlewares we have added RequestLogger to be successor of older Logger middleware.

Please do not take it personally and thank you for your PR.

lesismal commented 2 years ago

Since the data race does not matter about safety, it will not matter both we use atomic or not. We just get the data race warning during the test but can be disabled by /go:norace. Although most of the users custom logger themselves, not everyone does that. And also, since there are not too many people using echo's default logger, it will matter even less if we remove the atomic operations. πŸ˜„