Skip to content

Conversation

@ryzngard
Copy link
Contributor

@ryzngard ryzngard commented Feb 8, 2023

Add specific data from RemoteInvocationException to Watson cabs that get uploaded for reporting telemetry faults

@ryzngard ryzngard requested a review from a team as a code owner February 8, 2023 02:05
return false;
}

reporter.ReportFault(remoteInvocationException, remoteInvocationException.Message, remoteInvocationException.ErrorCode, remoteInvocationException.ErrorData, remoteInvocationException.DeserializedErrorData);
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

continue;
}

faultUtility.AddErrorInformation(data.ToString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whats a faultUtility and what does it do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of the Fault API we consume from VS

{
if (handler.HandleException(this, exception, message, @params))
{
handled = true;
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor

@maryamariyan maryamariyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@ryzngard ryzngard enabled auto-merge (squash) February 9, 2023 23:34
@ryzngard ryzngard merged commit 97d174a into dotnet:main Feb 10, 2023
333fred added a commit to 333fred/razor that referenced this pull request Feb 15, 2023
* upstream/main: (29 commits)
  Consolidate roslyn versions (dotnet#8103)
  Don't set always-auth
  Fix darc publishing (dotnet#8275)
  Update LSP protocol DLL version
  reducing line comment length
  Avoid killing the process when a login window is closed.
  Clean up serialization benchmarks
  Accepting applicationUrl with more than one url. "applicationUrl": "http://localhost:7104;https://localhost:7105"
  Removing more code related to blazorwasmdebuggingextension
  Add More Data For RemoteInvocationException (dotnet#8240)
  Trying to completely remove the BlazorWasmDebuggingExtension and getting URL information from launchSettings.json
  Cleanup
  Fix TextMate missing item
  Use ImmutableHashSet rather than HashSet in LegacySyntaxNodeExtensions
  Use more efficient stack for FlattenSpansInReverse
  Use an object pool rather than a thread-static
  Remove unneeded 'aggressive inlining' attributes
  Add lock to protect against multi-threaded access
  Rename fields to use _lazy prefix
  Clean up and further optimizations in LegacySyntaxNodeExtensions
  ...
@ryzngard ryzngard deleted the exceptions/remoteinvocation branch October 15, 2024 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants