search

fluent / fluent-plugin-prometheus

A fluent plugin that collects metrics and exposes for Prometheus.
Apache License 2.0
257 stars 79 forks source link

Update client version #133

Closed ganmacs closed 4 years ago

ganmacs commented 4 years ago

fixes https://github.com/fluent/fluent-plugin-prometheus/issues/130

Two tests about summary and histogram are failing now.

summray does not have a complete feature. https://github.com/prometheus/client_ruby/tree/260e51a9f01d86c43a25bf3235e512a78ad02d74#summary

For now, only sum and total (count of observations) are supported, no actual quantiles.

context: https://github.com/prometheus/client_ruby/issues/12#issuecomment-504662523

histogram seems to have a bug (will be fixed in https://github.com/prometheus/client_ruby/pull/176)

repeatedly commented 4 years ago

I recommend to update plugin version to v2 for this change.

ganmacs commented 4 years ago

I recommend to update plugin version to v2 for this change.

Right. Currently, I think we don't have to use v1 until it starts to support summary metric because It doesn't have new features but it has degradation(incomplete feature). so I'll probably close this PR :)

kazegusuri commented 4 years ago

I think it's better to wait until the new client implements full features that this plugin requires. When the client has the features, we should migrate to use that. If we do that, do we still need to update this plugin to v2? I think if we can keep compatibility around the configurations and the output behaviors we can keep v1.

ganmacs commented 4 years ago

I think it's better to wait until the new client implements full features that this plugin requires.

👍

If we do that, do we still need to update this plugin to v2?

Maybe not. I think we can keep compatibility with v1.

ganmacs commented 4 years ago

I'm closing this PR since prometheus/client_ruby does not have our requirements. if they support them, reopen this PR (or create brand new PR).

dmagliola commented 4 years ago

Hello! I'm sorry for the delay in responding! We've now merged your PR that fixes the bug in the prometheus client, and have released version 2.0.0 with that change.

You should now be able to reopen this PR, pointing to the latest version of the gem, and it should work fine. Please let me know if that's not the case and there's any way I can help.

Thank you, Daniel

ganmacs commented 4 years ago

@dmagliola Thank you for your hard work! Unfortunately, we can't use the latest prometheus_client because it supports only sum and total in summary metrics. Do you have any plan to support all value like prometheus_client v0.10. Our specs need it (ref: https://travis-ci.org/fluent/fluent-plugin-prometheus/jobs/634143418#L384-L445)

dmagliola commented 4 years ago

The problem with summaries is that they can't be aggregated across processes, so we chose to get rid of that feature, as a trade-off for supporting forking servers.

This was following the example of the Python client, which also doesn't support them.

The recommended approach (I believe) would be to use histograms instead.

I'll keep this in mind, however, and will have a think as to whether we can somehow still support summaries for single process scenarios.

Thank you for the feedback!