profiler: log configuration at profiling start #1114
Conversation
To match what the tracer package currently does, and make debugging a little easier, log user-supplied configuration when starting a profile. This is configurable with WithLogStartup (like tracer) and is on by default. Tests had to be patched as well since the new, on-by-default logging drowned out all the test results. Updated tests to discard logs by default for testing the profiler package.
There was a problem hiding this comment.
LGTM, but see comments.
A non-pedantic test case that asserts that a startup message gets logged and contains at least a few interesting things (e.g. enabled profiles) might also be nice. IMO there is no need to verify the presence of each field or to hard-code the entire log message as that will just annoy us as we add more things in the future.
|
|
||
| // logStartup records the configuration to the configured logger in JSON format | ||
| func logStartup(c *config) { | ||
| info := struct { |
There was a problem hiding this comment.
NIT: The tracer includes some additional fields that could be useful, e.g. go version, os version, os name, etc., see:
dd-trace-go/ddtrace/tracer/log.go
Lines 84 to 89 in b6dbd9f
Maybe we should log those as well?
Additionally the following fields from the config struct seem missing:
- hostname
- deltaProfiles
- maxGoroutinesWait
There was a problem hiding this comment.
Thanks for the review. I added the missing config fields and checked for the startup log in a test.
RE: the other fields, would it make sense to pull the osName & osVersion functions from the tracer package into their own internal package so the implementations can be shared?
There was a problem hiding this comment.
OK, I pulled out the OS info to its own package and added the other fields for the profiler log. PTAL
I want to use osName and osVersion in logging profiler configuration, so I'm putting the functions in their own package to re-use the code
| // so we want to discard it during tests to avoid flooding the terminal | ||
| // with logs | ||
| log.UseLogger(log.DiscardLogger{}) | ||
| os.Exit(m.Run()) |
There was a problem hiding this comment.
LGTM. But let's wait for the v1.35.0 release (hopefully tomorrow) before hitting merge (i.e. this will go into 1.36.0)
Arguably it's a small patch without much risk, but I think it's a slippery slope if we start allowing unrelated changes into a release-in-progress.
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@gmail.com>
ddtrace/tracer/log.go
Outdated
| ProfilerCodeHotspotsEnabled: t.config.profilerHotspots, | ||
| ProfilerEndpointsEnabled: t.config.profilerEndpoints, |
There was a problem hiding this comment.
Looks like ProfilerCodeHotspotsEnabled and ProfilerEndpointsEnabled were removed from here. Is that intentional?
There was a problem hiding this comment.
No, it wasn't intentional. I'm not quite sure how that happened... I'll add them back in, thanks for catching that
I somehow removed them by mistake.
To match what the tracer package currently does, and make debugging a little
easier, log user-supplied configuration when starting a profile. This is
configurable with WithLogStartup (like tracer) and is on by default.
Tests had to be patched as well since the new, on-by-default logging drowned
out all the test results. Updated tests to discard logs by default for testing
the profiler package.