Add collect-linux buffersize option - #5995
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The newly introduced validation rejecting --event-buffer-size-mb 0 is not currently covered by a functional test, leaving a regression gap for the new option behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new --event-buffer-size-mb option to dotnet-trace collect-linux so users can explicitly size per-CPU event buffers for bursty workloads, emitting the corresponding with_per_cpu_buffer_bytes(...) directive into the generated record-trace script.
Changes:
- Add
--event-buffer-size-mbCLI option and plumb it throughCollectLinuxArgs. - Emit
with_per_cpu_buffer_bytes(<bytes>)into the generated record-trace script when the option is provided. - Add a functional test validating the script emission for an 8 MB buffer size.
File summaries
| File | Description |
|---|---|
| src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs | Adds the nullable buffer-size option, validates it, and writes the corresponding script directive in bytes. |
| src/tests/dotnet-trace/CollectLinuxCommandFunctionalTests.cs | Updates test argument helper and adds coverage for the emitted script directive. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| Assert.Equal((int)ReturnCode.Ok, exitCode); | ||
| Assert.False(File.Exists(scriptPath)); | ||
| } | ||
|
|
||
| [ConditionalFact(nameof(IsCollectLinuxSupported))] | ||
| public void CollectLinuxCommand_PrintsStatusOnce_WhenCursorRepositioningUnsupported() |
| }; | ||
|
|
||
| private static readonly Option<uint?> EventBufferSizeInMBOption = | ||
| new("--event-buffer-size-mb") |
There was a problem hiding this comment.
The name of the argument doesn't suggest it is per-CPU so I suspect users will assume it is total buffer size. Rather than clarifying the name what if we redefine the behavior to mean the total buffer size and let the tool handle observing the number of CPUs and doing the division? I suspect users will find total buffer size a more natural concept to reason about than per-cpu buffer size.
Assuming we do that, dotnet collect also has a --buffersize argument and I think we should match the naming. Having two commands with different names for a very similarly purposed argument seems like inconsistent UX.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
hoyosjs
changed the title
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| return Encoding.UTF8.GetBytes(options); | ||
| } | ||
|
|
||
| internal static ulong GetOnlineProcessorCount() |
There was a problem hiding this comment.
Digging into this deeper it looks like one-collect doesn't always create buffers for all CPUs so this calculation still might not give the right result overall. Reverse engineering the set of CPUs one-collect actually uses from its configuration also doesn't seem great.
I think we'd need to either:
- let the one-collect portion of the tool do the global to per-CPU partitioning
- give up on specifying a global buffer size and offer per-CPU sizing only (with an argument name that clearly conveys it is a 'per-cpu' value).
Which one might depend on how important Beau/Brian think it is that buffers are specified as a per-CPU value in the overall tracing UX. I'm not aware that we've done anything similar in the past for other .NET profiling scenarios (via ETW for instance), but I am less familiar with what the Linux precedents are.
There was a problem hiding this comment.
I'll let the one-collect side of things go first then
| private static readonly Option<uint?> BufferSizeInMBOption = | ||
| new("--buffersize") | ||
| { | ||
| Description = "Requested total size of the event buffers, in megabytes. The size is divided across the available CPUs. When omitted, the recorder chooses a default based on the enabled features." |
There was a problem hiding this comment.
| Description = "Requested total size of the event buffers, in megabytes. The size is divided across the available CPUs. When omitted, the recorder chooses a default based on the enabled features." | |
| Description = "Requested total size of the event buffers, in megabytes. When omitted, the recorder chooses a default based on the enabled features." |
I think mentioning the size is divided across CPUs is more likely to create confusion than clarity. Users have no idea the interface to one-collect is a per-cpu measurement.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds
--buffersizetodotnet-trace collect-linux, matching the existingdotnet-trace collectoption name. The value is a requested total event-buffer size in megabytes and is forwarded unchanged to one-collect through the generated script. One-collect owns platform-specific normalization.When omitted, one-collect continues to choose its adaptive default. Zero is rejected before record-trace is invoked. This PR depends on the new Universal script setting in microsoft/one-collect#332 and should consume a package containing that change before merge.
Breaking changes: None.