Closed cchantep closed 3 years ago
up
A couple of thoughts:
prefixes
parameter? Can't you just define a separate target for each prefix?Also, this PR is adding a lot of different features:
This makes it difficult to assess each change individually so I'd suggest separate PRs.
Rather than using a parameter to filter based on storage class, what we could do is export metrics per-storage class for every storage class by default. Then users can filter based on storage class in PromQL without having to specifically set a storage class for each probe. This gives extra flexibility without an added configuration burden.
Our use case is not to even expose metrics for storage class we are not interested in (to avoid storage and network). You can then even tag with storage class even without such filter. That's an evolution.
Do we need an extra prefixes parameter? Can't you just define a separate target for each prefix?
No
This makes it difficult to assess each change individually so I'd suggest separate PRs.
I could generally, but for such a small PR, splitting is unnecessary from my point of view.
Our use case is not to even expose metrics for storage class we are not interested in (to avoid storage and network). You can then even tag with storage class even without such filter. That's an evolution.
How many storage classes are there, 7? In the context of a prometheus exporter, that's not a huge amount of cardinality and I don't think it would add much network burden in terms of the increase in the size of the response. For instance, there's a lot more possible http response codes but in general the ability to drop specific codes is not something that is built into exporters/libraries.
If you want to avoid storing series for particular storage classes, or you only want to keep a particular set, then you could use metrics_relabel_configs
in Prometheus to achieve that.
For those reasons, I don't think we need filtering of storage classes at the exporter level, as it can be achieved in Prometheus itself.
No
No we don't need it, or no you can't define a separate target for each prefix? As I see it, you can just enumerate the different bucket/prefix combos as targets so there's no need to add an extra parameter to the s3 exporter. Plus that gives you the benefit of multiple prefixes not sharing the same scrape timeout, or an error for one prefix affecting the scrape of another.
I could generally, but for such a small PR, splitting is unnecessary from my point of view.
I'm inclined not to accept the prefixes
parameter or the default options, so I would suggest we make this PR about refining the storage_class
label (which I think is a very valuable addition).
Thank you by the way for the contribution!
You are free to refactor this. Anyway we use the code as-is in our fork. Best regards.
Allow to configure the exporter to expose specific prefix tags and to filter for a specific storage class (e.g. exclude GLACIER objects)