liip / LiipImagineBundle

Symfony Bundle to assist in image manipulation using the imagine library
http://liip.ch
MIT License
1.66k stars 378 forks source link

Add telemetry hooks #1467

Open pbowyer opened 2 years ago

pbowyer commented 2 years ago

Is your feature request related to a problem? Please describe. We've recently started using telemetry in production and discovered how long LiipImagineBundle filter sets can sometimes take to run. On closer inspection it's because some filters are conditional (e.g. image rotation only runs if the image needs rotating) and a couple of filters we've created are slow.

Telemetry told us the overall 'use this bundle' call was slow, but it would be useful to monitor the time each filter takes.

Describe the solution you'd like I'd like some hooks to be present in LiipImagineBundle that we can subscribe to and use to create telemetry spans. We're using Tideways but others may use Opentelemetry, Datadog or want to push the details to Monolog.

Why have I created an issue and not a pull request? There are many ways this could be implemented and I haven't seen the PHP community reach any consensus on how they'd like to support telemetry in libraries (or to think about it). AFAIK there's no equivalent in PHP for https://hexdocs.pm/telemetry/readme.html.

The bundle already uses Symfony's EventDispatcher so using this wouldn't add extra dependencies. There's a balance between emitting useful events and emitting too many, but I think the execution of filters/post-processors is core enough to make sense to record.

We edited FilterManager::applyFilters() and FilterManager::applyPostProcessors() to create telemetry spans around each filter call https://github.com/liip/LiipImagineBundle/blob/2.x/Imagine/Filter/FilterManager.php#L88

Describe alternatives you've considered The alternatives we've used so far are:

  1. Add the spans to our custom filters. Pro: no need to change the library. Con: we don't get insight into other filters
  2. Edit LiipImagineBundle directly. Pro: we get full insight into filters. Con: we have to maintain a fork with the changes (actually: we didn't fork, we edited in place whilst testing 🙈
dbu commented 2 years ago

hi, thanks for the detailed explanation of why you want this :+1: i think raising filter start and finished events sounds like a good idea. it could also be used for other debugging purposes, e.g. if a filter breaks or such problems. i wonder if the events should allow to do anything with the image or e.g. tell that the filter has to be skipped or whatever, or if they would be purely informational. but unless there is something extremly obvious, its better we only have the image name and filter name in the event and don't support further side effect of the events until somebody has a good use case that they want to support.

when using something like blackfire, you would afaik see which part of the code took how long, but it needs to do things with the php runtime to report that.

do you want to do a pull request to have the filter manager do events?

pbowyer commented 2 years ago

I feel strongly that these should be side effect free. If someone wants to change the bundle to use or expose events which change behaviour that should be done separately, as it needs more thought.

I will do a pull request. It may take some weeks as I'm busy and want to get it right but it will come eventually.