trace2: improve lifecycle and instrumentation - #2408
Open
mjcheetham wants to merge 18 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
Moves Trace2 to a process-wide lifecycle and adds instrumentation across command, authentication, configuration, storage, process, and UI workflows.
Changes:
- Reworks Trace2 messages, writers, process correlation, and logical contexts.
- Removes injected Trace2 dependencies and centralizes exception reporting.
- Adds command, data, UI, authentication, and credential-store instrumentation.
Reviewed changes
Copilot reviewed 97 out of 99 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/TestInfrastructure/Objects/TestSettings.cs |
Removes injected Trace2 settings. |
src/TestInfrastructure/Objects/TestCommandContext.cs |
Removes context-owned Trace2. |
src/TestInfrastructure/Objects/NullTrace.cs |
Removes obsolete Trace2 stub. |
src/TestInfrastructure/GitTestUtilities.cs |
Updates child-process API usage. |
src/Microsoft.AzureRepos/AzureReposHostProvider.cs |
Removes tracing-specific exceptions. |
src/Microsoft.AzureRepos/AzureDevOpsRestApi.cs |
Uses standard exceptions. |
src/Microsoft.AzureRepos.Tests/AzureReposHostProviderTests.cs |
Updates exception assertions. |
src/Microsoft.AzureRepos.Tests/AzureDevOpsApiTests.cs |
Updates exception assertions. |
src/GitLab/UI/Commands/CredentialsCommand.cs |
Uses standard cancellation exception. |
src/GitLab/GitLabOAuth2Client.cs |
Removes Trace2 dependency. |
src/GitLab/GitLabHostProvider.cs |
Uses standard exceptions. |
src/GitLab/GitLabAuthentication.cs |
Removes injected tracing and specialized exceptions. |
src/GitLab.Tests/GitLabAuthenticationTests.cs |
Updates exception assertions. |
src/GitHub/UI/Commands/TwoFactorCommand.cs |
Uses standard cancellation exception. |
src/GitHub/UI/Commands/DeviceCommand.cs |
Uses standard cancellation exception. |
src/GitHub/UI/Commands/CredentialsCommand.cs |
Uses standard cancellation exception. |
src/GitHub/GitHubOAuth2Client.cs |
Removes Trace2 dependency. |
src/GitHub/GitHubHostProvider.cs |
Uses process-wide Trace2. |
src/GitHub/GitHubAuthentication.cs |
Adds authentication instrumentation. |
src/GitHub.Tests/GitHubHostProviderTests.cs |
Updates exception assertions. |
src/GitHub.Tests/GitHubAuthenticationTests.cs |
Updates exception assertions. |
src/git-credential-manager/Program.cs |
Owns Trace2 process lifecycle. |
src/Core/WslUtils.cs |
Updates WSL child-process creation. |
src/Core/UI/HelperApplication.cs |
Reports exceptions through Trace2. |
src/Core/UI/Commands/OAuthCommand.cs |
Uses standard cancellation exception. |
src/Core/UI/Commands/DeviceCodeCommand.cs |
Uses standard cancellation exception. |
src/Core/UI/Commands/DefaultAccountCommand.cs |
Uses standard cancellation exception. |
src/Core/UI/Commands/CredentialsCommand.cs |
Uses standard cancellation exception. |
src/Core/UI/AvaloniaUi.cs |
Instruments UI initialization and windows. |
src/Core/Tty/AnsiConsoleFactory.cs |
Instruments console creation. |
src/Core/Tracing/TraceUtils.cs |
Centralizes source formatting. |
src/Core/Tracing/Trace2Writer.cs |
Makes Trace2 writers abstract. |
src/Core/Tracing/Trace2TextWriter.cs |
Adds text-writer output. |
src/Core/Tracing/Trace2ProcessClass.cs |
Defines child-process classifications. |
src/Core/Tracing/Trace2PipeWriter.cs |
Refactors named-pipe output. |
src/Core/Tracing/Trace2Message.cs |
Defines Trace2 event messages and formats. |
src/Core/Tracing/Trace2FormatTarget.cs |
Defines output formats. |
src/Core/Tracing/Trace2FileWriter.cs |
Simplifies file output. |
src/Core/Tracing/Trace2.cs |
Implements process-wide Trace2. |
src/Core/Tracing/Trace.cs |
Relocates standard tracing. |
src/Core/Trace2StreamWriter.cs |
Removes obsolete stream writer. |
src/Core/Trace2Message.cs |
Removes previous message implementation. |
src/Core/Trace2Exception.cs |
Removes tracing-specific exceptions. |
src/Core/Trace2.cs |
Removes context-owned Trace2. |
src/Core/Settings.cs |
Removes Trace2 settings ownership. |
src/Core/ProcessManager.cs |
Moves process classification to creation. |
src/Core/PlatformUtils.cs |
Removes Trace2 parameters. |
src/Core/Interop/Windows/WindowsProcessManager.cs |
Updates Windows process creation. |
src/Core/Interop/Windows/Native/Win32Error.cs |
Removes tracing-specific interop errors. |
src/Core/HttpClientFactory.cs |
Uses global Trace2 reporting. |
src/Core/HostProviderRegistry.cs |
Uses global Trace2 reporting. |
src/Core/Gpg.cs |
Updates process tracing and exceptions. |
src/Core/GitConfiguration.cs |
Instruments configuration operations. |
src/Core/Git.cs |
Updates Git process tracing. |
src/Core/GenericHostProvider.cs |
Removes Trace2 injection. |
src/Core/Diagnostics/GitDiagnostic.cs |
Updates child-process API usage. |
src/Core/Diagnostics/EnvironmentDiagnostic.cs |
Removes Trace2 parameter. |
src/Core/CredentialStore.cs |
Instruments credential-store operations. |
src/Core/Constants.cs |
Removes Trace2 user-agent dependency. |
src/Core/ConsoleService.cs |
Lazily creates consoles. |
src/Core/Commands/StoreCommand.cs |
Instruments provider storage. |
src/Core/Commands/GitCommandBase.cs |
Instruments command parsing and dispatch. |
src/Core/Commands/GetCommand.cs |
Instruments provider retrieval. |
src/Core/Commands/EraseCommand.cs |
Instruments provider erasure. |
src/Core/Commands/DiagnoseCommand.cs |
Instruments diagnostics. |
src/Core/Commands/ConfigurationCommands.cs |
Instruments configuration commands. |
src/Core/Commands/CapabilityCommand.cs |
Instruments capability command. |
src/Core/CommandContext.cs |
Removes context-owned Trace2. |
src/Core/ChildProcess.cs |
Adds stable Trace2 child IDs and timing. |
src/Core/Authentication/OAuthAuthentication.cs |
Uses standard exceptions. |
src/Core/Authentication/OAuth/OAuth2Client.cs |
Adds OAuth instrumentation. |
src/Core/Authentication/Entra/EntraAuthentication.PublicClient.cs |
Uses standard exceptions. |
src/Core/Authentication/Entra/EntraAuthentication.Caching.cs |
Uses global Trace2 reporting. |
src/Core/Authentication/BasicAuthentication.cs |
Uses standard exceptions. |
src/Core/Authentication/AuthenticationBase.cs |
Removes injected Trace2 usage. |
src/Core/Application.cs |
Instruments command setup and errors. |
src/Core.Tests/WslUtilsTests.cs |
Updates WSL process tests. |
src/Core.Tests/Trace2Tests.cs |
Adds SID and depth tests. |
src/Core.Tests/Trace2MessageTests.cs |
Tests new Trace2 event formats. |
src/Core.Tests/TestProcessManager.cs |
Updates process manager test double. |
src/Core.Tests/ProcessManagerTests.cs |
Removes obsolete SID tests. |
src/Core.Tests/HttpClientFactoryTests.cs |
Updates factory construction. |
src/Core.Tests/HostProviderRegistryTests.cs |
Updates exception assertions. |
src/Core.Tests/GitTests.cs |
Updates Git construction. |
src/Core.Tests/GitConfigurationTests.cs |
Updates Git construction. |
src/Core.Tests/ConsoleServiceTests.cs |
Tests lazy console factories. |
src/Core.Tests/Authentication/OAuth2ClientTests.cs |
Removes Trace2 test dependency. |
src/Core.Tests/Authentication/Entra/EntraAuthenticationTests.cs |
Updates exception assertions. |
src/Core.Tests/Authentication/BasicAuthenticationTests.cs |
Updates exception assertions. |
src/Atlassian.Bitbucket/UI/Commands/CredentialsCommand.cs |
Uses standard cancellation exception. |
src/Atlassian.Bitbucket/OAuth2ClientRegistry.cs |
Removes Trace2 client injection. |
src/Atlassian.Bitbucket/DataCenter/BitbucketOAuth2Client.cs |
Removes Trace2 dependency. |
src/Atlassian.Bitbucket/Cloud/BitbucketOAuth2Client.cs |
Removes Trace2 dependency. |
src/Atlassian.Bitbucket/BitbucketOAuth2Client.cs |
Updates base OAuth construction. |
src/Atlassian.Bitbucket/BitbucketHostProvider.cs |
Uses global Trace2 reporting. |
src/Atlassian.Bitbucket/BitbucketAuthentication.cs |
Adds authentication instrumentation. |
src/Atlassian.Bitbucket.Tests/DataCenter/BitbucketOAuth2ClientTest.cs |
Updates client construction. |
src/Atlassian.Bitbucket.Tests/Cloud/BitbucketOAuth2ClientTest.cs |
Updates client construction and assertions. |
docs/development.md |
Documents added Trace2 events. |
Suppressed comments (1)
src/Core.Tests/Trace2Tests.cs:90
- This
catchconverts every assertion failure into a passing test and fails to restore the process environment on success. Change it tofinallyso cleanup is unconditional without suppressing failures.
catch
{
// Restore original environment variable for this process
Environment.SetEnvironmentVariable(Trace2.SidEnvar, originalSid);
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Gather tracing implementation files under a dedicated directory so the subsequent behavioral changes are easier to review. Rename writer source files at the same time and align the public region-scope API with its lifetime by naming the operation StartRegion. Folding that terminology into the source reorganization gives every later change the final scope vocabulary instead of preserving a transitional API name that would need a standalone cleanup. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Consolidate event serialization and normal/performance formatting so each message declares only its event-specific data. Extract the shared format and process classifications, and align writer implementations with their source names to make later lifecycle changes smaller and clearer. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
TRACE2 represents one process-wide event stream, so an injected instance obscures its lifetime and makes region state appear isolatable when it is not. Make event emission and writer state static first so the API shape can be reviewed apart from startup ownership and the downstream constructor migration. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Tracing begins before CommandContext exists and survives until dispatcher shutdown, so injected services create a circular dependency for process identity and settings. Let Trace2 establish its SID, depth, targets, and initial events at entry, then close writers after dispatch ends. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
CommandContext should expose replaceable execution services, not own a process-wide event stream. Removing TRACE2 from that surface makes the remaining injected dependencies explicit and avoids preserving a fake test tracer for state that can no longer vary by context. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Core infrastructure cannot meaningfully substitute a process-wide tracer, yet carrying it through process, Git, HTTP, and platform constructors forces every helper to mirror global lifetime. Remove that dependency from the service layer and emit its existing events through the static API, while leaving leaf exception conversion for the final pass. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
OAuth clients only forward the tracer so every provider can emit into the same process stream. That parameter creates a constructor cascade without representing per-client state. Remove it at the OAuth boundary so provider wiring follows the real ownership model, while reserving leaf exception changes for the final cleanup. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
After constructor ownership is gone, leaf exception wrappers and command call sites still refer to a tracer that cannot vary by caller. Route those final authentication, provider, storage, UI, and Win32 events through the process API so the migration becomes buildable again without introducing an adapter that would immediately disappear. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Exception subclasses that emit while being constructed can report failures more than once and make ordinary error handling depend on tracing. Emit once at the application boundary, preserve the richer Git and interop formats there, and return call sites to standard exception types. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Child start and exit events need a stable correlation identity and timing that survives fast process termination. Give each child wrapper its own Trace2 identifier, classify it when it is created, and report the operating system exit timestamp from the process notification. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Physical managed threads do not describe asynchronous work once execution crosses awaits or dispatcher boundaries. Flow an explicit Trace2 context with the execution context so thread identities, nested regions, and relative timing remain coherent, while allowing callers to restore the main context when needed. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Repeated fully qualified caller-info attribute names obscure the Trace2 API signatures. Import the namespace to keep those signatures readable without changing behavior. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
A process start records argv but does not identify which command parser branch ultimately won. Emit the canonical command name after parsing and carry an inherited hierarchy through child processes so nested helper activity can be related to its originating command. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Regions describe elapsed work but cannot explain the values that shaped that work. Add scalar and structured data events whose thread, nesting, and relative timing come from the active logical context, allowing later instrumentation to attach useful measurements without inventing new regions. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Top-level traces currently stop before command parsing and provider selection, leaving the most important dispatch decisions invisible. Add focused regions and metadata around setup, input parsing, provider resolution, and each command entry point so command execution can be followed end to end. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Command dispatch alone cannot explain time spent discovering Git, reading configuration, or opening the selected credential store. Trace those service boundaries and record the configuration scope and backing-store choices that drive their behavior. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Eager console creation and dispatcher hand-offs hide UI startup cost and can initialize terminal state before it is needed. Create consoles lazily and trace ANSI, Avalonia, rendering, and window-display boundaries so interactive latency is attributed to the correct lifecycle stage. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Authentication spans browser, device-code, refresh, account selection, and provider-specific decisions that are otherwise indistinguishable in a trace. Add regions and mode data at those boundaries so delays and user-flow choices can be diagnosed without recording credentials. Assisted-by: GPT-5.6 Sol Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GCM already emits Trace2 events, but the implementation is owned by
CommandContexteven though Trace2 represents a single process-wide event stream. This prevents tracing from starting before context construction and requires the tracer to be passed through services that cannot meaningfully replace it.The existing implementation also attributes asynchronous work to physical managed threads, reports some errors through tracing-specific exception types, and leaves major command, UI, and authentication boundaries uninstrumented.
This PR:
Trace2 continues to honour the existing Git environment variables and configuration settings, and supports event, normal, and performance output formats. The added instrumentation records control-flow and configuration choices without recording credentials.
The series is organised as follows:
To keep the cross-cutting ownership migration reviewable, commits 3-7 are intentionally not independently buildable. Commit 8 completes the migration and restores a buildable tree. The remaining commits build normally.