search

artprima / prometheus-metrics-bundle

Symfony 5/6 Prometheus Metrics Bundle
MIT License
133 stars 29 forks source link

AppMetrics: move stopwatch initialization logic to be compatible with long running servers #17

Closed luca-nardelli closed 4 years ago

luca-nardelli commented 4 years ago

Hi! First of all thanks for the bundle! I was using it and I recently switched our setup from Nginx + FPM to Swoole server. In doing that, I noticed several errors coming up that were like this

[2020-06-19T14:51:22.358209+00:00] app.ERROR: stop() called but start() has not been called before. {"from":"response_collector","class":"Artprima\\PrometheusMetricsBundle\\Metrics\\AppMetrics"} {"user":null}

Upon further testing, I noticed that the stopwatch to measure request time was initialized in the init function. In a situation such as with Swoole (where the container and services are built only once and then stay in memory) this results in the same stopwatch instance being used for all requests, without being started again because init is called only once.

Thus, I moved the initialization of the stopwatch to the onCollectRequest handler and this solved the issue for me. I also had to update AppMetricsTest because it was relying on the assumption that init would setup the stopwatch.

Let me know if I need to do something else for this PR to be merged, thanks again!

denisvmedia commented 4 years ago

Yeah, it seems it can work this way. Thank you for your Pull Request. Just may I ask you to change the destination branch to develop? Any changes should first go there in most cases.

luca-nardelli commented 4 years ago

Yeah, my bad, I've changed it now!

denisvmedia commented 4 years ago

Please use the develop branch for now. I will soon merge it to master and make a new release. Again thank you for your improvements!