Skip to content
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

Consolidating ErrorTracker methods into single method call (SCP-3378) #1046

Merged
merged 4 commits into from
Jun 3, 2021

Conversation

bistline
Copy link
Contributor

This update consolidates the usage of ErrorTracker for reporting errors to Sentry into a single method call of report_exception_with_context, removing the need to call format_extra_context before reporting each exception. The new merged method will take any number of "context objects", being instances of models, request parameters, or any other object. This makes using the module easier and still reports necessary context along with errors.

TO TEST:

As we do not report exceptions in development/test mode, the simplest way to test this is by invoking the method directly:

  1. Enter the rails console
  2. Load an assortment of objects for reporting:
user = User.first
study = Study.first
file = StudyFile.first
extra = {foo: 'bar'}
  1. Create an exception:
error = RuntimeError.new('this is the error')
  1. Using the data from above, report the exception:
ErrorTracker.report_exception_with_context(error, user, study, file, extra)
  1. Note the following message in development.log:
Suppressing error reporting to Sentry: RuntimeError:this is the error, context: {"study" => {...}, "study_file" => {...}, :foo => "bar"}

This PR satisfies SCP-3378.

@bistline bistline requested review from eweitz and devonbush May 24, 2021 18:21
@codecov
Copy link

codecov bot commented May 24, 2021

Codecov Report

Merging #1046 (8737978) into development (db15f39) will increase coverage by 0.23%.
The diff coverage is 10.63%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1046      +/-   ##
===============================================
+ Coverage        64.85%   65.09%   +0.23%     
===============================================
  Files              181      181              
  Lines            16505    16425      -80     
  Branches           601      601              
===============================================
- Hits             10705    10692      -13     
+ Misses            5608     5541      -67     
  Partials           192      192              
Impacted Files Coverage Δ
app/controllers/admin_configurations_controller.rb 33.66% <0.00%> (ø)
.../controllers/analysis_configurations_controller.rb 44.21% <0.00%> (+1.35%) ⬆️
app/controllers/api/v1/api_base_controller.rb 82.75% <0.00%> (ø)
...ontrollers/api/v1/directory_listings_controller.rb 96.61% <0.00%> (ø)
...ontrollers/api/v1/external_resources_controller.rb 96.53% <0.00%> (+0.47%) ⬆️
app/controllers/api/v1/site_controller.rb 77.19% <0.00%> (+0.75%) ⬆️
app/controllers/api/v1/studies_controller.rb 86.37% <0.00%> (+0.44%) ⬆️
...ontrollers/api/v1/study_file_bundles_controller.rb 95.80% <0.00%> (+0.57%) ⬆️
app/controllers/api/v1/study_files_controller.rb 92.44% <0.00%> (+0.24%) ⬆️
app/controllers/api/v1/study_shares_controller.rb 96.24% <0.00%> (+0.44%) ⬆️
... and 28 more

@bistline
Copy link
Contributor Author

There appears to be an intermittent issue with precompiling static assets in sprockets 4 - on rare occasions a segfault is thrown. This is apparently a known issue: rails/sprockets#633.

We should keep an eye on this as it could happen in any of our deployed environments.

Copy link
Contributor

@devonbush devonbush left a comment

Choose a reason for hiding this comment

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

this is great! Couple nits -- I'm not sure I understand why in some cases you kept .to_unsafe_hash, and in others you removed it. And then just a method naming preference that could go either way

app/controllers/api/v1/site_controller.rb Outdated Show resolved Hide resolved
lib/error_tracker.rb Outdated Show resolved Hide resolved
@bistline bistline requested a review from devonbush May 24, 2021 20:37
Copy link
Member

@eweitz eweitz left a comment

Choose a reason for hiding this comment

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

Looks good. Following up on our recent discussion about straggling unnecessary#{Time.zone.now}: prefixes in error messages, below I've removed the 43 cases here and tweaked remaining message content accordingly.

That feedback can be trivially applied via "Add suggestion to batch" and "Commit suggestions".

app/controllers/admin_configurations_controller.rb Outdated Show resolved Hide resolved
app/controllers/admin_configurations_controller.rb Outdated Show resolved Hide resolved
app/controllers/admin_configurations_controller.rb Outdated Show resolved Hide resolved
app/controllers/admin_configurations_controller.rb Outdated Show resolved Hide resolved
app/controllers/admin_configurations_controller.rb Outdated Show resolved Hide resolved
app/models/analysis_metadatum.rb Outdated Show resolved Hide resolved
app/models/study.rb Outdated Show resolved Hide resolved
app/models/study.rb Outdated Show resolved Hide resolved
app/models/study.rb Outdated Show resolved Hide resolved
app/models/study_share.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@devonbush devonbush left a comment

Choose a reason for hiding this comment

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

Looks good! I wouldn't be averse to yet another round of consolidation where the ErrorTracker also handles writing the log message. But that's a very-low-importance task for another day.

@bistline bistline merged commit c709d94 into development Jun 3, 2021
@bistline bistline deleted the jb-error-tracker-refactor branch June 3, 2021 13:55
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.

3 participants