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
Add Ability To Separate Frontend / Adminhtml in New Relic #12935
Add Ability To Separate Frontend / Adminhtml in New Relic #12935
Conversation
Just to document, based on testing done in response to this conversation on Twitter, this will also catch other area codes such as webapi_rest. |
I reopened this PR in order to restart build in travis |
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.
In general looks good, but here is some small issues. Can you also cover your changes with tests?
* @param string $appName | ||
* @return void | ||
*/ | ||
public function setAppName($appName) |
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'd like to define variable type to "string" there, we're not supporting php 5.6 anymore
try { | ||
$this->newRelicWrapper->setAppName($this->appName($state)); | ||
} catch (LocalizedException $e) { | ||
return; |
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 should we ignore such exception?
- Probably we should have
return $result
here
public function afterSetAreaCode(State $state, $result) | ||
{ | ||
if (!$this->shouldSetAppName()) { | ||
return; |
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.
Probably we should have return $result
here
} | ||
} | ||
|
||
private function appName(State $state) |
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.
Would be better to rename it to "setAppName"
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 a getter. Would you prefer getAppName
?
This would happen if for some reason the area code wasn't set
Hi @mpchadwick, |
@ihor-sviziev I will add tests. May take a little while. Still trying to get the integration tests to run in my local environment. |
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.
@mpchadwick please review static test, they are failing currently
@ihor-sviziev fixed |
Hi @ihor-sviziev, thank you for the review. |
Hi @mpchadwick. Thank you for your contribution. Please, consider to port this solution to 2.3 release line. |
Hi @mpchadwick, |
Description
Adds a new setting which, when enabled, reports frontend and adminhtml as separate apps to New Relic.
frontend and adminhtml are in addition to the "main" app which includes both (for backwards compatibility). The user needs to set "New Relic Application Name" to use this feature as the area is appended to the appname setting, separated by and underscore.
Useful as slow transactions in admin activity can cause false positives New Relic alerts / skew average response time metrics. Using this feature allows monitoring and alerting on only frontend traffic by configuring alerts for just the frontend New Relic app.
Manual testing scenarios
Contribution checklist