profiler: add WithLogger option, respect DD_TRACE_STARTUP_LOGS #1334
Conversation
| // WithLogger sets logger as the profiler's error printer. | ||
| // Note that setting the logger for the profiler sets the same logger for the | ||
| // tracer, and vice-versa. | ||
| func WithLogger(logger ddtrace.Logger) Option { |
There was a problem hiding this comment.
I don't know if this is a good approach. In hindsight, it was a bad choice for the tracer too. This setting is not app-specific, it is repo wide. Someone could come later and ask the same thing from AppSec or some other component that may come up. Or even worse, they could expect to be able to set different logger for different apps, which is wrong.
I suggest considering to make the logger exported instead...
There was a problem hiding this comment.
Do you mean make internal/log.UseLogger exported, as requested in #1331? Alternatively, I could keep this interface and refactor the profiler to do its own logging internally. Or we could refactor internal/log to provide a new Logger type with all the same Error, Warn, etc. methods, so that each package can use its own logger or fall back to a default, shared logger.
nsrip-dd
changed the title
profiler/log.go
Outdated
|
|
||
| func (l *logger) Info(format string, a ...interface{}) { l.msg("", format, a...) } | ||
| func (l *logger) Error(format string, a ...interface{}) { l.msg("", format, a...) } | ||
| func (l *logger) Warn(format string, a ...interface{}) { l.msg("", format, a...) } |
There was a problem hiding this comment.
Why is the lvl = "" for all of the msg() calls above?
Also: Wouldn't it be nicer to put this logger implementation into internal/log and reuse it here? Or is there a specific reason for the code duplication? (It's not a lot of duplication, so I'm not too worried, just curious).
There was a problem hiding this comment.
RE: lvl, that was an oversight, I'll fix it.
RE: duplication, I'm not sure what the best approach is. The first version of this PR set the logger using internal/log.UseLogger, which would set the global logger for every dd-trace-go package that uses internal/log. If we're okay with that, maybe making UseLogger public is better than adding a WithLogger option? Having two WithLogger options creates the impression that logging can be configured independently for different packages, as Gabriel points out.
If we do want a logger struct like this so that packages can do their own, independent logging, we could certainly move this code into internal/log, but unless we also make the tracer use this struct then there will still be some duplicated code.
Any thoughts on what's better?
There was a problem hiding this comment.
Thanks Felix! See my followup comments on the WithLogger option. I'm not 100% on whether to keep this code or do it a different way.
P.S. I was thinking of moving the DD_TRACE_STARTUP_LOGS change into a separate PR since I think we'll want to support that environment variable regardless of how we configure logging. Should I go ahead and do that, or just keep the change in this PR?
profiler/log.go
Outdated
|
|
||
| func (l *logger) Info(format string, a ...interface{}) { l.msg("", format, a...) } | ||
| func (l *logger) Error(format string, a ...interface{}) { l.msg("", format, a...) } | ||
| func (l *logger) Warn(format string, a ...interface{}) { l.msg("", format, a...) } |
There was a problem hiding this comment.
RE: lvl, that was an oversight, I'll fix it.
RE: duplication, I'm not sure what the best approach is. The first version of this PR set the logger using internal/log.UseLogger, which would set the global logger for every dd-trace-go package that uses internal/log. If we're okay with that, maybe making UseLogger public is better than adding a WithLogger option? Having two WithLogger options creates the impression that logging can be configured independently for different packages, as Gabriel points out.
If we do want a logger struct like this so that packages can do their own, independent logging, we could certainly move this code into internal/log, but unless we also make the tracer use this struct then there will still be some duplicated code.
Any thoughts on what's better?
|
I'm going to close this in favor of two separate PRs:
|
Resolves #1331