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 prometheus histogram metric tracker for multiple pools #1692

Merged
merged 2 commits into from Jan 12, 2021
Merged

Fix prometheus histogram metric tracker for multiple pools #1692

merged 2 commits into from Jan 12, 2021

Conversation

vaIgarashi
Copy link
Contributor

@vaIgarashi vaIgarashi commented Nov 15, 2020

Currently it's not possible to use PrometheusMetricsTrackerFactory if you have multiple pools. Even if you create one instance of factory it's not thread-safe and collector created multiple times.

Related to #1331
Fixes #1465

@codecov
Copy link

codecov bot commented Nov 15, 2020

Codecov Report

Merging #1692 (2220196) into dev (8217f20) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##                dev    #1692   +/-   ##
=========================================
  Coverage     70.11%   70.11%           
  Complexity      561      561           
=========================================
  Files            26       26           
  Lines          2118     2118           
  Branches        296      296           
=========================================
  Hits           1485     1485           
  Misses          487      487           
  Partials        146      146           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8217f20...2220196. Read the comment docs.

@vaIgarashi vaIgarashi changed the title Fix multiple prometheus histogram metric trackers Fix multiple prometheus histogram metric tracker for multiple pools Nov 15, 2020
@vaIgarashi vaIgarashi changed the title Fix multiple prometheus histogram metric tracker for multiple pools Fix prometheus histogram metric tracker for multiple pools Nov 15, 2020
@rabindrarakshit
Copy link

rabindrarakshit commented Jan 5, 2021

@brettwooldridge Could you please check this. It will be good to have this in. We want to use the Histogram approach, the normal summary approach blocks a huge number of threads in prometheus code blocks for a new pod scaled during peak hours.

Also this looks duplicate of these: #1465, #1467
May be we can close one.

@brettwooldridge
Copy link
Owner

@rabindrarakshit Ok, your chance to play God. Which of the two pull requests is better? Better being: concise and correct with good test coverage.

@rabindrarakshit
Copy link

rabindrarakshit commented Jan 7, 2021

Thanks for the honour @brettwooldridge :) I would like to go with this : #1692 It looks good to the point (also the ability to remove metrics when connection pool is shutting down) except one minor remark about the .* imports. If you are fine with that then we can proceed with this.

Both are somewhat copy paste of this : #1331

@rabindrarakshit
Copy link

@brettwooldridge could you please approve this pr so that we can get it merged and possibly a release would be very good. We want to test the histogram and see if it brings any improvement over the classical summary approach!

@brettwooldridge brettwooldridge merged commit 28cb7c0 into brettwooldridge:dev Jan 12, 2021
@brettwooldridge
Copy link
Owner

@rabindrarakshit I intend to release within the next week. Trying to get a few more pull requests integrated.

@rabindrarakshit
Copy link

ok that would be great. Thanks @brettwooldridge

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.

PrometheusHistogramMetricsTracker should not register metrics twice
3 participants