Laravel Instrumentation

[ChrisLightfootWild]

Hello 👋

I'm opening this as a draft, while I test it locally. This PR moves watcher registration from the Http Kernel into the Application contract.

This should allows for watcher registration in console Kernel contexts, such as commands that are configured to run in the Kernel scheduler.

I have tested this locally in Laravel 10 only so far.

[welcome]

Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: ChrisLightfootWild / name: Chris Lightfoot-Wild (347a001, 1221735, 24ae2ea, 3b24774, cb5eea9, 4cc513c, 7c02e60, f49d613, da40eaf, e376229, 792a80d, b52e707, 1eadc5c, f2b5e31, 4ed45f8, 632df00, d3e55ba, 973f695, a24857e, bc4c216, 2c18066, 22dd5b0, a8138ad)

[pdelewski]

Hello 👋

I'm opening this as a draft, while I test it locally. This PR moves watcher registration from the Http Kernel into the Application contract.

This should allows for watcher registration in console Kernel contexts, such as commands that are configured to run in the Kernel scheduler.

I have tested this locally in Laravel 10 only so far.

Are all tests passing?

[ChrisLightfootWild]

Each Watcher is already typed against \Illuminate\Contracts\Foundation\Application, so this maintains that type for consistency.

[ChrisLightfootWild]

Are all tests passing?

The existing ones do. I'm looking to write some which target the console but just need to figure out what that instrumentation should look like.

[codecov]

Codecov Report

Merging #184 (4f0ddcc) into main (c8e79a8) will increase coverage by 6.20%.
Report is 4 commits behind head on main.
The diff coverage is 8.51%.

❗ Current head 4f0ddcc differs from pull request most recent head a8138ad. Consider uploading reports for the commit a8138ad to get more accurate results

@@             Coverage Diff              @@
##               main     #184      +/-   ##
============================================
+ Coverage     28.60%   34.81%   +6.20%     
- Complexity      472      811     +339     
============================================
  Files            47       72      +25     
  Lines          1755     3091    +1336     
============================================
+ Hits            502     1076     +574     
- Misses         1253     2015     +762     

Flag	Coverage Δ
7.4	45.78% <92.30%> (-6.51%)	⬇️
8.0	27.15% <8.51%> (-0.71%)	⬇️
8.1	30.55% <8.51%> (+2.62%)	⬆️
8.2	34.75% <8.51%> (+6.13%)	⬆️

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

Files Changed	Coverage
...rumentation/Laravel/src/ConsoleInstrumentation.php	0.00%
...nstrumentation/Laravel/src/HttpInstrumentation.php	0.00%
...rumentation/Laravel/src/LaravelInstrumentation.php	0.00%
...trumentation/Laravel/src/Watchers/CacheWatcher.php	ø
...tion/Laravel/src/Watchers/ClientRequestWatcher.php	ø
...entation/Laravel/src/Watchers/ExceptionWatcher.php	ø
...nstrumentation/Laravel/src/Watchers/LogWatcher.php	ø
...trumentation/Laravel/src/Watchers/QueryWatcher.php	ø
src/ResourceDetectors/Container/src/Container.php	92.30%

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8e79a8...a8138ad. Read the comment docs.

[brettmc]

This looks pretty good to me. There's a linting error to fix to make the build green. I also think it's worth waiting on #184 to be merged, and then ensuring your changes work back to Laravel 6.x

[ChrisLightfootWild]

This looks pretty good to me. There's a linting error to fix to make the build green. I also think it's worth waiting on #184 to be merged, and then ensuring your changes work back to Laravel 6.x

Thanks @brettmc. I think I've got the linting one sorted now.

This is #184, did you mean this should go in after another that's in queue? 😄

Is there a reference/documented test strategy for this? Or do you tend to just go and manually create Laravel projects and test instrumentation that way? Happy to investigate this either way, all part of familiarising myself with the ecosystem.

Thanks!

[brettmc]

Sorry, #185 (which has just been merged) is for Laravel too. Can you incorporate those changes into your branch. It looks like we can support back to Laravel v6.x

Is there a reference/documented test strategy for this?

Just the existing integration tests, but verifying against a real Laravel app is also encouraged. If you do come across issues, please try to replicate them in a test. I'm very open to any improvements you can imagine :)

[ChrisLightfootWild]

Sorry, #185 (which has just been merged) is for Laravel too. Can you incorporate those changes into your branch. It looks like we can support back to Laravel v6.x

Is there a reference/documented test strategy for this?

Just the existing integration tests, but verifying against a real Laravel app is also encouraged. If you do come across issues, please try to replicate them in a test. I'm very open to any improvements you can imagine :)

Thanks, I've merged main into this branch to reflect those changes. I think I fixed the import error on the last build too; IDE and php-cs-fixer in slight disagreement over what sorting the imports should look like!

[ChrisLightfootWild]

I'm keeping this in draft still as I'm not happy with how it instruments the commands currently.

Will continue testing this through the coming evenings to better refine it.

[ChrisLightfootWild]

I'm about to push my latest changes (which currently have a failing test); I am still trying to get the artisan handle to be recorded as the first span in the trace in the tests but falling over with it currently.

--

I've used the following test route to produce a trace in Zipkin:

Route::get('/test', function () {
    \Illuminate\Support\Facades\Artisan::call('optimize');
    \Illuminate\Support\Facades\Artisan::call('optimize:clear');
});

[ChrisLightfootWild]

@brettmc thanks for taking the time to debug this. Apologies, I didn't realise how the spans were recorded - so that massively helped in my understanding. Fixed in a8138ad.