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.

Sign up for GitHub

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

Jump to bottom

Improve prometheus plugin #707

Closed
wants to merge 1 commit into from
Closed

Improve prometheus plugin #707

wants to merge 1 commit into from

Conversation

titilambert
Copy link
Contributor

Hello, this is an improvement of prometheus input plugin:

  • Prometheus parser added
  • "prometheus_" prefix removed from metric names (could be readded using "name_prefix" option)
  • Add includes/excludes options to select metric you (don't) want

Question:
@sparrc If those changes are fine to you, I would like to add "configuration examples" of Prometheus input plugin to get some data.
It could be something like (for kubelet):

[[inputs.prometheus]]
  urls = ["http://kube-node-1:4194/metrics"]
  excludes = ["^container_.*"]
  name_prefix = "k8s_"
  [inputs.prometheus.tags]
    kubeservice = "kubelet"

Where can I put such examples ? in plugins/inputs/prometheus/examples/*.conf folder ?

@titilambert titilambert mentioned this pull request Feb 17, 2016
Closed
@titilambert titilambert changed the title Improve prometheus plugin [WIP] Improve prometheus plugin Feb 17, 2016
sparrc
sparrc reviewed Feb 17, 2016
View reviewed changes
plugins/parsers/registry.go
@@ -53,6 +54,9 @@ type Config struct {

// DefaultTags are the default tags that will be added to all parsed metrics.
DefaultTags map[string]string

// PromFormat only to Prometheus
PromFormat map[string]string
Copy link
Contributor

Choose a reason for hiding this comment

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

These options in Config are more meant to be something that users would specify in the telegraf config file. From my understanding of the prometheus parser, PromFormat is something that gets specified by the headers in the http response?

ie, I don't think this needs to go here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok ! You're right, it's http header...
How can I pass this stuff to the Prometheus parser ?

@sparrc
Copy link
Contributor

sparrc commented Feb 17, 2016

@titilambert would any other plugin use this parser other than the prometheus input? The idea behind generic parsers was that it could get used by things like message broker consumers and the exec input. In this instance I don't think it's really the case? If so I think the prometheus parser should just live within the prometheus plugin directory.

@titilambert
Copy link
Contributor Author

@sparrc good point ! I don't know if could be interesting for exec plugin ... (I'm not a Prometheus user)
If not I will move it as you suggest.

@sparrc
Copy link
Contributor

sparrc commented Feb 17, 2016

let's just have it in the prometheus directory for now....it'd be easier to break it out into the general parsers later than the other way around

@titilambert
Copy link
Contributor Author

perfect !

@titilambert titilambert force-pushed the prometheus branch 2 times, most recently from c085151 to 475d535 Compare February 19, 2016 18:32
@titilambert
Copy link
Contributor Author

@sparrc I just made the change !

@sparrc
Copy link
Contributor

sparrc commented Feb 19, 2016

@titilambert I'd prefer not to have the includes and excludes options added here, is there a reason that pass and drop don't work for you?

If so I'd prefer we come up with a solution that applies to all plugins (similar to pass and drop)

@titilambert
Copy link
Contributor Author

@sparrc
When I query a Prometheus page I get a lot of data/metrics and sometimes I just want some of them.
includes/excludes is to filter on metrics name but pass/drop is to filter on fields name...

So I'm agree, I will add it like pass/drop and tagpass/tagdrop. Do you have any suggestion about names: metricpass/metricdrop ???

@sparrc
Copy link
Contributor

sparrc commented Feb 19, 2016

This should probably be in a separate PR, I would say:

  1. Rename pass/drop to field_pass/field_drop (but keep legacy support)
  2. Add metric_pass/metric_drop for dropping by metric names
  3. Also make this consistent in output plugins. I believe output plugins currently pass/drop by metric name (I don't think we need to implement field_pass/field_drop for them yet)
  4. Keep it all glob matching for now....if we need regex we can implement as separate arg or add a prefix to indicate regex

@titilambert
Copy link
Contributor Author

@sparrc Perfect !

@titilambert titilambert force-pushed the prometheus branch 3 times, most recently from f05a14b to 5d30300 Compare February 20, 2016 05:18
@titilambert titilambert mentioned this pull request Feb 20, 2016
Closed
@titilambert
Copy link
Contributor Author

This PR is now split (see #730)

@sparrc sparrc mentioned this pull request Feb 20, 2016
Closed
@sparrc
Copy link
Contributor

sparrc commented Mar 1, 2016

@titilambert now that #730 is merged can you rebase this?

I think that the additional config examples should go in the README (which might not exist yet)

@titilambert titilambert force-pushed the prometheus branch 2 times, most recently from 7f8573d to 2d6a8c3 Compare March 1, 2016 18:49
@titilambert
Copy link
Contributor Author

Rebased and Readme added

@titilambert titilambert changed the title [WIP] Improve prometheus plugin Improve prometheus plugin Mar 1, 2016
@titilambert titilambert mentioned this pull request Mar 4, 2016
Closed
@jchauncey
Copy link

So I have been using this PR for a few days and it works really well. Only thing that should be done is add the ability to specify the bearer token and verify ssl attribute that I posted in the gist on the kubernetes issue.

@sparrc
Copy link
Contributor

sparrc commented Mar 16, 2016

@titilambert is this ready to go?

@titilambert
Copy link
Contributor Author

@sparrc yes, it's good for me !
As @jchauncey said it' just missed token handling but I remember a PR for this ... but I can not found it :(

@jchauncey
Copy link

Look in the kubernetes plugin issue. I posted a gist that has the code.

@jchauncey
Copy link

If you don't have it. I can post a branch with your code and my token stuff so you can pull that over.

@titilambert
Copy link
Contributor Author

@jchauncey oki ! So @sparrc can merge this PR then we can add your token stuff with a new PR ?

@titilambert
Copy link
Contributor Author

I just created an issue for this: #864

@sparrc sparrc closed this in bac1c22 Mar 17, 2016
@sparrc
Copy link
Contributor

sparrc commented Mar 17, 2016

thanks again @titilambert !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@titilambert @sparrc @jchauncey