search

philkra / elastic-apm-laravel

Elastic APM Client for Laravel
94 stars 50 forks source link

33 address running tests and laravel start time #34

Closed dstepe closed 5 years ago

dstepe commented 5 years ago

This is a draft PR so we can discuss this proposed change.

I am concerned about the use of the LARAVEL_START constant. Not only does this cause problems in feature test scenarios, but that constant does not appear to be explicitly exposed by the framework in a sustainable way. Additional, this module reimplements timing and duration calculations for use in the APM transaction which could be provided with an existing Timer class.

This change does require an update to the elastic-apm-php-agent dependency.

The updated Timer class can accept an explicit start time. The PHP provided REQUEST_TIME_FLOAT appears to be the appropriate source for that start time. The popular Laravel DebugBar plugin has stopped using LARAVEL_START in favor of this approach.

The service provider will register a new Timer object and ensure it is correctly initialized. The service provider will also set a private start time member to the same value for use in query timing.

I believe this change is consistent with the Laravel framework conventions and consolidates APM related timing calculations.

philkra commented 5 years ago

@dstepe would you be interested to become a collabotrator for this repo?

dstepe commented 5 years ago

Yes, I'm open to that. We have a number of Laravel applications and are just getting started with APM, so I'm highly motivated right now. This module really helped me get started quickly and I'm happy to help work on it.

philkra commented 5 years ago

The baseline PR of the agent is merged.

dstepe commented 5 years ago

The composer file is cleaned up and ready.

dstepe commented 5 years ago

We've been running this in a Laravel app here in test for a few days and have not seen any problems. What's your normal process for testing and validation before merging and releasing?

philkra commented 5 years ago

Thanks for the contribution @dstepe and in case you are doing another PR for this repo, please add yourself to the contributors in the composer.json file.