philkra / elastic-apm-laravel

Elastic APM Client for Laravel
94 stars 50 forks source link

Add custom spans #41

Closed dstepe closed 5 years ago

dstepe commented 5 years ago

Closes #39 Adds custom spans for Laravel transactions (per https://github.com/philkra/elastic-apm-php-agent/issues/53)

@philkra What do you think about the customs span handling? It's pretty basic, but I think in the long run, providing Transaction and Span classes in the Elastic APM Agent project will be the way to go. Then these classes can be proxies so the functionality exposed to Laravel won't change.

philkra commented 5 years ago

@dstepe , I got a notice from packagist that there is a typo in the composer.json

dstepe commented 5 years ago

@philkra Packagist must have pulled my feature branch before I cleaned it up. There was a short period in which I did have a change and typo on the composer.json, but I opted to go a different route and reverted it. I squashed and cleaned up the commits on this branch, so you won't see that in the history.

If this approach looks good to you, I'll remove the draft status for a full review.

dstepe commented 5 years ago

@philkra Any thoughts on this? Do support this as a feature?

philkra commented 5 years ago

adding spans is already possible via the agent, https://github.com/philkra/elastic-apm-php-agent#adding-spans So I guess this is a duplicate way to add spans or am I on the wrong track?

Besides that, 👍 for the config query backtrace and rendersource config!

dstepe commented 5 years ago

Technically, you can only set the spans on the transaction since each call replaces any existing spans:

https://github.com/philkra/elastic-apm-php-agent/blob/master/src/Events/Transaction.php#L128

It would be possible to do something like this:

$transaction->setSpans(array_merge($transaction->getSpans(), $newSpans));

That seems to expose too much internal operation though.

Also, the Laravel request transaction is started and stopped in the middleware:

https://github.com/philkra/elastic-apm-php-agent/blob/master/src/Events/Transaction.php#L128

The Agent sends the transaction when the request terminates:

https://github.com/philkra/elastic-apm-laravel/blob/add-custom-spans/src/Middleware/RecordTransaction.php#L82

I don't see how I could easily access the transaction to add to it outside of the middleware. The Agent would know the transaction by name, but again, more internal details would need to be exposed in order to enable that.

My hope with this enhancement was to provide an easily access way to add spans without exposing internal implementation details. It also simply adds to the existing span collection which results in the minimum necessary change.

dstepe commented 5 years ago

@philkra Have you thought about this any more since my last response? I still see value providing the span feature through abstracted classes rather than directly using the underlying library code.

philkra commented 5 years ago

Sorry @dstepe for the delay. The pr looks good! What confuses me slightly is why this improved custom spans is not in the apm package?

dstepe commented 5 years ago

@philkra I consider this change a first step which enables custom span handling specifically in this package. I've tried to design this in a way that decouples it from the APM package implementation because I want to do work in that package as well. Both the Transaction and Span classes in this package will be proxies to the APM implementations. That should allow us isolate changes to these proxy classes when the APM implementation changes.

My goal is to solve an immediate need I have with Laravel applications, but minimize the impact of future changes to the APM package. I have some ideas for the APM package which I need to formulate as issues for consideration. I hope to so on that project issue list in the next few days.

dstepe commented 5 years ago

@philkra Any more thoughts on this? As per my previous comment, this package should provide an abstraction from the underlying implementation.

philkra commented 5 years ago

feel free to merge