-
Notifications
You must be signed in to change notification settings - Fork 229
Add More Data For RemoteInvocationException #8240
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
Changes from 4 commits
5b8c32f
9159bd0
312ae9b
5eea380
355da12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
|
||
| using System; | ||
|
|
||
| namespace Microsoft.AspNetCore.Razor.Telemetry; | ||
|
|
||
| internal interface IFaultExceptionHandler | ||
| { | ||
| bool HandleException(ITelemetryReporter reporter, Exception exception, string? message, object?[] @params); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,17 +17,21 @@ namespace Microsoft.AspNetCore.Razor.Telemetry; | |
| internal class TelemetryReporter : ITelemetryReporter | ||
| { | ||
| private readonly ImmutableArray<TelemetrySession> _telemetrySessions; | ||
| private readonly IEnumerable<IFaultExceptionHandler> _faultExceptionHandlers; | ||
| private readonly ILogger? _logger; | ||
|
|
||
| [ImportingConstructor] | ||
| public TelemetryReporter([Import(AllowDefault = true)] ILoggerFactory? loggerFactory = null) | ||
| : this(ImmutableArray.Create(TelemetryService.DefaultSession), loggerFactory) | ||
| public TelemetryReporter( | ||
| [Import(AllowDefault = true)] ILoggerFactory? loggerFactory = null, | ||
| [ImportMany] IEnumerable<IFaultExceptionHandler>? faultExceptionHandlers = null) | ||
| : this(ImmutableArray.Create(TelemetryService.DefaultSession), loggerFactory, faultExceptionHandlers) | ||
| { | ||
| } | ||
|
|
||
| public TelemetryReporter(ImmutableArray<TelemetrySession> telemetrySessions, ILoggerFactory? loggerFactory) | ||
| public TelemetryReporter(ImmutableArray<TelemetrySession> telemetrySessions, ILoggerFactory? loggerFactory, IEnumerable<IFaultExceptionHandler>? faultExceptionHandlers) | ||
| { | ||
| _telemetrySessions = telemetrySessions; | ||
| _faultExceptionHandlers = faultExceptionHandlers ?? Array.Empty<IFaultExceptionHandler>(); | ||
| _logger = loggerFactory?.CreateLogger<TelemetryReporter>(); | ||
| } | ||
|
|
||
|
|
@@ -48,7 +52,7 @@ public void ReportEvent<T>(string name, TelemetrySeverity severity, ImmutableDic | |
| Report(telemetryEvent); | ||
| } | ||
|
|
||
| public void ReportFault(Exception exception, string? message, object[] @params) | ||
| public void ReportFault(Exception exception, string? message, params object?[] @params) | ||
| { | ||
| try | ||
| { | ||
|
|
@@ -69,6 +73,20 @@ public void ReportFault(Exception exception, string? message, object[] @params) | |
| return; | ||
| } | ||
|
|
||
| var handled = false; | ||
| foreach (var handler in _faultExceptionHandlers) | ||
| { | ||
| if (handler.HandleException(this, exception, message, @params)) | ||
| { | ||
| handled = true; | ||
| } | ||
| } | ||
|
|
||
| if (handled) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var currentProcess = Process.GetCurrentProcess(); | ||
|
|
||
| var faultEvent = new FaultEvent( | ||
|
|
@@ -78,6 +96,16 @@ public void ReportFault(Exception exception, string? message, object[] @params) | |
| exceptionObject: exception, | ||
| gatherEventDetails: faultUtility => | ||
| { | ||
| foreach (var data in @params) | ||
| { | ||
| if (data is null) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| faultUtility.AddErrorInformation(data.ToString()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. whats a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is part of the Fault API we consume from VS |
||
| } | ||
|
|
||
| // Returning "0" signals that, if sampled, we should send data to Watson. | ||
| // Any other value will cancel the Watson report. We never want to trigger a process dump manually, | ||
| // we'll let TargetedNotifications determine if a dump should be collected. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
|
||
| using System; | ||
| using Microsoft.AspNetCore.Razor.Telemetry; | ||
| using StreamJsonRpc; | ||
|
|
||
| namespace Microsoft.AspNetCore.Razor.LanguageServer; | ||
|
|
||
| internal class JsonRPCFaultExceptionHandler : IFaultExceptionHandler | ||
| { | ||
| public bool HandleException(ITelemetryReporter reporter, Exception exception, string? message, object?[] @params) | ||
| { | ||
| if (exception is not RemoteInvocationException remoteInvocationException) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| reporter.ReportFault(remoteInvocationException, remoteInvocationException.Message, remoteInvocationException.ErrorCode, remoteInvocationException.ErrorData, remoteInvocationException.DeserializedErrorData); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the actual exception give us no useful information at all? I would have thought we'd somehow want to combine all of the info together.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does, and we're still keeping that info. We're just adding more now |
||
| return true; | ||
| } | ||
| } | ||
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.
My first thought was that we should stop iterating after it's been handled, but we could easily handle it twice (one to log and one to attempt recovery perhaps?), and so long as there aren't a ton of Exception handlers it shouldn't matter too much. No real suggestion, just putting some thoughts out there in case you can make better use of them than me.
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.
Yea I wasn't sure what the best behavior here would be. I think it's just a best guess until we add more
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.
I agree. It might be worth adding a comment so that we remember that we weren't quite sure about this behavior when it was written.