Don't suppress view component rendering errors #2410
Conversation
mjacobus
requested review from
fallwith,
hannahramadan,
kaylareopelle and
tannalynn
as code owners
|
|
github-actions
bot
added
the
community
mjacobus
changed the title
mjacobus
force-pushed
the
fix-view-component-instrumentation
branch
from
a0e8887
to
3b93d47
Compare
| @@ -14,8 +14,6 @@ def render_in_with_tracing(*args) | |||
| name: metric_name(self.class.identifier, self.class.name) | |||
| ) | |||
| yield | |||
| rescue => e | |||
| ::NewRelic::Agent.logger.debug('Error capturing ViewComponent segment', e) | |||
There was a problem hiding this comment.
Hi @mjacobus! Thank you for catching this—we shoudn't be swallowing errors! But we do want to capture and report them to New Relic. Could you update your code to the following?
rescue => e
NewRelic::Agent.notice_error(e)
raiseThen we can get this merged ◡̈
There was a problem hiding this comment.
Sure thing! There you go. Thank you @hannahramadan
We should be able to se error pages, specially in when Rails.env is development. The piece of code that was removed here prevented from happening. Instead, it would have the `render_in` method return `nil` when an error happened, resulting in either a blank page, or an incomplete page. I believe we also want to raise errors in production. We should not make a decision here, whether the error should be suppressed. Otherwise we are making the assumption "incomplete pages are better than error pages", which may be the correct assumption under some circumstances, but not under other circumstances. Fixes ViewComponent/view_component#1981
mjacobus
force-pushed
the
fix-view-component-instrumentation
branch
from
3b93d47
to
becdad9
Compare
|
Thank you @mjacobus! We will followup once these changes are released. |
|
Hi @mjacobus—Ruby agent 9.7.1 has been released and includes this fix. Thanks again for your help on this! |
Before contributing, please read our contributing guidelines and code of conduct.
Overview
Fixes a bug introduced in #2367, where the render_in behavior was changed, suppressing errors.
We should be able to se error pages, specially in when Rails.env is development. The piece of code that was removed here prevented from happening. Instead, it would have the
render_inmethod returnnilwhen an error happened, resulting in either a blank page, or an incomplete page.I believe we also want to raise errors in production. We should not make a decision here, whether the error should be suppressed. Otherwise we are making the assumption "incomplete pages are better than error pages", which may be the correct assumption under some circumstances, but not under other circumstances.
Fixes ViewComponent/view_component#1981
Submitter Checklist:
Testing
The agent includes a suite of unit and functional tests which should be used to
verify your changes don't break existing functionality. These tests will run with
GitHub Actions when a pull request is made. More details on running the tests locally can be found
here for our unit tests,
and here for our functional tests.
For most contributions it is strongly recommended to add additional tests which
exercise your changes.
Reviewer Checklist