[common/log] Unify logger package #6779
Conversation
3vilhamster
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
jakobht,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| @@ -20,23 +20,16 @@ | |||
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | |||
| // SOFTWARE. | |||
|
|
|||
| //go:generate mockgen -package=$GOPACKAGE -destination=limiterfactory_mock.go github.com/uber/cadence/common/quotas LimiterFactory | |||
|
|
|||
| package quotas | |||
There was a problem hiding this comment.
Is it to avoid a circulair dependency we are moving this to the dynamicconfig package. It feels a bit strange, the ratelimiters does not have anything to do with dynamic config?
There was a problem hiding this comment.
they use dynamic config and log and introduce circular dependency :(
There was a problem hiding this comment.
yeah, well everything uses dynamic config.. but I guess it's very tied to these loggers, so maybe it's ok
| const skipForThrottleLogger = 6 | ||
|
|
||
| // NewThrottledLogger returns an implementation of logger that throttles the | ||
| // log messages being emitted. The underlying implementation uses a token bucket | ||
| // ratelimiter and stops emitting logs once the bucket runs out of tokens | ||
| // | ||
| // Fatal/Panic logs are always emitted without any throttling | ||
| func NewThrottledLogger(logger log.Logger, rps dynamicconfig.IntPropertyFn) log.Logger { | ||
| var log log.Logger | ||
| func NewThrottledLogger(logger Logger, rps func() int) Logger { |
There was a problem hiding this comment.
I don't think it's better to have a naked function than the "sematic" dynamicconfig.IntPropertyFn
There was a problem hiding this comment.
This introduces circular dependency - dynamic config use log.Logger, so log package is not allowed to have dependency on the dynamicconfig. Also, rps is always called without params, so the annotation of IntPropertyFn does not help.
There was a problem hiding this comment.
I disagree it does not help. It says it's a property from dynamic config and not just a random function that returns an int.
I see the issue with the cyclic dependency. That's probably why it was split :) I think it's ok since we have the issue.
There was a problem hiding this comment.
kinda looks like the underlying issue is that we have multiple complex implementations in dynamicconfig itself, which are importing other stuff in what's otherwise a leaf-node package that everything uses (aside from common/types). might be worth making e.g. dynamicconfig/fileconfig and dynamicconfig/collection packages
There was a problem hiding this comment.
Makes sense. I will follow up with a PR that moves parts of dynamicconfig and can restore this use.
There was a problem hiding this comment.
Implemented the move in #6780
3vilhamster
force-pushed
the
simplify-log
branch
from
9ed5c86 to
57c4650
Compare
What changed?
Merge loggerimpl package with log.
Why?
As part of FX integration, I need components to have fewer dependencies, which will lower the chances of having circular dependencies.
Merging these packages eliminated the redundancy of two implementations of Noop loggers and provided clear responsibility for the log package.
I also had to touch the quotas package and move dynamic configurable quotas to their package.
How did you test it?
Unit tests
Potential risks
None
Release notes
Refactoring of the log package may require import updates.
Documentation Changes