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

Report Handled Exceptions To New Relic #11944

Merged

Conversation

mpchadwick
Copy link
Contributor

Description

Currently, if you're using New Relic you'll know about fatal errors via the APM instrumentation, but handled exceptions will not be reported. This change reports handled exceptions to New Relic

Manual testing scenarios

  1. Configure the New Relic PHP agent in your environment and enable the New Relic module
  2. Purposely enter the wrong database credentials in app/etc/env.php.
  3. Notice the exception being reported to New Relic

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@toddbc
Copy link
Contributor

toddbc commented Nov 2, 2017

This is a really important change to enable error metrics, alerts, and even correlation within New Relic.

Really looking forward to this support within Magento 2.

@@ -153,6 +153,8 @@ public function launch()
*/
public function catchException(Bootstrap $bootstrap, \Exception $exception)
{
$this->_eventManager->dispatch('application_handled_exception', ['exception' => $exception]);
Copy link
Contributor

Choose a reason for hiding this comment

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

New events must not be created, you can implement this logic as a before plugin for catchException method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@orlangur i refactored it to use a plugin.

$this->newRelicWrapper = $newRelicWrapper;
}

public function beforeCatchException(Http $subject, $bootstrap, $exception)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add types for $bootstrap and $exception? Also phpdoc block is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ihor-sviziev this is done.

/**
* @var NewRelicWrapper
*/
protected $newRelicWrapper;
Copy link
Contributor

Choose a reason for hiding this comment

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

In general looks good, btw new properties should be private instead of protected. Sorry for missing if prev. time

Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering - even new properties to new classes? I understand the BC concerns for existing classes (2.2.0 lacking something that 2.1.11 has, etc.), but does it help for new classes too in some way?

Copy link
Contributor

Choose a reason for hiding this comment

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

According to technical guidelines: http://devdocs.magento.com/guides/v2.1/coding-standards/technical-guidelines/technical-guidelines.html

2.7. All non-public properties and methods SHOULD be private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ihor-sviziev this is done

@vrann vrann self-assigned this Nov 9, 2017
@vrann vrann added this to the November 2017 milestone Nov 9, 2017
@okorshenko okorshenko merged commit 6bc4fcf into magento:2.2-develop Nov 10, 2017
@iainhubbard
Copy link
Contributor

Not done my background research on this but does lib/internal/Magento/Framework/App/Http.php encompass all the entry points into magento e.g. cron, cli, XML API etc. We have always done this change in pub/errors/report.php. Still a core change obviously. e.g.

    $processor->saveReport($reportData);
    if (function_exists('newrelic_notice_error')) {
        if (isset($reportData[1])) {
            if (!isset($e) || $e->getTraceAsString() !== $reportData[1]) {
                newrelic_notice_error($reportData[0] . "\n" . $reportData[1]);
            } else {
                newrelic_notice_error($e->getMessage(), $e);
            }
        } elseif (isset($e)) {
            newrelic_notice_error($e->getMessage(), $e);
        }
    }

Not tested etc etc

This has the benefit of not relying on magento for anything especially in the event of a catastrophic failure.

@ihor-sviziev
Copy link
Contributor

@iainhubbard if you have any issues with this implementation and know how to improve this module - that's really good! waiting for a PR from you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants