search

nipwaayoni / elastic-apm-php-agent

PHP Agent for Elastic APM
Other
28 stars 15 forks source link

Add TimerFactory class and use in Span #35

Closed dstepe closed 4 years ago

dstepe commented 4 years ago

Here is my alternative with a TimerFactory class. The testing, in my opinion, is much simpler and easier to follow.

While the TimerFactory class is barely more than a return new Timer() right now, it serves to make testing easier. Maybe that's all it will ever do, but I personally feel the benefit is worth the added class.

tsantos84 commented 4 years ago

Should we add also the factory class to Transaction?

dstepe commented 4 years ago

I added more tests for Span using the TimerFactory approach. I think you are correct that the we need to redo how timing is done from the ground up. I'm only looking for incremental improvements for this release, though. This PR now also fixes an issue in EventBean due to the use of the class creation time as the timestamp rather than the start time of the Timer object.

If you're good with this, we'll merge it to the event-timer-changes branch and then get that PR merged to the 7.1 dev branch.

dstepe commented 4 years ago

Regarding using TimerFactory in the Transaction class, yes, that's a good idea. The Transaction class also has the same timestamp issue found in Span. While looking into this, I also see some issues with the Timer behavior when a start time is provided vs assumptions about timers autostarting. I'll keep working on it.

tsantos84 commented 4 years ago

Regarding using TimerFactory in the Transaction class, yes, that's a good idea. The Transaction class also has the same timestamp issue found in Span. While looking into this, I also see some issues with the Timer behavior when a start time is provided vs assumptions about timers autostarting. I'll keep working on it.

Nice! I'll do my best to review. :)

dstepe commented 4 years ago

The change to Transaction is done to the point existing and new tests are passing. I'll put it through real-world tests tomorrow. This introduced some breaking changes, but I feel it's appropriate to make Transaction more consistent with Span. I added a note to the 7.2 Release project to implement a common TimedEvent solution for those. I hope the impact on users is minimal because the Agent should be used to create Transaction objects.

tsantos84 commented 4 years ago

Hi @dstepe , do you know Symfony Stopwatch component? It solves exactly the same problem we have (timing). As soon as they starts the clock, the return is an event (the same abstraction you've mentioned early). Beside that, they measure memory consumption in that period and could be useful to us in some point. If you think that coupling this library with Stopwatch is non desired, I propose at least read they code base to get insights. :)

dstepe commented 4 years ago

I am all for not trying to solve a problem that someone else has already solved. The StopWatch component looks really useful.

Do you have any other comments on this PR or are you ready to close your review for merging?