Create System.Diagnostic.Activity spans for parsing and invocation of S.CL commands #2529

baronfel · 2025-03-13T16:02:43Z

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.CommandLine

Parse Activity:

Name: System.CommandLine.Parse
Status: Ok if parsing succeeded, Error if any parse errors were detected
Tags:
- Command - the name of the command that ended up being parsed
Baggage:
- Errors - newline-delimited list of parse error message strings

Invoke Activity:

Name: System.CommandLine.Invoke
Status: Ok if invocation returned a 0 exit code, Error if any nonzero exit code was returned, or if an exception was thrown during invocation
Tags:
- Command - the name of the command that was invoked
- InvokeType - either Async or Sync based on which kind of method was used
- ExitCode - the exit code returned
Baggage:
- Exception - the rendered exception if one was thrown during invocation

src/System.CommandLine/Activities.cs

KalleOlaviNiemitalo

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.

src/System.CommandLine/Activities.cs

KalleOlaviNiemitalo · 2025-03-14T06:06:52Z

src/System.CommandLine/Invocation/InvocationPipeline.cs

+            activity.AddTag(DiagnosticsStrings.ExitCode, 1);
+            if (exception is not null)
+            {
+                activity.AddBaggage(DiagnosticsStrings.Exception, exception.ToString());


Activity.AddException from dotnet/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?

This prompted me to file dotnet/dotnet-api-docs#11091

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.

@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.

KalleOlaviNiemitalo · 2025-03-14T06:08:03Z

src/System.CommandLine/ParseResult.cs

@@ -236,6 +236,7 @@ public Task<int> InvokeAsync(CancellationToken cancellationToken = default)
        /// <returns>A value that can be used as a process exit code.</returns>
        public int Invoke()
        {
+


Spurious addition

KalleOlaviNiemitalo · 2025-03-14T06:24:09Z

src/System.CommandLine/Invocation/InvocationPipeline.cs

+            if (invokeActivity is not null)
+            {
+                invokeActivity.DisplayName = parseResult.CommandResult.FullCommandName();
+                invokeActivity.AddTag(DiagnosticsStrings.Command, parseResult.CommandResult.Command.Name);


Tag value could be an array of strings

I could see that, but I'm not sure how different transports would serialize the data.

src/System.CommandLine/Parsing/CommandLineParser.cs

KalleOlaviNiemitalo · 2025-03-14T07:23:33Z

Create spans for parsing and invocation of S.CL commands

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 · 2025-03-14T07:46:26Z

InvokeType - either Async or Sync based on which kind of method was used

I don't see InvokeType in the diff.

baronfel · 2025-03-14T20:17:27Z

Sorry @KalleOlaviNiemitalo - I missed a commit. I've pushed it up now!

baronfel · 2025-03-17T15:38:59Z

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 · 2025-03-17T16:48:10Z

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.

baronfel · 2025-03-17T16:49:30Z

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 · 2025-03-17T17:07:33Z

https://opentelemetry.io/docs/concepts/signals/traces/#span-events maybe

baronfel · 2025-03-25T19:20:22Z

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?

mthalman · 2025-03-25T19:28:00Z

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 on SBRP 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 to add it to SBRP.

baronfel · 2025-03-25T19:29:52Z

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 :)

mthalman · 2025-03-25T20:57:09Z

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 baseline can be updated to include that version.

baronfel · 2025-03-26T01:53:23Z

That's a good question - I'm having darc auth issues so I can't check easily, but it should just be flowing to main branches for all the repos, so .NET 10 only.

mthalman · 2025-03-26T12:44:25Z

Yes, darc shows flow is just going to main branch. So updating the prebuilt baseline will resolve this.

KalleOlaviNiemitalo · 2025-04-04T12:21:23Z

src/System.CommandLine/Activities.cs

+    internal const string LibraryNamespace = "System.CommandLine";
+    internal const string ParseMethod = LibraryNamespace + ".Parse";
+    internal const string InvokeMethod = LibraryNamespace + ".Invoke";
+    internal const string InvokeType = "invoke.type";
+    internal const string Async = "async";
+    internal const string Sync = "sync";
+    internal const string ExitCode = "exitcode";
+    internal const string Exception = "exception";
+    internal const string Errors = "errors";
+    internal const string Command = "command";


If I understand OpenTelemetry Recommendations for Application Developers and Signal-specific Attributes correctly, the attribute names (tag names in .NET API) should preferably be namespaced even if the namespace is already in new ActivitySource(DiagnosticsStrings.LibraryNamespace) (instrumentation scope) and in the StartActivity calls (span name).

DiagnosticsStrings.InvokeType semantics look specific to this library. DiagnosticsStrings.Async and DiagnosticsStrings.Sync are used only as attribute values and don't need a namespace.

For DiagnosticsStrings.ExitCode, one could perhaps use process.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.

For DiagnosticsStrings.Exception, there is the Exception 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.

For DiagnosticsStrings.Errors, see Recording errors on spans. I think it means the library should do activity.AddTag("error.type", "System.CommandLine.Parsing.ParseError") and activity.StatusDescription = string.Join(…), rather than add baggage.

For DiagnosticsStrings.Command, the semantics don't match any of process.command, process.command_args, or process.command_line in OpenTelemetry Semantic Conventions. Nothing suitable in Semantic conventions for CLI (command line interface) programs either.

According to Event Basics and open-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.

KalleOlaviNiemitalo · 2025-04-04T12:48:54Z

src/System.CommandLine/Activities.cs

+internal static class ActivityExtensions
+{
+
+    /// <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>
+    internal static string FullCommandName(this Parsing.CommandResult commandResult)


It feels a bit wonky that ActivityExtensions defines an extension method for CommandResult rather than for Activity.

jonsequitur reviewed Mar 13, 2025

View reviewed changes

src/System.CommandLine/Activities.cs Show resolved Hide resolved

KalleOlaviNiemitalo suggested changes Mar 14, 2025

View reviewed changes

baronfel force-pushed the spans branch from c6097ae to 8837701 Compare March 14, 2025 20:16

Create spans for parsing and invocation of S.CL commands

2706703

baronfel force-pushed the spans branch from 8837701 to 2706703 Compare March 14, 2025 21:28

baronfel changed the title ~~Create spans for parsing and invocation of S.CL commands~~ Create System.Diagnostic.Activity spans for parsing and invocation of S.CL commands Mar 14, 2025

add some basic tests to assert on activity presence and shapes

0f15752

baronfel force-pushed the spans branch from 4c194c7 to 0f15752 Compare March 24, 2025 19:55

baronfel added 2 commits March 24, 2025 15:49

events for exceptions during execution instead of baggage

6cc650c

Try to get past list multiple iteration?

827eab1

baronfel added 2 commits March 25, 2025 14:31

go back a version to prevent prevuilts

975c17e

try to track source-build usage

d3fa6ef

baronfel force-pushed the spans branch from fe452ee to d3fa6ef Compare March 25, 2025 20:10

KalleOlaviNiemitalo reviewed Apr 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create System.Diagnostic.Activity spans for parsing and invocation of S.CL commands #2529

Create System.Diagnostic.Activity spans for parsing and invocation of S.CL commands #2529

baronfel commented Mar 13, 2025 •

edited

Loading

KalleOlaviNiemitalo left a comment

KalleOlaviNiemitalo Mar 14, 2025

KalleOlaviNiemitalo Mar 14, 2025

baronfel Mar 14, 2025

jzabroski Apr 4, 2025

KalleOlaviNiemitalo Mar 14, 2025

KalleOlaviNiemitalo Mar 14, 2025

baronfel Mar 14, 2025

KalleOlaviNiemitalo commented Mar 14, 2025

KalleOlaviNiemitalo commented Mar 14, 2025

baronfel commented Mar 14, 2025

baronfel commented Mar 17, 2025

KalleOlaviNiemitalo commented Mar 17, 2025

baronfel commented Mar 17, 2025

KalleOlaviNiemitalo commented Mar 17, 2025

baronfel commented Mar 25, 2025

mthalman commented Mar 25, 2025

baronfel commented Mar 25, 2025

mthalman commented Mar 25, 2025

baronfel commented Mar 26, 2025

mthalman commented Mar 26, 2025

KalleOlaviNiemitalo Apr 4, 2025

KalleOlaviNiemitalo Apr 4, 2025

KalleOlaviNiemitalo Apr 4, 2025

Create System.Diagnostic.Activity spans for parsing and invocation of S.CL commands #2529

Are you sure you want to change the base?

Create System.Diagnostic.Activity spans for parsing and invocation of S.CL commands #2529

Conversation

baronfel commented Mar 13, 2025 • edited Loading

KalleOlaviNiemitalo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KalleOlaviNiemitalo commented Mar 14, 2025

KalleOlaviNiemitalo commented Mar 14, 2025

baronfel commented Mar 14, 2025

baronfel commented Mar 17, 2025

KalleOlaviNiemitalo commented Mar 17, 2025

baronfel commented Mar 17, 2025

KalleOlaviNiemitalo commented Mar 17, 2025

baronfel commented Mar 25, 2025

mthalman commented Mar 25, 2025

baronfel commented Mar 25, 2025

mthalman commented Mar 25, 2025

baronfel commented Mar 26, 2025

mthalman commented Mar 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baronfel commented Mar 13, 2025 •

edited

Loading