- Notifications
You must be signed in to change notification settings - Fork412
Create System.Diagnostic.Activity spans for parsing and invocation of S.CL commands#2529
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
KalleOlaviNiemitalo 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.
The new System.Diagnostics.DiagnosticSource package dependency on .NET Standard is a bit annoying.
How do you expect these diagnostics to be processed; what's the scenario? If the application references a telemetry library and initialises it before System.CommandLine, then the diagnostics can be sent out of process, over OTLP or such; but the endpoint for the data would have to be configured with files or environment variables as the command line has not been parsed yet. I think it unlikely that interactive users would set those environment variables. They can be configured in containers but there the diagnostics seem less valuable as the command line will likely be constant across executions.
Uh oh!
There was an error while loading.Please reload this page.
| activity.AddTag(DiagnosticsStrings.ExitCode,1); | ||
| if(exceptionis notnull) | ||
| { | ||
| activity.AddBaggage(DiagnosticsStrings.Exception,exception.ToString()); |
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.
Activity.AddException fromdotnet/runtime#102905 is in System.Diagnostics.DiagnosticSource 9.0.0 or greater, thus not in the 8.0.1 LTS that you specify in Directory.Packages.props, but do you expect to release System.CommandLine before .NET 10 LTS?
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.
This prompted me to filedotnet/dotnet-api-docs#11091
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.
I chose 8.0 because this repo currently targets net8.0 and I wanted to keep alignment with that, that's all. If we target 10 before .NET 10 releases, I would be happy to use that. I'd also love to follow any guidance from the BCL around tags vs baggage vs events, etc.
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.
@jonsequitur Just chiming in to say if System.CommandLine goes public for .NET 10, it would be great if the versioning aligned with all out-of-box packages and be package versioned at 10.0.0, and match the same AssemblyVersion schema as other out-of-box packages to make binding rules simpler.
| /// <returns>A value that can be used as a process exit code.</returns> | ||
| publicintInvoke() | ||
| { | ||
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.
Spurious addition
| if(invokeActivityis notnull) | ||
| { | ||
| invokeActivity.DisplayName=parseResult.CommandResult.FullCommandName(); | ||
| invokeActivity.AddTag(DiagnosticsStrings.Command,parseResult.CommandResult.Command.Name); |
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.
Tag value could be an array of strings
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.
I could see that, but I'm not sure how different transports would serialize the data.
Uh oh!
There was an error while loading.Please reload this page.
KalleOlaviNiemitalo commentedMar 14, 2025
I initially thought this was about method overloads with ReadOnlySpan<char> parameters. Would be good to mention "diagnostic" or "Activity" in case the PR title is ever listed in release notes. |
KalleOlaviNiemitalo commentedMar 14, 2025
I don't see InvokeType in the diff. |
Sorry@KalleOlaviNiemitalo - I missed a commit. I've pushed it up now! |
I see this as being useful for local testing as well as just a general good guideline for libraries in modern .NET - observability of applications is important, and knowing what your libraries are doing in semantic sections is useful. I did basically the equivalent of this in a one-off on the dotnet CLI to prove a point about the behavior of the CLI for workloads downloading, and it was kind of annoying that I had to make my own spans for parsing + invocation to show the 'full' lifecycle of the app. |
KalleOlaviNiemitalo commentedMar 17, 2025
The names of tags and baggage are Capitalized in the PR description but lowercase in the implementation. I feel uneasy about this use of baggage. Baggage is inherited by child activities. I'm not sure inheriting exceptions makes sense. Could be ActivityEvent instead. |
I like the idea of activity events - do you know of any docs or posts I could read to learn more about their intended use? |
KalleOlaviNiemitalo commentedMar 17, 2025
hey @dotnet/source-build I've introduced a prebuilt for System.Diagnostics.DiagnosticSource.8.0.1 - is it ok to just add this into the baselines? |
No, it can't be added to the baseline. That would only be possible if it could rely on the latest version of that package being flowed in by the runtime repo in the VMR build. But command-line-api builds before runtime, so it relies onSBRP or the previously source built packages. So that wouldn't have 8.0.1. I see that 8.0.0 already exists in SBRP. Can you use that version instead? Otherwise, you'll need toadd it to SBRP. |
I probably can - I was defaulting to a principle of "keeping patched" assuming that there was some reason to not go back. I'll roll the version back :) |
I see you still had a prebuilt for 8.0.0. My bad. I didn't realize the main branch here was consuming the packages from SBRP's release/8.0 branch. So yeah, that version doesn't exist there. Nor can it exist there; you can't have an 8.0 package in the 8.0 SBRP branch, only lower versions are allowed. But that shouldn't be a problem. In which .NET versions will this change flow to? Will it only be 10.0? In that case, then things are already setup in SBRP to have the necessary 8.0.0 version in which case the prebuilt baselinecan be updated to include that version. |
That's a good question - I'm having darc auth issues so I can't check easily, but itshould just be flowing to |
Yes, darc shows flow is just going to main branch. So updating the prebuilt baseline will resolve this. |
| internalconststringLibraryNamespace="System.CommandLine"; | ||
| internalconststringParseMethod=LibraryNamespace+".Parse"; | ||
| internalconststringInvokeMethod=LibraryNamespace+".Invoke"; | ||
| internalconststringInvokeType="invoke.type"; | ||
| internalconststringAsync="async"; | ||
| internalconststringSync="sync"; | ||
| internalconststringExitCode="exitcode"; | ||
| internalconststringException="exception"; | ||
| internalconststringErrors="errors"; | ||
| internalconststringCommand="command"; |
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.
If I understand OpenTelemetryRecommendations for Application Developers andSignal-specific Attributes correctly, the attribute names (tag names in .NET API) should preferably be namespaced even if the namespace is already innew ActivitySource(DiagnosticsStrings.LibraryNamespace) (instrumentation scope) and in theStartActivity calls (span name).
DiagnosticsStrings.InvokeType semantics look specific to this library.DiagnosticsStrings.Async andDiagnosticsStrings.Sync are used only as attribute values and don't need a namespace.
ForDiagnosticsStrings.ExitCode, one could perhaps useprocess.exit.code from OpenTelemetry Semantic Conventions. But I'm not sure it is correct to use that when the process has not exited yet and could even prompt the user for more commands to parse and execute.
ForDiagnosticsStrings.Exception, there is theException specification in OpenTelemetry Semantic Conventions. There, the event name is "exception", and the event may have tags "exception.message" (Exception.Message in .NET) and "exception.stacktrace" (Exception.ToString() in .NET, rather than Exception.StackTrace), among others.
ForDiagnosticsStrings.Errors, seeRecording errors on spans. I think it means the library should doactivity.AddTag("error.type", "System.CommandLine.Parsing.ParseError") andactivity.StatusDescription = string.Join(…), rather than add baggage.
ForDiagnosticsStrings.Command, the semantics don't match any ofprocess.command,process.command_args, orprocess.command_line in OpenTelemetry Semantic Conventions. Nothing suitable inSemantic conventions for CLI (command line interface) programs either.
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.
According toEvent Basics andopen-telemetry/opentelemetry-specification#4430, OpenTelemetry intends to deprecate Span Events and recommend Events in a log instead. Which means in .NET that exceptions would be written via Microsoft.Extensions.Logging.ILogger rather than System.Diagnostics.ActivityEvent.
In .NET though, ActivityEvent has not been deprecated. I think it's okay to use in this PR.
| internalstaticclassActivityExtensions | ||
| { | ||
| /// <summary> | ||
| /// Walks up the command tree to get the build the command name by prepending the parent command names to the 'leaf' command name. | ||
| /// </summary> | ||
| /// <param name="commandResult"></param> | ||
| /// <returns>The full command name, like 'dotnet package add'.</returns> | ||
| internalstaticstringFullCommandName(thisParsing.CommandResultcommandResult) |
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.
It feels a bit wonky thatActivityExtensions defines an extension method forCommandResult rather than forActivity.
Uh oh!
There was an error while loading.Please reload this page.
Based on a chat with@maddymontaquila we think this would be useful :)
Tried to follow some practices I saw in the BCL around defining per-use-case Activities and an Activity Source per-library, otherwise totally open to suggestions here.
Activity Source Name:
System.CommandLineParseActivity:System.CommandLine.ParseCommand- the name of the command that ended up being parsedErrors- newline-delimited list of parse error message stringsInvokeActivity:System.CommandLine.InvokeCommand- the name of the command that was invokedInvokeType- eitherAsyncorSyncbased on which kind of method was usedExitCode- the exit code returnedException- the rendered exception if one was thrown during invocation