textfile: fix duplicate metrics error

[liwei]

The textfile gatherer should only be added to gatherer list once.

Closes #704

[SuperQ]

LGTM, Thanks!

[fpletz]

Applying this PR fixed these annoying errors for us.

[WilliButz]

This actually fixes a critical issue for us, as the machines which have the textfile collector enabled run out of memory and start swapping heavily after a few hours because of the steadily growing log messages. I hope this will be included in the next patch release :)

[mdlayher]

Nit: with this being at the package level, addOnce is pretty generic. How about textfileAddOnce?

[liwei]

looks reasonable, i will fix and resubmit.

[juliusv]

Hmm... At first I wondered "Why is NewTextFileCollector() even called multiple times?". Then I saw that the introduction of request-param-based collector filtering changed the node exporter to re-instantiate every collector on every scrape. Wouldn't it be better to instantiate all the collectors only once at startup and then only filter the registration per scrape? (/cc @siavashs)

Of course, the fact that this constructor modifies the global default gatherer is somewhat of a hack that's necessary because the node exporter's collector module interface doesn't allow a collector to have a method that gets called to extend the default gatherer (which in the case of the textfile collector is necessary because we need to set client-side custom timestamps on the metrics). I wonder if introducing that to the collector.Collector interface is a good idea just for this single one-off collector though.

There's something broken about the current state though (with or without this PR): if the constructor registers the textfile output with the Prometheus default gatherer, and

node_exporter/node_exporter.go

Lines 54 to 57 in f3a7022

	gatherers := prometheus.Gatherers{
	prometheus.DefaultGatherer,
	registry,
	}

adds the default gatherer to every scrape, then we don't properly filter out the textfile collector when it isn't included in the passed HTTP collect[] filters.

I think maybe that makes the case for introducing that extra custom gatherer method to the collector.Collector interface indeed, instead of stacking one hack ontop of another?

[brian-brazil]

I think maybe that makes the case for introducing that extra custom gatherer method to the collector.Collector interface indeed, instead of stacking one hack ontop of another?

That'd be yet another hack in my opinion. The correct solution is to make the text file collector a normal collector.

[juliusv]

Without setting a custom timestamp? Or is that possible somehow meanwhile? On Nov 24, 2017 10:52 AM, "Brian Brazil" <[email protected]> wrote: I think maybe that makes the case for introducing that extra custom gatherer method to the collector.Collector interface indeed, instead of stacking one hack ontop of another? That'd be yet another hack in my opinion. The correct solution is to make the text file collector a normal collector. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#738 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAg1mKj5xObKX8Uey8xAGj3G00DBsV1dks5s5i-KgaJpZM4Qei4J> .

[brian-brazil]

In principle there shouldn't be timestamps on the data.

…

On 24 Nov 2017 04:31, "Julius Volz" ***@***.***> wrote: Without setting a custom timestamp? Or is that possible somehow meanwhile? On Nov 24, 2017 10:52 AM, "Brian Brazil" ***@***.***> wrote: I think maybe that makes the case for introducing that extra custom gatherer method to the collector.Collector interface indeed, instead of stacking one hack ontop of another? That'd be yet another hack in my opinion. The correct solution is to make the text file collector a normal collector. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/prometheus/node_exporter/pull/ 738#issuecomment-346730640>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ AAg1mKj5xObKX8Uey8xAGj3G00DBsV1dks5s5i-KgaJpZM4Qei4J> . — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#738 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGyTdoBgkpQzuBnWSbQV3duYhmkPjnW_ks5s5jiagaJpZM4Qei4J> .

[tarokkk]

Is there any info on this PR? Will it be merged or refactored? This bug totally crash the textfile collector feature. We had to rollback this release (although we patched up flags etc according to the 0.15.0 releas) on all machine because node_exporter ate up Memory and started to crash machines...

[SuperQ]

@tarokkk It's not recommended to deploy master branch unless you know what you are doing. Please use the latest official release v0.15.2 that does not contain the collector refactor that triggers this bug.

We are still working out if this is the correct change to eliminate the collection bug with the textfile collector.

[brian-brazil]

👍

[SuperQ]

Sorry for the delay, we're going to take this solution for now and think about longer term refactoring.

Thanks for fixing it.

[juliusv]

@brian-brazil Sorry, the textfile module doesn't actually set custom client-side timestamps. I remembered that wrong. I guess the reason it uses the injection hook is because it's the easiest way to go from text format -> protobuf -> text format? But do we strictly need that, or can we take the protobufs and make normal prometheus.Metric objects out of it (that then ironically get converted back into protobufs and then back into text, haha). Then we could use the normal Update() method to send the metrics.

[brian-brazil]

I guess the reason it uses the injection hook is because it's the easiest way to go from text format -> protobuf -> text format?

Yes, that's why I originally implemented it this way.

But do we strictly need that, or can we take the protobufs and make normal prometheus.Metric objects out of it (that then ironically get converted back into protobufs and then back into text, haha). Then we could use the normal Update() method to send the metrics.

That's what I'd like to do.