-
Notifications
You must be signed in to change notification settings - Fork 386
[DiagnosticsClient] Add user events probe contract and API #5656
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
tommcdon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment for your consideration, though given the necessity for this capability check I think this approach is reasonable.
Support to probe any CommandSet/CommandID support may be added in the future, so to avoid conflicting APIs, keep this internal for now as the query is only relevant to dotnet-trace collect-linux.
|
The test failure looks unrelated, filed #5659 |
|
Why couldn't we just use the GetProcessInfo DiagnosticClient command and check runtime version? That seems more stable short-term solution until we have a real capabilities API and will continue to work going forward instead of adding dependency on specific formatting errors for a specific command. You will get back the full runtime version 10.0.0 and we know that user events are part of version 10 and later, so should be straight forward to check if runtime is vesion 10 or later. You can also get back the rid from the runtime meaning that you can validate that it's also running on linux as an additional check if needed. Doing it this way you don't need to probe anything, you could just add logic to the collect-linux command, first checking the version of the runtime using the GetProcessInfo and then decide if you can use collect-linux command or not. Other tools does similar things to detect version of runtime and take decisions based on that, like dotnet-counters. |
This will return false-positives for .NET 10 Preview versions < Preview7. If we are fine with those, moving
Good point. But for the machine wide trace should we just connect back to the runtime running
I think this works for a single process trace. I'm wondering if we should just have a warning that the process doesn't support user events, in case the user still wants to collect perf events? |
|
With the runtime version check through GetProcessInfo, the changes in this PR are no longer needed to detect CollectTracing5 support. |
For an arbitrary .NET process, it's not obvious whether that .NET process supports emitting user_events dotnet/runtime#115265 without knowing the exact runtime version. To make it easier for users to probe a .NET process to determine whether they should use
dotnet-trace collect-linuxor fallbackdotnet-trace collectto collect .NET runtime events, this PR proposes a contract and an internal API addition intended to be invoked bydotnet-trace collect-linux. To note,dotnet-trace collect-linuxwill not fail even though there may be no .NET processes capable of emitting user_events.Adding a bit more context. I briefly explored a standardized way to probe for CommandSet/CommandID availability (Dump/EventPipe/Profiler/Process) that would be backwards compatible with all prior runtime versions. An empty payload doesn't work (see dotnet/runtime#122257), so each CommandSet/CommandId requires a custom payload tailored to their parsing strategy to pass the UnknownCommand checkpoint and hit a separate error to not trigger the command.
Although this could work for Dump/EventPipe/Profiler, no custom payload can probe for Process commands availability without triggering the command, because Process commands trigger without a payload.
Due to the customization between CommandSets and even between CommandIDs (e.g. CollectTracing5 first deserializes a session_type whereas CollectTracing(2)(3)(4) first deserialize a circular_buffer_size, which have different "invalid" values) + it seems historically no one needed to query commandset/id availability + this seems to be the only mechanism in .NET 10 to detect user_events capability since dotnet-trace collect-linux/record-trace don't error even if no .NET 10 process is running, I felt that just adding a UserEvents EventPipe Command probe was more appropriate.
I do think that .NET 11 could introduce a probe command set that could return if any command set/id is supported (not just eventpipe) if there's a demand (maybe it fits as command set 0x00?), and it could possibly be made backwards compatible with older .NET versions by using customized payloads. Process commands would still be tricky for backwards compatibility given the empty payload design.
The
dotnet-trace collect-linux --probeimplementation is #5657