search

miekg / caddy-prometheus

Prometheus metrics middleware for caddy
Apache License 2.0
65 stars 26 forks source link

Allow specifying custom latency and size buckets #52

Closed hairyhenderson closed 5 years ago

hairyhenderson commented 5 years ago

Adds 2 new options: size_buckets and latency_buckets, which can be used to set custom bucket values for the latency and size histograms.

Signed-off-by: Dave Henderson dhenderson@gmail.com

miekg commented 5 years ago

Should not make the defaults work better? If we can get away with fixing it without adding new options that would be great.

On Sat, 15 Jun 2019, 20:00 Dave Henderson, notifications@github.com wrote:

Adds 2 new options: size_buckets and latency_buckets, which can be used to set custom bucket values for the latency and size histograms.

Signed-off-by: Dave Henderson dhenderson@gmail.com

You can view, comment on, or merge this pull request online at:

https://github.com/miekg/caddy-prometheus/pull/52 Commit Summary

  • Allow specifying custom latency and size buckets

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/miekg/caddy-prometheus/pull/52?email_source=notifications&email_token=AACWIW5B2J74ZTISMVV4DDTP2U36XA5CNFSM4HYPUMRKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GZWU3GQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AACWIW6QKNBQWCFGKRAQAU3P2U36XANCNFSM4HYPUMRA .

hairyhenderson commented 5 years ago

Should not make the defaults work better?

Sure, I could change the defaults, but different people have different expectations around which thresholds are important to measure. Plus, the current latency histograms have 20 buckets each, which some consider too many.

If we can get away with fixing it without adding new options that would be great.

Not sure there is a way, since my goal here is to add flexibility 😉

hairyhenderson commented 5 years ago

@miekg any thoughts on this? If you're not OK with this, I understand, I can maintain a fork instead :)

miekg commented 5 years ago

I'm fine with changi g the default buckets, but just that. Don't think we should have an option though

On Thu, 27 Jun 2019, 15:45 Dave Henderson, notifications@github.com wrote:

@miekg https://github.com/miekg any thoughts on this? If you're not OK with this, I understand, I can maintain a fork instead :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/miekg/caddy-prometheus/pull/52?email_source=notifications&email_token=AACWIWZASJKZ5AHYLJSPTHLP4TG7JA5CNFSM4HYPUMRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYXLERA#issuecomment-506376772, or mute the thread https://github.com/notifications/unsubscribe-auth/AACWIWZUKI7LLW74KGWOYL3P4TG7JANCNFSM4HYPUMRA .

hairyhenderson commented 5 years ago

@miekg can I ask why? It seems to me that with something as general-purpose as Caddy, flexibility like this would be quite useful.

miekg commented 5 years ago

Why isn't a default good enough?

On Thu, 27 Jun 2019, 16:22 Dave Henderson, notifications@github.com wrote:

@miekg https://github.com/miekg can I ask why? It seems to me that with something as general-purpose as Caddy, flexibility like this would be quite useful.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/miekg/caddy-prometheus/pull/52?email_source=notifications&email_token=AACWIW7IBB3WWCA3B5SC75TP4TLLNA5CNFSM4HYPUMRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYXO43I#issuecomment-506392173, or mute the thread https://github.com/notifications/unsubscribe-auth/AACWIW3E4NEWPHPQHLIUCELP4TLLNANCNFSM4HYPUMRA .

hairyhenderson commented 5 years ago

@miekg we have a few different teams that use Caddy to serve different things, and each team has different targets for latency - rather than add a half-dozen more "odd" buckets to the list, I'd rather be able to customize for each

hairyhenderson commented 5 years ago

@miekg it's OK if the answer is "no thanks" - if you're not totally convinced that it's the right feature to support long-term, you probably should say no - I certainly would in your place!

miekg commented 5 years ago

Im fine with merging. In fact, how about becoming a co-maintainer?

On Thu, 27 Jun 2019, 16:49 Dave Henderson, notifications@github.com wrote:

@miekg https://github.com/miekg it's OK if the answer is "no thanks" - if you're not totally convinced that it's the right feature to support long-term, you probably should say no - I certainly would in your place!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/miekg/caddy-prometheus/pull/52?email_source=notifications&email_token=AACWIW6SEA44R2XS6GUBM43P4TOOZA5CNFSM4HYPUMRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYXRQTA#issuecomment-506402892, or mute the thread https://github.com/notifications/unsubscribe-auth/AACWIWYW2RHL3S2ZYJPUKFTP4TOOZANCNFSM4HYPUMRA .

hairyhenderson commented 5 years ago

Thanks! I'd be fine with co-maintaining - what sort of expectations would you have from me?

miekg commented 5 years ago

[ Quoting notifications@github.com in "Re: [miekg/caddy-prometheus] Allow ..." ]

Thanks! I'd be fine with co-maintaining - what sort of expectations would you have from me?

lol, you already have access :-)

expectations we come down to: try to apply KISS, fields questions, close issues, maintain the code, etc.

Thanks!

hairyhenderson commented 5 years ago

lol, you already have access :-)

totally forgot you gave me access ages ago 😂

expectations we come down to: try to apply KISS, fields questions, close issues, maintain the code, etc.

Ok, will do my best!

I'll start by merging this one 😉