search

elli-lib / elli_prometheus

Elli middleware for collecting stats via Prometheus.
https://github.com/elli-lib/elli_prometheus/blob/master/doc/elli_prometheus.md
Other
14 stars 8 forks source link

Small rewrite #3

Closed deadtrickster closed 8 years ago

deadtrickster commented 8 years ago

Hey!

This PR incorporates #1 and #2. It also allows configurable scrape path, format and duration buckets. I implemented configuration based on prometheus application environment. It's probably not idiomatic from elli's point of view so please do let me know if this is not acceptable.

I would also like to note that status_code label can lead to 'overflow'. While set of values of this label is definitely finite it's still can be large. Prometheus treats each label set as separate time series. Likely there is also usability issue - imagine you want to graph all successful replies then you have to use regex matches. So maybe introduce status_class label and make status_code optional?

yurrriq commented 8 years ago

Is the something inherently wrong with regex matches? I'm very new to Prometheus. Thanks for all your work on this.

deadtrickster commented 8 years ago

Not 'wrong'! Maybe it's just me not liking them :-) (they are definitely slower and require more thought). Anyway, status_code can dramatically increase storage usage. While this is highly use-case dependent (prometheus can handle millions of TS after all) it looks really verbose. I want to stress I don't want to remove status_code but introduce status_class (informational|success|redirection|client-error|server-error) as default.

yurrriq commented 8 years ago

Right. :+1: for status_class. Those values sound good to me too.

yurrriq commented 8 years ago

Maybe we should do the configuration in elli_prometheus instead. I could imagine an edge case where someone is using another Prometheus plugin of sorts and the configuration conflicts.

deadtrickster commented 8 years ago

the configuration conflicts

this is why I'm namespacing settings under elli_exporter.

yurrriq commented 8 years ago

Bah, right. Ok this looks good to me then. I'm mobile atm.

deadtrickster commented 8 years ago

So do you want me to implement status_class? I feel like I want it in prometheus-plugs too and therefore I'll just add it as a helper to prometheus.erl itself (and default duration buckets too).

yurrriq commented 8 years ago

That sounds good to me!