Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Mar 18, 2024

Conversation

gaby
Copy link
Collaborator

@gaby gaby commented Mar 17, 2024

  • 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.

@ansrivas ansrivas merged commit bfc9e01 into ansrivas:master Mar 18, 2024
6 checks passed
@gaby gaby deleted the fixes branch March 25, 2024 02:53
@gaby gaby mentioned this pull request Apr 30, 2024
@sashayakovtseva
Copy link

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
Copy link
Collaborator Author

gaby commented Jul 25, 2024

@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
Copy link
Collaborator Author

gaby commented Jul 25, 2024

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
Copy link

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
Copy link
Collaborator Author

gaby commented Jul 25, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

incorrect path when using groups
4 participants