fix: Concurrency issues with Global Registry, issues with incorrect Path in Metrics

ansrivas / fiberprometheus

prometheus middleware for Fiber

MIT License

172 stars 37 forks source link

fix: Concurrency issues with Global Registry, issues with incorrect Path in Metrics #197

Closed gaby closed 7 months ago

gaby commented 7 months ago

Fixed concurrency issues related to using the Prometheus Global Registry. Each instance now uses it's own registry
Fixed issue with Path in metrics, it now uses the Path from the Request ctx.Request().RequestURI().
Added benchmarks
Added unit-tests for group handlers
Replaced deprecated adaptor middleware with the one in https://github.com/gofiber/fiber

Fixes #187 #191 Closes #195 #196

@ansrivas I figured fixing these issues here first before migrating the middleware would make more sense. That way anyone using this repo will still get them.

sashayakovtseva commented 3 months ago

Seems like this PR broke existing code and http metrics will not be collected for users who used default registry and not used RegisterAt (for instance, one could have a separate metrics server, created elsewhere apart from fiberprometheus.New)

gaby commented 3 months ago

@sashayakovtseva That was a bug, the middleware was sharing a global variable. It was exposed when running test in parallel.

You can pass your current registry to the middleware and they both will use the same using NewWithRegistry()

gaby commented 3 months ago

Another option would be adding a config boolean to allow New use default registry or not. We are moving this middleware to https://github.com/gofiber/contrib I will implement that change there

sashayakovtseva commented 3 months ago

You can pass your current registry to the middleware and they both will use the same using NewWithRegistry()

Yeah, I found that out already when metrics disappeared :)

the middleware was sharing a global variable

I guess that is an issue if one has multiple fiberprometheus.New calls concurrently, correct?

gaby commented 3 months ago

You can pass your current registry to the middleware and they both will use the same using NewWithRegistry()

Yeah, I found that out already when metrics disappeared :)

the middleware was sharing a global variable

I guess that is an issue if one has multiple fiberprometheus.New calls concurrently, correct?

Yes, if you have multiple prometheus-go clients they all use the same defaultRegistry. So one solution is to create a Registry a pass it to all your clients.