-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add ActivitySource telemetry to Microsoft.Dotnet.Installation #51580
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
Add ActivitySource telemetry to Microsoft.Dotnet.Installation #51580
Conversation
- Added System.Diagnostics.DiagnosticSource package reference - Created ActivitySource for Microsoft.Dotnet.Installer library - Added Activity for DotnetInstaller.Install with tags for install root, arch, component, and version - Added sub-activities for Prepare and Commit stages in ArchiveDotnetExtractor Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
|
|
||
| internal static class InstallationActivitySource | ||
| { | ||
| private static readonly ActivitySource s_activitySource = new("Microsoft.Dotnet.Installer", "1.0.0"); |
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.
@copilot can you get the version of the MIcrosoft.Dotnet.Installation dll from assembly attributes and use it here in stead of 1.0.0?
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.
Done in f92fa73. The ActivitySource now uses AssemblyInformationalVersionAttribute to get the version from the Microsoft.Dotnet.Installation assembly, with "1.0.0" as a fallback.
Get version from AssemblyInformationalVersionAttribute with "1.0.0" as fallback Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
|
|
||
| public void Commit(IEnumerable<ReleaseVersion> existingSdkVersions) | ||
| { | ||
| using var activity = InstallationActivitySource.ActivitySource.StartActivity("DotnetInstaller.Commit"); |
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.
Should we be using a try-catch pattern to log exceptions which are thrown? I don't know if that's done automatically.
catch (Exception ex)
{
activity?.SetStatus(ActivityStatusCode.Error);
activity?.AddEvent(new ActivityEvent("exception", tags: new ActivityTagsCollection {
{ "exception.type", ex.GetType().FullName },
{ "exception.message", ex.Message },
{ "exception.stacktrace", ex.ToString() }
}));
throw;
}
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 good news is that this is automatically done for us. The BCL listeners already catch and finalize activities where exceptions are thrown in much the same way that you've suggested here. The whole system does so much correct out of the box for you, it's really a joy.
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.
That is awesome!
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 BCL listeners already catch and finalize activities where exceptions are thrown in much the same way that you've suggested here.
I searched for ActivityStatusCode.Error references at https://source.dot.net/ but I don't see any activity listeners setting that. AFAICT, it's instead done by the code that has an ActivitySource and starts and stops an Activity. I therefore don't agree with "this is automatically done for us".
nagilson
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.
Thanks, I think this is a great idea. I didn't know about system.diagnostics.
It looks like it collects perf data for us too, which is great. We should likely inform aspire once this is merged how to enable the telemetry.
Implementation Complete ✅
Summary
This implementation adds System.Diagnostics.Activities support to enable Aspire and other telemetry tools to collect installation metrics before the SDK's own telemetry is initialized.
Key Changes
Created
InstallationActivitySource.cs- Contains a static ActivitySource with name "Microsoft.Dotnet.Installer" using the assembly's InformationalVersionModified
DotnetInstaller.Install- Wraps the installation process in an Activity named "DotnetInstaller.Install" with tags for:install.root- The installation pathinstall.arch- The target architecture (x86, x64, arm64)install.component- The component type (SDK, Runtime, etc.)install.version- The version being installedAdded sub-activities in
ArchiveDotnetExtractor:DotnetInstaller.Prepare- Tracks the download/preparation phaseDotnetInstaller.Commit- Tracks the extraction/installation phaseDesign Decisions
install.*prefix for tag names to follow OpenTelemetry semantic conventions.The implementation is minimal, focused, and follows standard patterns for OpenTelemetry instrumentation in .NET.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.