Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating plugin to support latest prometheus ruby client version #180

Merged
merged 1 commit into from
Feb 18, 2021

Conversation

AntoineC44
Copy link
Contributor

@AntoineC44 AntoineC44 commented Nov 8, 2020

Hello,

I would like to implement the following feature of the ruby prom client: init_label_set that would allow metric init if specified by user in fluent.conf.

Though current fluent-plugin-prometheus build is blocked to old versions <0.10 of prometheus ruby client. init_label_set method is only available in recent versions.

This PR thus aims at using latest prom client version.

In detail

  • fixing method signatures to match the recent versions of prometheus client
  • fixing now mandatory declaration of labels at initialization of metrics
  • updating tests to reflect changes
  • updating internal monitoring plugins to reflect change

Please note the loss of summary metric quantiles as removed in this Pull Request on prometheus-client repo
This effectively means that any summary metric will no longer display quantile labels (e.g. quantile="0.99") but only sum and count.
Though personal experience on production environments showed anyway fluentd performance issues when over-using summary quantile computations. Prefer thus histograms instead - with custom buckets if needed - and computation of quantiles on prometheus server side. See the prometheus official documentation on this topic here.

@AntoineC44
Copy link
Contributor Author

Looking for some feedback before giving sign-off
All tests passed + redid some tests in local

if @registry.exist?(name)
@registry.get(name)
else
@registry.gauge(name, docstring: docstring, labels: @base_labels.keys + [:plugin_id, :plugin_category, :type])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really fond of duplicating the labels [:plugin_id, :plugin_category, :type], could be abstracted in an attribute or a method to align labels here and in labels method line 91. Any feedback?
Same for the 2 other monitoring plugins

@AntoineC44 AntoineC44 marked this pull request as draft November 14, 2020 17:29
@AntoineC44 AntoineC44 force-pushed the feature/update_plugin branch from 1da0ab6 to ed599a4 Compare November 21, 2020 13:12
@AntoineC44 AntoineC44 marked this pull request as ready for review November 21, 2020 13:20
@@ -1,6 +1,6 @@
Gem::Specification.new do |spec|
spec.name = "fluent-plugin-prometheus"
spec.version = "1.8.4"
spec.version = "1.9.0"
Copy link
Member

Choose a reason for hiding this comment

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

Don't include version update in feature request patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be sure, you want this removed and you will yourself do a commit for version update and changelog? Or should I do another dedicated commit?

@@ -14,7 +14,7 @@ Gem::Specification.new do |spec|
spec.require_paths = ["lib"]

spec.add_dependency "fluentd", ">= 1.9.1", "< 2"
spec.add_dependency "prometheus-client", "< 0.10"
spec.add_dependency "prometheus-client", "= 2.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

= is not recommended because users can't use bug fixed version like 2.1.1.
Use >= 2.1.0 or ~> 2.2 like pattern.

@AntoineC44 AntoineC44 force-pushed the feature/update_plugin branch from ed599a4 to 8bbf46f Compare November 24, 2020 22:36
@AntoineC44
Copy link
Contributor Author

Hello @repeatedly, is this ok for you?

@repeatedly repeatedly merged commit f910765 into fluent:master Feb 18, 2021
@repeatedly
Copy link
Member

Sorry for the delay. Just merged!
I will release new version soon with major version update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants