Added exception for security update error handling. #9977
Conversation
| @@ -12,6 +12,17 @@ module Operations | |||
| class CreateSecurityUpdatePullRequest | |||
| include SecurityUpdateHelpers | |||
|
|
|||
| class SecurityUpdateError < StandardError | |||
There was a problem hiding this comment.
should we call this TransitiveSecurityUpdateError for specificity?
There was a problem hiding this comment.
i matched with class name convention, specificity would be a good idea for sure. ill wait for someone to review once to be sure in case im missing anything.
| def log_error_for_security_update_failure(dependency:) | ||
| raise SecurityUpdateError.new( | ||
| message: "Security Update Error, No files were updated!", | ||
| error_context: dependency |
There was a problem hiding this comment.
I'm not sure I understand what dependency provides in terms of context. Could you share what we'd be sending to an error tracking service and how it would help us and/or users?
There was a problem hiding this comment.
@landongrindheim , i have changed error context to back to thrown exception object. it should maintain the older context information but with a newer exception class.
| rescue StandardError | ||
| log_error_for_security_update_failure(dependency: dependency) | ||
| end | ||
|
|
||
| def log_error_for_security_update_failure(dependency:) | ||
| raise SecurityUpdateError.new( | ||
| message: "Security Update Error, No files were updated!", |
There was a problem hiding this comment.
🤔 Rescuing StandardError is a pretty broad approach since it will catch all subclasses of StandardError. That would mean that almost any error that is encountered in #check_and_create_pr_with_error_handling will be logged as SecurityUpdateError, "Security Update Error, No files were updated!".
I think it would be wise to preserve some context. Maybe that happens in the message (i.e. we include the original error's class and/or message), or maybe that's passed along in the error_context.
There was a problem hiding this comment.
@landongrindheim , i have redone the code , earlier issue was that due to a double exception throw (StandardError and then SecurityUpdateError), there was an issue related with double log of sentry issue. so i didn't opt for a double exception.
I have raised a new exception class (SecurityUpdateError) and maintained the context info from thrown exception that should maintain the error_context from older exception. I have also setup the exception backtrack from SecurityUpdateError to thrown exception which should maintain the stack trace. It should start logging security updates error under new category.
There was a problem hiding this comment.
I think what Landon was saying is still the case, any error that raises here will now be raised as a SecurityUpdateError instead of its native error class?
|
What exactly is the problem that we're trying to solve here? |
The problem is to separate security update failures due to transitive dependencies (because DG doesn't currently sent that information) into its own error bucket so we can actually track its impact. The current bucket is a catch-all and is masking several errors. |
|
Thanks for clarifying @abdulapopoola I think the current implementation would make this even worse, we'd now get a single catch all masking any error that happens here. |
|
@sachin-sandhu let's start off with adding some tests around this that show that only the right errors are going into this new bucket, and that unrelated errors are not going into it |
| @@ -69,8 +80,11 @@ def check_and_create_pr_with_error_handling(dependency) | |||
| error_type: "inconsistent_registry_response", | |||
There was a problem hiding this comment.
I think the error is surfaced here.
can we detect the actual transitive failure mode and add a specific error class for that scenario? That way we are not swallowing up all errors.
I would expect something like this:
raise Dependabot::SecurityUpdateFailureDueToTransitiveDependency.new(...
...
That way, we can ensure we are actually separating out that error category.
There was a problem hiding this comment.
@abdulapopoola , im working on this
|
@abdulapopoola , I did some more debugging on core issue.
This might happen when you were using dependencies versions which when updated via security update will break your solution. To test this ,I ran NPM standalone on repos causing the security update issue. I cleared the particular dependency content from package-lock.json so that NPM CLI can recreate the dependency tree with non breaking change. Similarly I removed similar content from repo's package-lock.json dsp-testing/6133_3@6f11e1c , Then I think for now we can just separate out security update issue in a separate exception category and close the ticket. |
already has support for transitive dependencies updates for NPM and YARN. So it should be able to handle the updates well in both security updates and regular updates. However there are several reasons why this might fail.
What are you trying to accomplish?
Related with Sentry issue 👇
When there is Security update related exception in NPM and YARN ecosystem, the error response is generic ("No files were updated!") which is not very useful in Sentry analysis. The fix creates a newer exception class to better isolate the issues related with Security Update failures. All security update generic issues will be logged under a new category SecurityUpdateError which would be useful in metric and analysis until a permanent fix is found.
Anything you want to highlight for special attention from reviewers?
How will you know you've accomplished your goal?
All NPM and YARN security update issues will be logged under new exception "Security Update Error, No files were updated!" in Sentry
Checklist