Add option to get logs from librecordtrace as needed#5709
Add option to get logs from librecordtrace as needed#5709hoyosjs wants to merge 1 commit intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new collect-linux CLI switch to enable/route diagnostic logging from the underlying recordtrace/librecordtrace invocation, with functional tests validating argument generation.
Changes:
- Extend
CollectLinuxArgsandcollect-linuxcommand parsing to support--debug-log(zero-or-one value) and hidden--debug-filter. - Pass
--log-mode/--log-file/--log-filterthrough to recordtrace based on the new options. - Add functional tests covering console logging, default file logging, custom file logging, custom filter, and ignoring filter without debug-log.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs | Adds new CLI options, parses them, and appends corresponding recordtrace logging arguments. |
| src/tests/dotnet-trace/CollectLinuxCommandFunctionalTests.cs | Adds coverage to ensure the correct recordtrace args are generated for the new debug logging options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Description = @"Enable diagnostic logging for collect-linux itself. Specify 'console' to log to the console, or a file path to log to a file. If no value is provided, logs are written to '<tracefile>.debuglog'.", | ||
| Arity = ArgumentArity.ZeroOrOne | ||
| }; | ||
|
|
||
| private static readonly Option<string> DebugFilterOption = | ||
| new("--debug-filter") | ||
| { | ||
| Description = @"Override the default log filter for debug logging. Only used when --debug-log is specified. Default filter: 'error,one_collect::helpers::dotnet::os::linux=debug'.", |
There was a problem hiding this comment.
--debug-log help text says it enables logging for collect-linux itself, but the implementation wires this option into the underlying recordtrace/librecordtrace arguments (--log-mode/--log-file/--log-filter). Consider updating the option description to explicitly mention recordtrace/librecordtrace so users understand what will emit the logs and what they’re for.
| Description = @"Enable diagnostic logging for collect-linux itself. Specify 'console' to log to the console, or a file path to log to a file. If no value is provided, logs are written to '<tracefile>.debuglog'.", | |
| Arity = ArgumentArity.ZeroOrOne | |
| }; | |
| private static readonly Option<string> DebugFilterOption = | |
| new("--debug-filter") | |
| { | |
| Description = @"Override the default log filter for debug logging. Only used when --debug-log is specified. Default filter: 'error,one_collect::helpers::dotnet::os::linux=debug'.", | |
| Description = @"Enable diagnostic logging for the underlying recordtrace/librecordtrace library used by collect-linux. Specify 'console' to log to the console, or a file path to log to a file. If no value is provided, logs are written to '<tracefile>.debuglog'.", | |
| Arity = ArgumentArity.ZeroOrOne | |
| }; | |
| private static readonly Option<string> DebugFilterOption = | |
| new("--debug-filter") | |
| { | |
| Description = @"Override the default recordtrace/librecordtrace log filter used for debug logging. Only used when --debug-log is specified. Default filter: 'error,one_collect::helpers::dotnet::os::linux=debug'.", |
| if (args.DebugLogOutput is not null) | ||
| { | ||
| Console.WriteLine($"Generated recordtrace options: {options}"); | ||
| } |
There was a problem hiding this comment.
When --debug-log is enabled, the code always prints Generated recordtrace options: ... to the console, even when logs are configured to go to a file. This adds unexpected console output and may interfere with automation that parses stdout/stderr; consider only emitting this when logging to console, sending it to stderr, or guarding it behind a dedicated verbosity/debug flag.
| isSupportedLinuxPlatform = !ostype.Contains("ID=alpine"); | ||
| } | ||
| catch (Exception ex) when (ex is FileNotFoundException or DirectoryNotFoundException or IOException) {} | ||
| catch (Exception ex) when (ex is FileNotFoundException or DirectoryNotFoundException or IOException) { } |
There was a problem hiding this comment.
Poor error handling: empty catch block.
| catch (Exception ex) when (ex is FileNotFoundException or DirectoryNotFoundException or IOException) { } | |
| catch (Exception ex) when (ex is FileNotFoundException or DirectoryNotFoundException or IOException) | |
| { | |
| Debug.WriteLine($"Failed to read /etc/os-release to determine Linux distribution: {ex}"); | |
| } |
| } | ||
| } catch { } | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
Poor error handling: empty catch block.
| catch { } | |
| catch (Exception ex) | |
| { | |
| Console.Error.WriteLine($"[WARN] Failed to delete temporary script file '{scriptPath}': {ex.Message}"); | |
| } |
|
|
||
| if (args.DebugLogOutput is not null) | ||
| { | ||
| string logFilter = args.DebugFilter ?? "error,one_collect::helpers::dotnet::os::linux=debug"; |
There was a problem hiding this comment.
@brianrob, since we used this filter for debugging, do you think it's a good fit for the default debug filter?
There was a problem hiding this comment.
I would recommend just using "info" by default. That should give you a good assessment of what's happening and you can always dial that up if necessary. That's the default that we use in record-trace as well.
| Arity = ArgumentArity.ZeroOrOne | ||
| }; | ||
|
|
||
| private static readonly Option<string> DebugFilterOption = |
There was a problem hiding this comment.
I think we should try to set a direction we can standardize on across our tools. We've already got several of our tools that have each done it a different way:
dotnet-dump --diag, dotnet-symbol --diagnostics, dotnet-gcdump --verbose, dotnet-dsrouter --verbose <level>
I'd suggest lets standardize on --verbose [<level>] where the default level is 'info' when unspecified. This aligns with many pre-existing Linux tools that use --verbose or -v. Not as part of this PR but we could add support for --verbose in our other tools and treat whatever they currently support ("diag","diagnostics", etc) as an alias for --verbose.
fyi @lateralusX
There was a problem hiding this comment.
From a consistency POV, I think this seems very reasonable. Record-trace takes a different approach and uses the rust-based RUST_LOG pattern which allows for customizable log messages per target (e.g. component). While you may not want to expose this directly in the command line, you may wish to have an escape hatch such as an environment variable in case you need to turn on very verbose logging for a particular component, but not for everything when reproducing a bug. Of course this isn't a hard requirement - just want to make the origin of this filter clear.
There was a problem hiding this comment.
yeah, if we need to hit that level of detail something like an env var sounds nice. I'm hoping we can resolve most issues prior to needing that precision.
| }; | ||
|
|
||
| private static readonly Option<string> DebugLogOption = | ||
| new("--debug-log") |
There was a problem hiding this comment.
How important is this? For example would it be sufficient to write all the verbose output to stderr and let the user pipe it to a file if they want to? I don't recall that any of our other diagnostic tools write diagnostic output to a file by default.
There was a problem hiding this comment.
Some of the logs were pretty verbose and take a lot of console. Just consider it easy to redirect them to a file since the rust lib enabled that, and console coming from 2 tools is something that could interleave - this felt cleaner. Setting the environment variable didn't work as expected when I tried last week, and this was useful when debugging so I preferred to add it as something that could be useful here. I did consider the consistency aspect of it, but didn't standardize on this quick commit. Happy to standardize if we see value to having this option in the tool.
There was a problem hiding this comment.
Some of the logs were pretty verbose and take a lot of console. Just consider it easy to redirect them to a file since the rust lib enabled that, and console coming from 2 tools is something that could interleave - this felt cleaner. Setting the environment variable didn't work as expected when I tried last week, and this was useful when debugging so I preferred to add it as something that could be useful here. I did consider the consistency aspect of it, but didn't standardize on this quick commit. Happy to standardize if we see value to having this option in the tool.
There was a problem hiding this comment.
console coming from 2 tools is something that could interleave
two tools as in two libraries in the same process? If you run it with output to the console have you every observed it interleave or you mean its possible in theory but not observed?
Some of the logs were pretty verbose and take a lot of console
How verbose are we talking... 100s of lines, 1000s, 10K+?
4 participants
No description provided.