Add 'collect' URL parameter to filter enabled collectors #235

siavashs · 2017-10-07T12:55:12Z

As discussed on #234 and IRC, this PR adds a collect URL parameter to filter currently enabled collectors.

Closes: #234

SuperQ · 2017-10-07T15:53:51Z

mysqld_exporter.go

@@ -170,6 +173,76 @@ func init() {
 	prometheus.MustRegister(version.NewCollector("mysqld_exporter"))
 }

+func filter(filters map[string]bool, name string) bool {
+	flg := flag.Lookup("collect." + name)


Minor nit, I think the golang style for this would just be f.

I had used f in the original implementation, but changed it to flg later 😄
I'll change it back.

SuperQ · 2017-10-07T15:55:48Z

mysqld_exporter.go

+		HeartbeatTable:       *collectHeartbeatTable,
+	}
+
+	//start := time.Now()


Please remove commented out extras.

Ah, some leftovers from my lazy copy/pasting 😅

SuperQ · 2017-10-07T15:56:25Z

Very nice! A few minor nits.

SuperQ · 2017-10-07T16:00:39Z

Please update the README.md with usage info.

SuperQ · 2017-10-07T16:03:56Z

/cc @roman-vynar This will be great for PMM.

brian-brazil · 2017-10-07T16:58:06Z

mysqld_exporter.go

+
+	if len(query) > 0 {
+		filters = make(map[string]bool)
+		params := strings.Split(query, ",")


The usage should be ?collect[]=foo&collect[]=bar to keep in line with other places in Prometheus

Good point 👌🏻

brian-brazil · 2017-10-07T17:57:20Z

mysqld_exporter.go

+		HeartbeatTable:       *collectHeartbeatTable,
+	}
+
+	registry := prometheus.NewRegistry()


This won't include the default process stuff, so you should add it back in

roman-vynar · 2017-10-08T12:24:29Z

@SuperQ awesome 😎 Much better than having 3 exporters.
/cc @rnovikovP

@siavashs Great work!

SuperQ · 2017-10-08T14:33:54Z

mysqld_exporter.go

@@ -170,6 +172,74 @@ func init() {
 	prometheus.MustRegister(version.NewCollector("mysqld_exporter"))
 }

+func filter(filters map[string]bool, name string) bool {
+	f := flag.Lookup("collect." + name)


The only down-side to this approach is that we want to switch to kingpin for flag parsing. I haven't been able to find an equivalent feature in kingpin. 😦

Yes, it would have been nice if kingpin had a Lookup() method 😕
I can change the filter() implementation or submit a PR for kingpin 😅

Oh, I totally forgot to merge #222, which does the kingpin change. I would like to merge that, and have the filter changed to match. I think it will take too much time to get it implemented in kingpin.

Sorry, I should have taken care of the flag issue first.

No problem, I'll update my branch.

siavashs · 2017-10-12T09:12:15Z

As discussed on IRC, currently the http metrics are missing.
I could not find an example of how to instrument a handler function which initialises a custom registry, blackbox and snmp exporters only calculate the scrape duration and expose it.
I also noticed that some instrumentation functions are deprecated now and we should use promehttp instead.
So I need some help, maybe @beorn7 can give me a hint 😄

beorn7 · 2017-10-12T10:30:14Z

So I need some help, maybe @beorn7 can give me a hint 😄

The promhttp documentation is here: https://godoc.org/github.com/prometheus/client_golang/prometheus/promhttp

Prior to removing the deprecated functions, there will be more documentation, examples, and tooling. https://godoc.org/github.com/prometheus/client_golang/prometheus#InstrumentHandler is handy but doing so much wrong that we cannot just not declare it deprecated.

siavashs · 2017-10-12T13:14:09Z

143b2d0 exposes the http metrics but the values are 0/Nan.

beorn7 · 2017-10-13T08:37:58Z

mysqld_exporter.go

+	registry.MustRegister(collector.New(dsn, collect))
+	prometheus.DefaultRegisterer = registry
+
+	// Delegate http serving to Promethues client library, which will call collector.Collect.


Promethues -> Prometheus

I copied this typo from another exporter 😄

beorn7 · 2017-10-13T08:43:31Z

mysqld_exporter.go

+	registry.MustRegister(prometheus.NewGoCollector())
+	registry.MustRegister(prometheus.NewProcessCollector(os.Getpid(), ""))
+	registry.MustRegister(collector.New(dsn, collect))
+	prometheus.DefaultRegisterer = registry


This will create a data race if multiple requests hit /metrics concurrently.

I assume you are only doing this here to make prometheus.InstrumentHandler work.

If you use the individual handler instrumentation helpers in promhttp, you can avoid that. Also, you can instrument the handler appropriately. (I'm pretty sure that some of the things that get auto-instrumented now are not needed, like request size. And response latency might be better done in a histogram so that you can aggregate over many exporters.)

Yes, I noticed the issue while trying to make the instrumentation metrics work.
The next commit fixes this issue by creating a Gatherers instance from default gatherer and newly created registry per request.

beorn7 · 2017-10-13T09:17:37Z

mysqld_exporter.go

+
+	registry := prometheus.NewRegistry()
+	registry.MustRegister(collector.New(dsn, collect))
+	prometheus.DefaultRegisterer = registry


I think you can now delete this line. (As long as this line is there, you have a race.)

Ah, of course.

beorn7 · 2017-10-13T10:08:46Z

👍 from my side, leaving final say to @SuperQ .

SuperQ

Great!

arvenil · 2018-03-12T17:54:19Z

mysqld_exporter.go

+	}
+
+	registry := prometheus.NewRegistry()
+	registry.MustRegister(collector.New(dsn, collect))


@SuperQ @siavashs I think this broke scrapes_total and scrape_errors_total metrics. Previously collector.New() was created once - in main func. Now it's created on every scrape and so is scrapes_total and scrape_errors_total leading to constant values as every time scrapes_total is reset to 0, so we get only 1 scrape. It's not total anymore.
https://github.com/prometheus/mysqld_exporter/blob/master/collector/exporter.go#L107-L118

I can confirm this bug, I think we can fix it by having a separate collector for exporter metrics.

Sounds correct to me. Thanks for the report. I'll make a separate issue for this.

For the reference: #269

@SuperQ @siavashs Actually, there is one more confusing thing. Looks like now this metric is constant 2 #245, and even after fixing it, this will be increased every http call by 2 because each call ask for metrics and their description, and Describe again increases scrapes_total metric. Should scrapes_total be increased also when Describe is called? If only Collect would increase this metric then each http call would increase this metric only by 1. I'm working right now on fix for this so let me please know ;)
But maybe increasing this metric by 2 is correct and we simply should have separate metric for http calls?

arvenil · 2018-06-27T15:29:43Z

mysqld_exporter.go

-	}
-
-	c := collector.New(dsn, collect)
-	prometheus.MustRegister(c)


After this change, registering collector every http request, we now call Describe() every http request, and as in result we call Collect() twice every http request.

SuperQ self-requested a review October 7, 2017 15:53

SuperQ reviewed Oct 7, 2017

View reviewed changes

SuperQ requested a review from brian-brazil October 7, 2017 15:56

brian-brazil reviewed Oct 7, 2017

View reviewed changes

SuperQ reviewed Oct 8, 2017

View reviewed changes

siavashs added 8 commits October 8, 2017 18:59

Add 'collect' URL parameter to filter enabled collectors

a350141

Add promhttp to vendor

9132aa5

Some nit picking

f0bfe39

Use collect[] parameter for compatibility

c95b06a

Add process collector

eb3351e

Update README

93c1855

Corrections to README

dac50b0

Reimplement filter() for compatibility with kingpin flags

2e325ac

siavashs force-pushed the parameters branch from ee4d75f to 2e325ac Compare October 8, 2017 17:22

Add go collector

4c77a9a

Change default registerer to instrument http handler

143b2d0

beorn7 reviewed Oct 13, 2017

View reviewed changes

siavashs added 2 commits October 13, 2017 10:44

Pass default gatherer to promhttp handler to expose all metrics

3e25158

Remove comments, fix typo

b2b8a66

beorn7 reviewed Oct 13, 2017

View reviewed changes

Some code cleanup

3e2ecb7

SuperQ approved these changes Oct 13, 2017

View reviewed changes

SuperQ merged commit 8b92fe8 into prometheus:master Oct 13, 2017

frimik mentioned this pull request Nov 11, 2017

collect parameter commit broke metric counter(s)? #245

Closed

arvenil reviewed Mar 12, 2018

View reviewed changes

arvenil reviewed Jun 27, 2018

View reviewed changes

arvenil mentioned this pull request Jun 27, 2018

PMM-2612: Scrape MySQL database only once and fix scrape counter. #292

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'collect' URL parameter to filter enabled collectors #235

Add 'collect' URL parameter to filter enabled collectors #235

siavashs commented Oct 7, 2017 •

edited by SuperQ

Loading

SuperQ Oct 7, 2017

siavashs Oct 7, 2017

SuperQ Oct 7, 2017

siavashs Oct 7, 2017

SuperQ commented Oct 7, 2017

SuperQ commented Oct 7, 2017

SuperQ commented Oct 7, 2017

brian-brazil Oct 7, 2017

siavashs Oct 7, 2017

brian-brazil Oct 7, 2017

roman-vynar commented Oct 8, 2017 •

edited

Loading

SuperQ Oct 8, 2017

siavashs Oct 8, 2017 •

edited

Loading

SuperQ Oct 8, 2017

siavashs Oct 8, 2017

siavashs commented Oct 12, 2017

beorn7 commented Oct 12, 2017

siavashs commented Oct 12, 2017

beorn7 Oct 13, 2017

siavashs Oct 13, 2017

beorn7 Oct 13, 2017

siavashs Oct 13, 2017

beorn7 Oct 13, 2017

siavashs Oct 13, 2017

beorn7 commented Oct 13, 2017

SuperQ left a comment

arvenil Mar 12, 2018

siavashs Mar 12, 2018

SuperQ Mar 12, 2018

AlekSi Mar 20, 2018

arvenil Jun 21, 2018 •

edited

Loading

arvenil Jun 27, 2018 •

edited

Loading

Add 'collect' URL parameter to filter enabled collectors #235

Add 'collect' URL parameter to filter enabled collectors #235

Conversation

siavashs commented Oct 7, 2017 • edited by SuperQ Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperQ commented Oct 7, 2017

SuperQ commented Oct 7, 2017

SuperQ commented Oct 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roman-vynar commented Oct 8, 2017 • edited Loading

Choose a reason for hiding this comment

siavashs Oct 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siavashs commented Oct 12, 2017

beorn7 commented Oct 12, 2017

siavashs commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 commented Oct 13, 2017

SuperQ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvenil Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

arvenil Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

siavashs commented Oct 7, 2017 •

edited by SuperQ

Loading

roman-vynar commented Oct 8, 2017 •

edited

Loading

siavashs Oct 8, 2017 •

edited

Loading

arvenil Jun 21, 2018 •

edited

Loading

arvenil Jun 27, 2018 •

edited

Loading