pkg/promtail: propagate a logger rather than using util.Logger globally #2438
Conversation
This commit allows for creating promtail with a custom log.Logger instance which will be propagated and used consistently throughout the Promtail package. This allows for clients to provide a Promtail-specific logger. Address a portion of grafana#2405.
Codecov Report
@@ Coverage Diff @@
## master #2438 +/- ##
==========================================
- Coverage 61.55% 61.53% -0.03%
==========================================
Files 160 160
Lines 13612 13616 +4
==========================================
- Hits 8379 8378 -1
- Misses 4607 4611 +4
- Partials 626 627 +1
|
| type Option func(p *Promtail) | ||
|
|
||
| // WithLogger overrides the default logger for Promtail. | ||
| func WithLogger(log log.Logger) Option { |
There was a problem hiding this comment.
This seems like a bit of a weird way to expose this. Since we're expecting to modify this externally, couldn't we just make the field public? Perhaps you wanted to make it only modifiable via this new function. I'm not really opinionated here though.
There was a problem hiding this comment.
It'd have to be a public field in the Config struct for this to work since the logger is already passed around when New is called. I'm open to this, but I think it'd be a little strange to do that.
I do agree that the Option pattern is a big heavy handed for this specific PR, but my justification is that there's going to be a second With<X> function in my final PR for #2405, where you give it a custom mux router, where having two option functions would feel more justified.
I don't feel that strongly about this either though, I'm happy to make it a public field of Config if you think that'd be a better approach here.
This PR allows for creating Promtail with a custom log.Logger instance which will be propagated and used consistently throughout the Promtail package. This allows for clients to provide a Promtail-specific logger.
The options pattern is now used for instantiating a Promtail instance; the only option to apply as of this PR is
WithLogger. If unspecified, the default behavior is unchanged from how Promtail behaves today (i.e., using the globalutil.Logger).The downside of this PR is having pass through the logger to more functions now, leaving the code a bit messier.
Address a portion of #2405.