Skip to content

Implements #init_label_set method for all metric classes#156

Merged
dmagliola merged 1 commit into
prometheus:masterfrom
jbernardo95:init-label-set
Nov 11, 2019
Merged

Implements #init_label_set method for all metric classes#156
dmagliola merged 1 commit into
prometheus:masterfrom
jbernardo95:init-label-set

Conversation

@jbernardo95

Copy link
Copy Markdown
Contributor

This a draft PR of a solution for the problem described in #141.

Comment thread lib/prometheus/client/histogram.rb
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 62ab050 on jbernardo95:init-label-set into b3bbe23 on prometheus:master.

@coveralls

coveralls commented Oct 1, 2019

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 0e0f17f on jbernardo95:init-label-set into 9f08a53 on prometheus:master.

Comment thread lib/prometheus/client/metric.rb
@jbernardo95
jbernardo95 force-pushed the init-label-set branch 2 times, most recently from a67a90a to 6ad23c9 Compare October 1, 2019 21:27
@jbernardo95

Copy link
Copy Markdown
Contributor Author

@SuperQ @dmagliola @Sinjo can I get a review on this on when you have the chance?

Comment thread lib/prometheus/client/histogram.rb Outdated

@dmagliola dmagliola left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Joao! Sorry for the slow review!
I really like this, thank you for doing it!

I'd like to simplify the code in Histograms a bit (see my comments there), but the rest looks great.

One more important thing, though: Could you add a section to the Readme, probably right before "Reserved labels", briefly explaning the problem and how this method solves it? (you don't want metrics to spontaneously appear the first time they're incremented, you want them to be there from the beginning, so if you know what your labels values are upfront, you can declare them)

And a brief example ideally :)
Thank you

@jbernardo95
jbernardo95 force-pushed the init-label-set branch 3 times, most recently from 85c6081 to c819556 Compare November 2, 2019 22:55
@jbernardo95

jbernardo95 commented Nov 2, 2019

Copy link
Copy Markdown
Contributor Author

@dmagliola Feedback addressed let me know what you think! 🙂

@jbernardo95
jbernardo95 requested a review from dmagliola November 2, 2019 22:56
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread lib/prometheus/client/histogram.rb Outdated

@dmagliola dmagliola left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few requests, mostly around the README, and then we're ready to merge!

@jbernardo95

jbernardo95 commented Nov 4, 2019

Copy link
Copy Markdown
Contributor Author

So many typos 😬

@dmagliola Ready for a second round 👀

@jbernardo95
jbernardo95 requested a review from dmagliola November 4, 2019 22:54
Comment thread README.md Outdated
Comment thread lib/prometheus/client/histogram.rb Outdated

@dmagliola dmagliola left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 tiny changes. Please do those two and i'll merge it tomorrow!
Thanks!

Signed-off-by: Joao Bernardo <jb.amaro95@gmail.com>
@jbernardo95

Copy link
Copy Markdown
Contributor Author

Done 🚀

@dmagliola
dmagliola merged commit c792af8 into prometheus:master Nov 11, 2019
@dmagliola

Copy link
Copy Markdown
Collaborator

Thank you @jbernardo95 for this!

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.

4 participants