-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add activity spans for template creation and most tool usage #50163
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
base: main
Are you sure you want to change the base?
Changes from all commits
ef5941c
c11acd2
0fd00ff
d101bee
4847bba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,13 +51,13 @@ internal InstantiateCommand( | |
Arity = new ArgumentArity(0, 999) | ||
}; | ||
|
||
internal IReadOnlyList<Option> PassByOptions { get; } = new Option[] | ||
{ | ||
internal IReadOnlyList<Option> PassByOptions { get; } = | ||
[ | ||
SharedOptions.ForceOption, | ||
SharedOptions.NameOption, | ||
SharedOptions.DryRunOption, | ||
SharedOptions.NoUpdateCheckOption | ||
}; | ||
]; | ||
|
||
internal static Task<NewCommandStatus> ExecuteAsync( | ||
NewCommandArgs newCommandArgs, | ||
|
@@ -74,6 +74,7 @@ internal static async Task<IEnumerable<TemplateGroup>> GetTemplateGroupsAsync( | |
HostSpecificDataLoader hostSpecificDataLoader, | ||
CancellationToken cancellationToken) | ||
{ | ||
using var createTemplateGroupsActivity = Activities.Source.StartActivity("create-template-groups"); | ||
IReadOnlyList<ITemplateInfo> templates = await templatePackageManager.GetTemplatesAsync(cancellationToken).ConfigureAwait(false); | ||
return TemplateGroup.FromTemplateList(CliTemplateInfo.FromTemplateInfo(templates, hostSpecificDataLoader)); | ||
} | ||
|
@@ -84,6 +85,7 @@ internal static HashSet<TemplateCommand> GetTemplateCommand( | |
TemplatePackageManager templatePackageManager, | ||
TemplateGroup templateGroup) | ||
{ | ||
using var getTemplateActivity = Activities.Source.StartActivity("get-template-command"); | ||
//groups templates in the group by precedence | ||
foreach (IGrouping<int, CliTemplateInfo> templateGrouping in templateGroup.Templates.GroupBy(g => g.Precedence).OrderByDescending(g => g.Key)) | ||
{ | ||
|
@@ -114,7 +116,7 @@ internal static HashSet<TemplateCommand> GetTemplateCommand( | |
templateGroup, | ||
candidates); | ||
} | ||
return new HashSet<TemplateCommand>(); | ||
return []; | ||
} | ||
|
||
internal static void HandleNoMatchingTemplateGroup(InstantiateCommandArgs instantiateArgs, IEnumerable<TemplateGroup> templateGroups, IReporter reporter) | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OpenTelemetry.Exporter.OpenTelemetryProtocol exports Activity.DisplayName, not Activity.OperationName; so DisplayName is what matters for privacy. |
||
|
||
IEnumerable<TemplateGroup> allTemplateGroups = await GetTemplateGroupsAsync( | ||
templatePackageManager, | ||
|
@@ -273,10 +277,11 @@ private static async Task<NewCommandStatus> HandleTemplateInstantiationAsync( | |
{ | ||
TemplateCommand templateCommandToRun = candidates.Single(); | ||
args.Command.Subcommands.Add(templateCommandToRun); | ||
|
||
var templateParseActivity = Activities.Source.StartActivity("reparse-for-template"); | ||
ParseResult updatedParseResult = args.ParseResult.RootCommandResult.Command.Parse( | ||
args.ParseResult.Tokens.Select(t => t.Value).ToArray(), | ||
args.ParseResult.Configuration); | ||
templateParseActivity?.Stop(); | ||
return await candidates.Single().InvokeAsync(updatedParseResult, cancellationToken).ConfigureAwait(false); | ||
} | ||
else if (candidates.Any()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,16 +41,23 @@ internal class ToolExecuteCommand(ParseResult result, ToolManifestFinder? toolMa | |
|
||
public override int Execute() | ||
{ | ||
VersionRange versionRange = _parseResult.GetVersionRange(); | ||
VersionRange? versionRange = _parseResult.GetVersionRange(); | ||
PackageId packageId = new PackageId(_packageToolIdentityArgument.Id); | ||
|
||
var toolLocationActivity = Activities.Source.StartActivity("find-tool"); | ||
toolLocationActivity?.SetTag("packageId", packageId.ToString()); | ||
toolLocationActivity?.SetTag("versionRange", versionRange?.ToString() ?? "latest"); | ||
Comment on lines
+48
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are This PR is adding |
||
|
||
// Look in local tools manifest first, but only if version is not specified | ||
if (versionRange == null) | ||
{ | ||
var localToolsResolverCache = new LocalToolsResolverCache(); | ||
|
||
if (_toolManifestFinder.TryFindPackageId(packageId, out var toolManifestPackage)) | ||
{ | ||
toolLocationActivity?.SetTag("kind", "local"); | ||
toolLocationActivity?.Stop(); | ||
|
||
var toolPackageRestorer = new ToolPackageRestorer( | ||
_toolPackageDownloader, | ||
_sources, | ||
|
@@ -82,6 +89,8 @@ public override int Execute() | |
additionalFeeds: _addSource); | ||
|
||
(var bestVersion, var packageSource) = _toolPackageDownloader.GetNuGetVersion(packageLocation, packageId, _verbosity, versionRange, _restoreActionConfig); | ||
toolLocationActivity?.SetTag("kind", "one-shot"); | ||
toolLocationActivity?.Stop(); | ||
|
||
// TargetFramework is null, which means to use the current framework. Global tools can override the target framework to use (or select assets for), | ||
// but we don't support this for local or one-shot tools. | ||
|
@@ -119,6 +128,10 @@ public override int Execute() | |
restoreActionConfig: _restoreActionConfig); | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "source" seems a strange tag name for |
||
var commandSpec = ToolCommandSpecCreator.CreateToolCommandSpec(toolPackage.Command.Name.Value, toolPackage.Command.Executable.Value, toolPackage.Command.Runner, _allowRollForward, _forwardArguments); | ||
var command = CommandFactoryUsingResolver.Create(commandSpec); | ||
var result = command.Execute(); | ||
|
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.
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. Ifdotnet
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.