Skip to content

Conversation

hahuja2
Copy link
Contributor

@hahuja2 hahuja2 commented Sep 23, 2025

This PR does the following:

  • Adds instrumentation that ensures the transaction is properly stopped for laravel horizon.
  • Adds integration tests verifying the transaction is properly stopped.

@newrelic-php-agent-bot
Copy link

newrelic-php-agent-bot commented Sep 23, 2025

Test Suite Status Result
Multiverse 16/16 passing
SOAK 84/85 passing

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.17%. Comparing base (09b8a53) to head (819903e).

Files with missing lines Patch % Lines
agent/fw_laravel.c 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1123      +/-   ##
==========================================
+ Coverage   78.14%   78.17%   +0.02%     
==========================================
  Files         193      193              
  Lines       28053    28058       +5     
==========================================
+ Hits        21923    21934      +11     
+ Misses       6130     6124       -6     
Flag Coverage Δ
agent-for-php-7.2 78.40% <0.00%> (+<0.01%) ⬆️
agent-for-php-7.3 78.42% <0.00%> (?)
agent-for-php-7.4 78.28% <0.00%> (+<0.01%) ⬆️
agent-for-php-8.0 77.51% <100.00%> (+0.02%) ⬆️
agent-for-php-8.1 77.82% <100.00%> (+0.02%) ⬆️
agent-for-php-8.2 77.43% <100.00%> (+0.02%) ⬆️
agent-for-php-8.3 77.43% <100.00%> (+0.02%) ⬆️
agent-for-php-8.4 77.45% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hahuja2 hahuja2 marked this pull request as ready for review September 24, 2025 19:46
@hahuja2 hahuja2 requested review from mfulb and lavarou September 24, 2025 19:53
* used by wrapping the handle method of the HorizonCommand and
* SupervisorCommand class.
*/
NR_PHP_WRAPPER(nr_laravel_horizon_end) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer a name like nr_laravel_horizon_end_txn to be more clear what this function does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, updated the name: f049a15


$txn = get_txn();
foreach ($txn->getTrace()->findSegmentsByName('Custom/foobar') as $segment) {
echo $segment->name;
Copy link
Contributor

Choose a reason for hiding this comment

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

This output should be captured in an EXPECT statement of some sort as well as any other echo's - this is true for all tests in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added: c1712c5

bduranleau-nr
bduranleau-nr previously approved these changes Oct 1, 2025
mfulb
mfulb previously approved these changes Oct 1, 2025
@hahuja2 hahuja2 dismissed stale reviews from mfulb and bduranleau-nr via d8e9adb October 3, 2025 16:53

nr_php_txn_end(1, 0 TSRMLS_CC);

nrl_verbosedebug(NRL_TXN, "Ending Laravel Horizon Transaction");
Copy link
Member

Choose a reason for hiding this comment

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

The code is not very consistent about using logging subsystems, but most log messages in fw_*.c modules use NRL_FRAMEWORK logging subsystem. Since this code runs in the context of framework's instrumentation, I'd suggest using either NRL_FRAMEWORK or NRL_INSTRUMENT subsystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, changed: 819903e

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.

6 participants