search

nipwaayoni / elastic-apm-php-agent

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

Not commit transactions on __destruct #6

Closed tsantos84 closed 4 years ago

tsantos84 commented 4 years ago

As I've commented previously, commit transactions on agent's destruct method can ommit network bugs. I think it's better to always commit the transactions manually. WDYT?

dstepe commented 4 years ago

I've read your other thread and agree we should look at that behavior. While the destructor is a convenient place to implement the commit behavior, I don't believe it's consistent with the intended use of a destructor method. Destructors should be used to clean up the resources used by the object being destructed and should not cause side-effects, especially considering their limits (called even on exit, don't allow throwing exceptions).

I don't believe I would support making the behavior configurable. That does not solve the actual problem of side-effects from the destructor. I'm not sure what alternative to suggest at this point, though. What are your thoughts for this?

tsantos84 commented 4 years ago

I think we can send it manually through Agent::send method whenever we want to. Totally agree with you about destruct behavior.

dstepe commented 4 years ago

I see that the send on destruct behavior is listed in the planned breaking changes for 7.x. Since 7.0 never made it out of RC, now is the time to implement this. I plan to skip 7.0 and make 7.1 the first release under the new org. Will you make a PR for this, including changes to documentation and tests if needed?

tsantos84 commented 4 years ago

I can do that. In terms of coding, the only change would be remove __destruct method and keep using the send method, right?

dstepe commented 4 years ago

Yes, I believe that's the only required coding change.

dstepe commented 4 years ago

I'm about to merge a PR that will apply the PSR-12 code style (or as close to it as i can easily get with php-cs-fixer right now). Not only is the current code updated, but there is now a workflow job to check and enforce the style. You will want to update your master branch before starting any changes.

dstepe commented 4 years ago

I just tagged 7.1.0-rc1. I'm making time to try get a first stable release out so we have something to build on. If you don't have a chance to get to the change described in this issue in the next couple of days, I'll work on it. I'd like to make sure this closed before a final release.

tsantos84 commented 4 years ago

Hi @dstepe, sorry for the long days without reply you. As you can see, the PR is open and waiting for your review. I didn't found any place that makes mention to __destruct method, so the only change needed was remove the method from the agent.

dstepe commented 4 years ago

The "breaking changes" document mentions the destruct behavior, but destruct is misspelled so your search would not have found it. :)

I merged this with a note that we need to review that doc anyway.

Thanks!