Add activity spans for template creation and most tool usage #50163
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds activity spans for observability to template creation and tool operations in the .NET CLI. It builds on previous work by adding diagnostic activity tracking to dotnet new template instantiation and various dotnet tool commands including installation, execution, and package downloading.
Key changes include:
- Adding activity spans to tool package downloading and extraction operations
- Instrumenting template creation workflow with diagnostic activities
- Adding activity tracking to tool installation and execution commands
- Adding new localization strings for improved error messaging
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
ToolPackageDownloaderBase.cs |
Adds activity spans for moving global tool content and updating runtime config |
ToolPackageDownloader.cs |
Instruments package download and extraction operations with activities |
ToolRunCommand.cs |
Adds activity span for local tool execution and removes nullable disable directive |
ToolInstallGlobalOrToolPathCommand.cs |
Comprehensive refactoring with nullable reference types and activity instrumentation for tool installation |
ToolExecuteCommand.cs |
Adds activity spans for tool location and execution operations |
TemplateInvoker.cs |
Instruments template invocation and creation processes with activities |
TemplateCommand.cs |
Adds activity spans for template constraint validation and invocation |
InstantiateCommand.cs |
Instruments template group creation and command parsing with activities |
Activities.cs |
Adds constants for trace parent environment variables |
Various .xlf files |
Adds new localization entries for tool installation error message |
CliCommandStrings.resx |
Adds new error message for missing package ID in tool installation |
src/Cli/dotnet/Commands/Tool/Install/ToolInstallGlobalOrToolPathCommand.cs
Show resolved
Hide resolved
| @@ -204,6 +206,8 @@ private static async Task<NewCommandStatus> ExecuteIntAsync( | |||
|
|
|||
| return await templateListCoordinator.DisplayCommandDescriptionAsync(instantiateArgs, cancellationToken).ConfigureAwait(false); | |||
| } | |||
| using var createActivity = Activities.Source.StartActivity("instantiate-command"); | |||
| createActivity?.DisplayName = $"Invoke '{instantiateArgs.ShortName}'"; | |||
There was a problem hiding this comment.
Is instantiateArgs.ShortName already part of the telemetry?
There was a problem hiding this comment.
OpenTelemetry.Exporter.OpenTelemetryProtocol exports Activity.DisplayName, not Activity.OperationName; so DisplayName is what matters for privacy.
| toolLocationActivity?.SetTag("packageId", packageId.ToString()); | ||
| toolLocationActivity?.SetTag("versionRange", versionRange?.ToString() ?? "latest"); |
There was a problem hiding this comment.
Are packageId and versionRange already part of the telemetry? packageId could easily refer to a private package. I assume these strings won't be hashed before they are exported over OTLP.
This PR is adding packageId to other activities too. I won't repeat this comment for those.
| using var toolExecuteActivity = Activities.Source.StartActivity("execute-tool"); | ||
| toolExecuteActivity?.SetTag("packageId", packageId.ToString()); | ||
| toolExecuteActivity?.SetTag("version", toolPackage.Version.ToString()); | ||
| toolExecuteActivity?.SetTag("source", toolPackage.Command.Runner); |
There was a problem hiding this comment.
"source" seems a strange tag name for toolPackage.Command.Runner.
| /// <summary> | ||
| /// The environment variable used to transfer the chain of parent activity IDs. | ||
| /// This should be used when constructing new sub-processes in order to | ||
| /// track spans across calls. | ||
| /// </summary> | ||
| public const string TRACEPARENT = nameof(TRACEPARENT); | ||
| /// <summary> | ||
| /// The environment variable used to transfer the trace state of the parent activities. | ||
| /// This should be used when constructing new sub-processes in order to | ||
| /// track spans across calls. | ||
| /// </summary> | ||
| public const string TRACESTATE = nameof(TRACESTATE); |
There was a problem hiding this comment.
TRACEPARENT and TRACESTATE don't seem to be used in this PR yet.
I wouldn't use nameof here. If the constants are ever renamed (e.g. to mixed case) in the source code, their string values should remain unchanged. Instead #pragma warning disable CA1507.
In #49668 (comment), I asked about possible interference between instrumentation of the .NET SDK and instrumentation of tools. For example, when a system sets the TRACEPARENT environment variable, runs a tool via dotnet, and expects the tool to receive this TRACEPARENT value. If dotnet changes TRACEPARENT, and sends its own telemetry to Microsoft rather than to the same endpoint as the tool, then the trace will be somewhat broken as it will not be possible to find the correct parent span. I expect the trace ID will be preserved though.
This PR continues the slicing off of the sub-parts of #49409 because it's so big.
This part builds on the previous PR, which introduced the ActivitySource for the CLI and the System.Diagnostics.DiagnosticsSource PackageReference. It adds activity usage to
dotnet newinvocation, and many parts ofdotnet toolusage (includingdnxand package downloading).None of these actually light up in the current
mainbranch because there are no listeners for the activities created.