search

prometheus / client_java

Prometheus instrumentation library for JVM applications
http://prometheus.github.io/client_java/
Apache License 2.0
2.17k stars 792 forks source link

Redeploying WAR to container fails due to MetricsFilter not unregistering collectors #359

Closed evfool closed 6 years ago

evfool commented 6 years ago

Currently Prometheus MetricsFilter registers collectors in the servlet Filter init method, but doesn't unregister them in the destroy method, making effectively impossible of redeploying a WAR file (which calls destroy() on filters, then after refreshing the classes, calls init() again) due to metrics already being registered.

Expected outcome: MetricsFilter destroy should unregister metrics collectors (registered in the filter init), so that a redeploy of a WAR file is possible without restarting the container.

brian-brazil commented 6 years ago

Metrics shouldn't go away until the process dies, so this behaviour is as designed. You need to reuse the metrics.

evfool commented 6 years ago

Yes, I agree that that should be the normal behaviour, and I also found that the metrics are reset. Any other option to allow context refresh (that would mean querying the existing histogram collector and using that if exists, creating it if it doesn't )?

In a microservice architecture where only one webapplication is executed inside a JVM a JVM restart equals to a webapplication restart, but in context of multiple webapplications deployed in a servlet container refreshing/redeploying a war should not be blocked by metrics, this is the problem I am trying to solve. As for the metrics being reset, this is similar to how a single-webapp JVM would function: redeploying it would reset the metrics, and I considered that the same thing should happen on application redeploy.

evfool commented 6 years ago

And with your analogy, until the process dies: when the webapplication is redeployed, the "webapplication process" dies, and starts afresh from all points of view, possibly with new classes, resources, etc, so it could be accepted that metrics are also reset. Based on the Servlet Filter specification[1], the destroy method "Includes any special cleanup requirements in your implementation. ". Given that without this cleanup the re-registration of the filter is not possible, blocking the webapp from starting, maybe it should be done.

[1] http://otndnld.oracle.co.jp/document/products/as10g/101300/B25221_03/web.1013/b14426/filters.htm

brian-brazil commented 6 years ago

By process I mean process. If you're treating a single processes as a cluster scheduler, then you need isolation between the applications inside it.

evfool commented 6 years ago

Ok, so I guess there is no chance of you accepting a solution as a merge request, so we will have to implement a custom MetricsFilter (as subclassing doesn't help given that we won't have the histogram instance to unregister) to support WAR redeploy. Thanks for your comments, feel free to close this.

brian-brazil commented 6 years ago

The filter is intended to be something to get you up and running quickly. If you're doing something more fancy you should use your own instrumentation.