arkaitzgarro / elastic-apm-laravel

Laravel APM agent for Elastic v2 intake API
MIT License
79 stars 17 forks source link

fix: always initialize an Agent and a Transaction #125

Closed arkaitzgarro closed 4 years ago

arkaitzgarro commented 4 years ago

We faced a situation when using facades, but having the package disabled with APM_ACTIVE=false, was making the application fail. The problem is that we don't initialize an Agent nor start a transaction when the package is not enabled, but, the ApmAgent facade expects those to exist, as it exposes functionality related to the Agent and the current transaction.

I made a change to always register the Agent and the middleware but not the collectors if the package is not enabled. This means that an instance of the Agent will always exist, a transaction will be created but no spans will be collected or send to APM.

Edit: there is still a small issue though. We don't create a transaction if it matches the APM_IGNORE_PATTERNS pattern, and ApmAgent::getCurrentTransaction() will throw an exception as well. I think we should always create a transaction, but don't collect event around it nor send it to APM if it needs to be ignored. @dstepe maybe something the underlying package should take care of?

arkaitzgarro commented 4 years ago

registerAgent also add a a terminate handler that tries to send data from the agent. Does it make sense if collectors are not registered? Maybe we can move that bit elsewhere to not execute code that we know won't do anything.

I did nothing because the underlying Agent will not send the events, and I wanted to avoid extra if statements everywhere. Also, the agent will clean up the store, it looks like it's a good idea to actually call send :)

countless-integers commented 4 years ago

mkay, then send does more than just send 😉

dstepe commented 4 years ago

then send does more than just send

Unfortunately, yes. That's a remnant of the philkra agent. It will take a major release with breaking changes to sort that out.

countless-integers commented 4 years ago

then send does more than just send

Unfortunately, yes. That's a remnant of the philkra agent. It will take a major release with breaking changes to sort that out.

I'm sure well get there eventually 👍