-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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 Cron Jobs names to New Relic transactions #25957
Add Cron Jobs names to New Relic transactions #25957
Conversation
Hi @lbajsarowicz. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
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.
Hi @lbajsarowicz,
In general your changes looks good, but would be really good to have some tests for this feature.
Could you cover your changes with some tests?
That will, actually, take some time. |
*/ | ||
private function isCommandSkipped(Command $command): bool | ||
{ | ||
return in_array($command->getName(), $this->skipCommands); |
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.
Looks like this code will be executed quite often. In array actually is quite slow, could you replace it to finding in map?
Related magento/magento-coding-standard#143
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 part is run only for bin/magento commands execution, however - I changed it to avoid in_array
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.
@lbajsarowicz after all your changes - could you squash all changes to single commit, because 18 commits for this small feature is really too much.
app/code/Magento/NewRelicReporting/Test/Unit/Model/NewRelicWrapperTest.php
Outdated
Show resolved
Hide resolved
42cdc0a
to
5819ded
Compare
Hi @ihor-sviziev, thank you for the review. |
✔️ QA passed |
Hi @lbajsarowicz, thank you for your contribution! |
Description (*)
Feature: Add missing transaction names for Cron Jobs in New Relic reports
I've modified private methods, as this is not backward incompatible.
Fixed Issues (if relevant)
Manual testing scenarios (*)
bin/magento cron:run
Contribution checklist (*)