trace2: add regions #1162
Merged
trace2: add regions #1162
Conversation
This file contains 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
ldennington
force-pushed
the
regions
branch
3 times, most recently
from
3369871 to
9e54a39
Compare
|
Marked as ready for review, since the only remaining failures are due to the expired CodeQL certificate. |
|
Latest push adds a new commit that writes exit message times in UTC rather than local time. @jeffhostetler and I discovered this little bug while doing some testing with App Insights this morning. |
derrickstolee
approved these changes
Mar 28, 2023
mjcheetham
reviewed
Mar 28, 2023
mjcheetham
approved these changes
Mar 28, 2023
Move Trace2 messages into a new file. The Trace2 file was getting a bit long and unwieldy, so this change is intended to separate it in a logical way into two smaller files that are (hopefully) easier to parse and reason about.
In Git, dots are only used on nested region messages, which are currently not supported in GCM. Remove the AddDots() method that is used incorrectly for formatting child process messages.
Currently, we are recording child process times incorrectly for the performance format target. This change fixes that by: 1. Ensuring we're recording the absolute time in child_start messages for the performance format target, which we previously were not doing. 2. Ensures we do not conflate the absolute and relative times in child_exit, which we were previously doing.
We are currently writing exit message times in local rather than UTC time. This change updates to UTC time to align with the expectations of the Trace2 API.
Add descriptive comments to Trace2-related classes that currently do not have any.
With the upcoming introduction of regions, we will need to capture thread names. This change adds a new static BuildThreadName() method that defines a thread name by: 1. Determining if it is the entry thread and, if so, calling it "main", per Trace2 convention. 2. If it's not the main thread, determining whether it is a thread pool thread and naming it with a static prefix and the thread pool thread id. 3. If it is not the entry thread or a thread pool thread, use the thread name, if it has one. 4. Return an empty string if none of the above are true.
Previously, we only set up custom performance format span handling for time-related information (see [1]). However, with the upcoming introduction of regions, we will need custom spans for the category and repo strings. This change refactors and adds to the current logic to correctly handle all 3 span types (i.e. time, repo, and category). [1] 59a2692
Add supporting infrastructure for regions. This includes the normal `Write` methods in Trace2 and corresponding Trace2 messages with a few notable distinctions: 1. There is an abstract parent RegionMessage that inherits from Trace2Message. The RegionEnterMessage and RegionLeave messages inherit from this parent (rather than Trace2Message) to maximize reuse of shared fields. 2. We create a new Region class that implements DisposableObject and call it using a CreateRegion method in Trace2. This ensures that we call WriteRegionLeave before a Region object is disposed - a key requirement of Git's Trace2 API for regions. Additionally, it allows us to manage getting the executing thread name and region timings within the class, thus removing this burden from consumers. An additional note is that the region_enter and region_leave events will always have the same line number with this model. This was deemed to be acceptable, however, given that knowing where a region begins executing provides enough information for inspection of the region if it is needed.
Add region tracing to key methods in OAuth2Client. This was deemed a good starting point for regions due to the server interaction required to obtain tokens. Additional regions can and should be added to additional sections of the code that are deemed performance-critical in the future.
Document how users can set up and utilize the Trace2 API.
mjcheetham
added a commit
that referenced
this pull request
May 2, 2023
**Changes:** - Support ports in URL-scoped config (#825) - Support URL-scoped enterprise default settings (#1149) - Add support for client TLS certificates (#1152) - Add TRACE2 support(#1131, #1151, #1156, #1162) - Better browser detection inside of WSL (#1148) - Handle expired OAuth refresh token for generic auth (#1196) - Target *-latest runner images in CI workflow (#1178) - Various bug fixes: - Ensure we create a WindowsProcessManager on Windows (#1146) - Ensure we start child processes created with ProcessManager (#1177) - Fix app path name of Windows dropping file extension (#1181) - Ensure we init IEnvironment before SessionManager (#1167) - git: consistently read from stdout before exit wait (#1136) - trace2: guard against null pipe client in dispose (#1135) - Make Avalonia UI the default Windows and move to in-process (#1207) - Add Git configuration options for trace & debug (#1228) - Transition from Nerdbank.GitVersioning to a version file (#1231) - Add support for using the current Windows user for WAM on DevBox (#1197) - Various documentation updates: - org-rename: update references to GitCredentialManager (#1141) - issue templates: remove core suffix (#1180) - readme: add link to project roadmap (#1204) - docs: add bitbucket app password requirements (#1213) - .net tool: clarify install instructions (#1126) - docs: call out different GCM install paths in WSL docs (#1168) - docs: add trace2 to config/env documentation (#1230)
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.
Add
TRACE2region tracing capability.The first six commits contain minor refactors, bug fixes, and cleanups. The first moves Trace2 messages into their own file. The second removes the unnecessary
AddDots()method. The third fixes certain conflated timing information inchild_startandchild_exitevents. The fourth ensures we're writing exit message times in UTC. The fifth adds descriptive comments to Trace2-related classes that did not yet have them. The sixth adds best-effort logic to capture thread names.The next three commits are dedicated to preparing for and adding region-related logic. The seventh commit updates custom performance format span handling to account for the repo and category fields, which are used in region tracing. The eighth adds region-related events, encapsulated in a class that inherits from
DisposableObject(which in turn inherits fromIDisposable). The ninth adds region tracing to methods of interest pertaining to OAuth authentication.The final commit updates GCM's documentation to inform users of how to enable and utilize Trace2 tracing.
A final note: This implementation does not support nested regions, as it is not required for the current regions of interest.