-
Notifications
You must be signed in to change notification settings - Fork 79
SLVS-2764 Start collecting request level exceptions #6578
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
SLVS-2764 Start collecting request level exceptions #6578
Conversation
6082677 to
27be1b5
Compare
56a0b3a to
28c247f
Compare
src/SLCore/ISLCoreInstanceHandler.cs
Outdated
| { | ||
| logger.WriteLine(SLCoreStrings.SLCoreHandler_StartingInstance); | ||
| await currentInstanceHandle.InitializeAsync(); | ||
| monitoringService.Init(); |
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.
This will re-initialize on every SLCore restart (if it crashed, for example). Do we still want that or do we only want to initialize once?
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.
Telemetry is re-initialized every time SLCORE crashes, I think it it a good practice the keep the same behavior also we rely on telemetry to decide if we initialize monitoring
src/SLCore/Core/IJsonRpc.cs
Outdated
|
|
||
| protected override JsonRpcError.ErrorDetail CreateErrorDetails(JsonRpcRequest request, Exception exception) | ||
| { | ||
| monitoringService.ReportException(exception, $"JsonRpcWrapper.CreateErrorDetails:{request.Method ?? "unknown"}"); |
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 need to check if there's anything useful in those exceptions. Unfortunately, from the top of my head, it always says "Internal error" and not much 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.
This is one example: https://sonar-x0.sentry.io/issues/7167581220/?project=4510622904877056&query=is%3Aunresolved&referrer=issue-stream
We at least get the request causing the exception. We discussed and did the same capturing for IntelliJ, I think it's a good start.
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'm not familiar with the .NET implementation, but there is normally a way to get the SLCORE stack trace that led to the 'internal error'. There is probably a certain type of exception, in which you can access those details, by running in debug mode you might be able to find that.
The 2nd thing is that reporting all exceptions from this method might be too much. I think we could try to focus only on exceptions that are uncaught by SLCORE and translate to 'internal errors'.
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.
They have InternalError = -32603, defined, I guess we could only report these as a start then
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.
Also added error data information
28c247f to
89a67a7
Compare
89a67a7 to
65b9d6c
Compare
georgii-borovinskikh-sonarsource
left a comment
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.
A few more comments + please test this on an old VS on the VM that we have
868e45c to
c48e777
Compare
georgii-borovinskikh-sonarsource
left a comment
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.
Some things to fix with synchronization, otherwise looks good. Please only merge once those are fixed
| { | ||
| try | ||
| { | ||
| sentrySdk.Init(options => |
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.
Did you find a way to turn off automatic collection of uncaught exceptions?
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.
We have a separate ticket for this
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 will start working on it after I merge this to feature branch. We decided to merge to feature since that ticket is implemented to avoid noise
| { | ||
| try | ||
| { | ||
| sentrySdk.Init(options => |
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.
We will probably need a way to uniquely identify users on the VS side too
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.
As discussed we can start implementing this after the unhandled exception task is finished
66b1200 to
1b9cd37
Compare
|
|
||
| private static void TryAddCommonErrorData(Sentry.Scope scope, Exception exception) | ||
| { | ||
| if (exception is not LocalRpcException localRpcException) |
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.
Why only local exceptions? Do we rely on SLCore to capture other exceptions?
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.
Yes we also have a exception catching mechanism on SLCORE, currently only for intellij, but I will also include other IDEs as well
SonarQube reviewer guideImportant We are currently testing different models for AI Summary. Model A:Summary: Adds Sentry SDK integration and monitoring service for error reporting with telemetry opt-in/out support. Review Focus: The monitoring service initialization and error reporting flow, particularly the integration with JsonRpcWrapper's CreateErrorDetails method which reports internal/invalid parameter errors to Sentry. Verify exception handling doesn't interfere with normal operation and that telemetry status correctly controls Sentry activation. Start review at: Model B:Summary: Add Sentry error monitoring integration with telemetry-aware exception reporting to enable crash tracking based on user telemetry preferences. Review Focus:
Start review at:
|
|
|
||
| if (errorDetail.Code == JsonRpcErrorCode.InternalError) | ||
| if (errorDetail.Code == JsonRpcErrorCode.InternalError || | ||
| errorDetail.Code == JsonRpcErrorCode.InvalidParams) |
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.
well, this one was just an example, there are other types. there are also other exception types such as https://learn.microsoft.com/en-us/dotnet/api/streamjsonrpc.remoterpcexception?view=streamjsonrpc-2.21
It's a bit unclear to me why we only choose some of them and not all
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.
We would like to limit the exceptions to ones that we are interested to look first. InternalError was one thing we wanted to tackle first. It doesn't mean we will not look at the others, when we spend enough time and tackle these we might actually disable internal errors and tackle other exceptions. The purpose is to have focus during such investigations
03158cf
into
feature/ef/SLVS-enable-sentry



No description provided.