trace2: add error event #1156
Merged
trace2: add error event #1156
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
error-event
branch
from
2bb8414
to
c1aa4ae
Compare
ldennington
force-pushed
the
error-event
branch
from
c1aa4ae
to
4ac5248
Compare
ldennington
force-pushed
the
error-event
branch
from
4ac5248
to
111e5b9
Compare
|
This looks good. I did have a few comments. Sorry that it causes so much churn in the code to squeeze it in. |
jeffhostetler
approved these changes
Mar 17, 2023
|
Oh, I did find it a little odd that the Windows terminal didn't need to be updated, but that's more of a curiosity than a question. |
|
And yes, you mentioned somewhere about not getting every error/exception because of excess churn, and that is fine. |
TRACE2 convention does not require file names to be lowercase. Remove the ToLower() call when setting messages' FileName properties, as it it unnecessary.
I just didn't want to pass it an extra parameter that was unused, since that class doesn't have any exceptions. |
ldennington
force-pushed
the
error-event
branch
3 times, most recently
from
894c2f1
to
19d031d
Compare
|
I looked thru the entire series. I had a few comments, but it all looks good. |
Order was originally included for each TRACE2 json property to ensure alignment with Git's TRACE2 json property ordering. However, given that the json output is meant for consumption only by tooling (e.g. the OTel Collector) andi that the order requirement adds overhead with each new message, this change removes it.
Pass instance of Trace2 to OAuth2Client in preparation for capturing exceptions, which will be added in a future commit. Note that this also removes the unused instance of Trace in GCM's OAuth2 infrastructure.
Pass instance of Trace2 to Git in preparation for capturing exceptions, which will be added in a future commit.
Pass instance of Trace2 to terminal classes in preparation for capturing exceptions, which will be added in a future commit.
Pass instance of Trace2 to Gpg in preparation for capturing exceptions, which will be added in a future commit.
Add error event, which will capture exceptions, to Trace2 tracing system.
Add exceptions for which TRACE2 tracing can optionally be used. We choose to add these exceptions to ease the experience of writing TRACE2 error information - instead of having to add a call to Trace2's WriteError() message before every exception of interest, one can instead just use the appropriate Trace2 exception.
ldennington
force-pushed
the
error-event
branch
from
19d031d
to
e898ae7
Compare
|
this looks good. thanks! |
Thank you! I will plan to clean up the test failures and merge! |
ldennington
force-pushed
the
error-event
branch
2 times, most recently
from
06fec95
to
23196d7
Compare
Add TRACE2 error tracing statements throughout GCM codebase. Note that this is a best effort - there are certain places (most notably certain static and unsafe methods) where statements were purposefully not added because it is either not safe or was deemed to much lift/churn to do so. Note that there are some instances in which a "parameterized" message is passed to TRACE2. This is used only when PII information could be passed into the message (e.g. through a path or remote URI) and should be redacted for collection purposes. Finally, there are certain instances in which we write an error with no exception thrown. These align with the places where we are currently writing to GCM Trace without throwing an error for the purposes of parity.
ldennington
force-pushed
the
error-event
branch
from
23196d7
to
94f8d91
Compare
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.
This series adds the
TRACE2error event.The first two commits are opportunistic fixes that remove unnecessary components of
Trace2messages. The next four commits addTrace2as a dependency to certain classes where exceptions are thrown in order to capture those exceptions with the new error event. The seventh commit adds the error event, and eighth adds special exceptions that write toTrace2. Finally, the ninth commit adds error tracing exceptions and messages throughout the GCM codebase.