-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Improve the logging error handling in our ServiceHub MEF creation #80124
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
Improve the logging error handling in our ServiceHub MEF creation #80124
Conversation
We use this log since if things go terribly bad during initialization we might not actually get error information back to the devenv process, and we don't have telemetry active at that point to report to either.
The CompositionErrors collection is ordered -- the first set of errors are the "root" errors, then there are follow up sets that are downstream failures from those first ones. Since our code was only looking at the first set, it's possible that an "expected" failure there might still cause downstream issues which could break the rest of the system.
We already have this, it seems odd not to pass it along.
} | ||
|
||
protected override void LogTrace(string message) | ||
{ | ||
_traceLogger.TraceEvent(TraceEventType.Information, 0, message); |
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.
Is Information the level that normally gets logged? I think it would be good if these are logged by default (since it only happens once at startup and isn't generally in a user facing log)
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.
Oh right I forgot to dig into this: the default seems to be warning-level stuff gets logged only. We need to figure out where that configuration comes from.
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.
It's not configurable. The belief is anything interesting for now we're logging at warning or higher, and we should ask the ServiceHub folks for a setting to let this be specified differently in the manifest.
@@ -208,7 +208,7 @@ private bool CheckForAndReportCompositionErrors(CompositionConfiguration configu | |||
catch (CompositionFailedException ex) | |||
{ | |||
// The ToString for the composition failed exception doesn't output a nice set of errors by default, so log it separately | |||
LogError($"Encountered errors in the MEF composition: {ex.Message}{Environment.NewLine}{ex.ErrorsAsString}"); | |||
LogError($"Encountered errors in the MEF composition: {ex.ErrorsAsString}", ex); |
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.
Errors as string can be pretty long (and IIRC contains new lines itself) - probably should be started on a new line?
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.
Newline added.
/backport to release/dev18.0 |
Started backporting to release/dev18.0: https://github.com/dotnet/roslyn/actions/runs/17448217047 |
This improves some logging and handling in error situations for our ServiceHub MEF creation. Review commit-at-a-time.