Bring the textfile collector over from node_exporter #174

poblahblahblah · 2018-03-20T20:14:52Z

This adds a slightly modified textfile collector from the official
node_exporter project.

poblahblahblah · 2018-03-20T20:15:34Z

I am not sure how to test creating and using the MSI installer, so I am fairly confident that it's not correct. Any tips on how to test this locally would be much appreciated and I can follow up with a PR for the README.

poblahblahblah · 2018-03-20T20:17:11Z

collector/textfile.go

+)
+
+var (
+	textFileDirectory = flag.String("collector.textfile.directory", "C:\\Program Files\\prometheus\\textfile_inputs", "Directory to read text files with metrics from.")


Is this a sane default?

poblahblahblah · 2018-03-20T20:17:52Z

exporter.go

@@ -27,7 +27,7 @@ type WmiCollector struct {
 }

 const (
-	defaultCollectors            = "cpu,cs,logical_disk,net,os,service,system"
+	defaultCollectors            = "cpu,cs,logical_disk,net,os,service,system,textfile"


Perhaps we don't want this as a default?

carlpett

Thanks! This has been requested quite a few times, so great work implementing it.
There are a couple of things that I think needs some discussion, but in general, seems good!

carlpett · 2018-03-20T20:17:53Z

collector/textfile.go

+)
+
+var (
+	textFileDirectory = flag.String("collector.textfile.directory", "C:\\Program Files\\prometheus\\textfile_inputs", "Directory to read text files with metrics from.")


We default the installation to C:\Program Files\wmi_exporter so it would be nice to use that, instead of requiring a new directory here.

carlpett · 2018-03-20T20:19:25Z

collector/textfile.go

+	textFileDirectory = flag.String("collector.textfile.directory", "C:\\Program Files\\prometheus\\textfile_inputs", "Directory to read text files with metrics from.")
+	textFileAddOnce   sync.Once
+	mtimeDesc         = prometheus.NewDesc(
+		"node_textfile_mtime_seconds",


I don't think we should be putting metrics inside the node namespace, even ones that are supposed to be compatible? Seems like a dangerous route to go down...
That is, I think this should be wmi_... instead.

carlpett · 2018-03-20T20:21:52Z

collector/textfile.go

+				buckets, values...,
+			)
+		default:
+			panic("unknown metric type")


I know this probably comes from the node_exporter code, but should we really panic? I don't like the idea of crashing the exporter just because one of the data sources are bad.
I would prefer just logging this, possibly increasing some diagnostic metric, and then continue.

carlpett · 2018-03-20T20:22:42Z

collector/textfile.go

+	// Export if there were errors.
+	ch <- prometheus.MustNewConstMetric(
+		prometheus.NewDesc(
+			"node_textfile_scrape_error",


As mentioned previously, I don't think this should be in the node namespace.

carlpett · 2018-03-20T20:23:37Z

collector/textfile_test.go

+
+		// Suppress a log message about `nonexistent_path` not existing, this is
+		// expected and clutters the test output.
+		log.AddFlags(kingpin.CommandLine)


We haven't switched the wmi_exporter to kingpin for flags yet, so this probably doesn't make sense

carlpett · 2018-03-20T20:25:12Z

collector/textfile_test.go

+		path string
+		out  string
+	}{
+		{


I believe these files need to exist for the tests to pass?

carlpett · 2018-03-20T20:25:59Z

README.md

@@ -22,6 +22,7 @@ process | [Win32_PerfRawData_PerfProc_Process](https://msdn.microsoft.com/en-us/
 service | [Win32_Service](https://msdn.microsoft.com/en-us/library/aa394418(v=vs.85).aspx) metrics (service states) | &#10003;
 system | Win32_PerfRawData_PerfOS_System metrics (system calls) | &#10003;
 tcp | [Win32_PerfRawData_Tcpip_TCPv4](https://msdn.microsoft.com/en-us/library/aa394341(v=vs.85).aspx) metrics (tcp connections) |
+textfile | Read prometheus metrics from a text file |


Since the collector is added to the defaults further down, it should be marked as such here as well (or not be enabled by default)

carlpett · 2018-03-20T20:26:34Z

README.md

@@ -39,6 +40,7 @@ Name | Description
 `LISTEN_ADDR` | The IP address to bind to. Defaults to 0.0.0.0
 `LISTEN_PORT` | The port to bind to. Defaults to 9182.
 `METRICS_PATH` | The path at which to serve metrics. Defaults to `/metrics`
+`TEXTFILE_DIR` | As the `--collector.textfile.directory` flag, provide a directory to read text files with metrics from


Currently should just be one -, ie -collector.textfile.directory, since we haven't switched to Kingpin yet

carlpett · 2018-03-20T20:30:02Z

There's a compilation failure in the tests, see here: https://ci.appveyor.com/project/martinlindhe/wmi-exporter/build/402/job/2l6kquvy0c8s6ln7.

carlpett · 2018-03-20T21:50:19Z

Regarding the MSI, assuming you have compiled the exporter already, you can build it by running ./installer/build.ps1 -PathToExecutable ./wmi_exporter.exe -Version 0.2.99. This will build an msi that goes into ./installer/Output/, and you can test it from there. It should be fairly well-behaved, so just uninstall as usual if something isn't working, and try again.

poblahblahblah · 2018-03-20T22:06:03Z

It looks like wmi_exporter and node_exporter are more different than I had assumed when I just cargo culted this over, so the tests were fairly incompatible with wmi_exporter. This is not ideal, but for now I've removed the tests.

poblahblahblah · 2018-03-20T22:26:34Z

The MSI that gets built doesn't work - will troubleshoot that.

poblahblahblah · 2018-03-20T22:41:34Z

I guess the good news is that even if I build the installer from master it still doesn't work, so I am guessing this is a local issue. I'll try to find another wandows computer to test on.

poblahblahblah · 2018-03-21T21:26:25Z

@carlpett it looks like -log.format is not a valid argument for wmi_exporter.exe, which is why my MSI is failing to install.

Here is what the wmi_exporter.wxs file configures the service to use:

"C:\Program Files\wmi_exporter\wmi_exporter.exe" -log.format logger:eventlog?name=wmi_exporter

Here's what happens when I try that command line flag against the compiled exe:

PS C:\Users\Patrick O'Brien\src\go\src\github.com\martinlindhe\wmi_exporter> ./wmi_exporter.exe -log.format logger:eventlog?name=wmi_exporter
2018/03/21 14:26:06 warning: Couldn't open registry to determine IIS version: The system cannot find the file specified.
flag provided but not defined: -log

Any guidance here?

martinlindhe · 2018-03-21T22:19:05Z

@poblahblahblah due to powershell parsing arguments with dots in them, you need to specify them like this:

"-log.format"

More info in #96

poblahblahblah · 2018-03-21T22:57:31Z

@martinlindhe I guess my point is I didn't change anything with the wxs file so it looks like any time i build an MSI with that wxs file the MSI installer fails. Are you building the official packages with a different wxs file? I am pretty confused

pete-leese · 2018-03-21T23:02:20Z

This is an interesting feature!

Do you have any examples of where this could be used in a Windows world? Cheers Pete

carlpett · 2018-03-22T07:06:58Z

@poblahblahblah So sorry for missing this! Due to #169, current master doesn't build working msi:s. I've been pretty busy the last few days, but I'll see if I can fix it today.
If you just want to test your msi, you could make a temporary change which removes the logging flags in the .wxs
Again, apologies for wasting your time on this!

poblahblahblah · 2018-03-22T17:25:11Z

@carlpett no worries!

I've updated the wxs file to work correctly. Everything works and my change looks good.

poblahblahblah · 2018-03-28T21:25:19Z

@carlpett Any chance this can get a re-review?

carlpett · 2018-03-29T06:52:27Z

@poblahblahblah Yes, of course!
I did a fix for the log-flags in #112, which will also affect this PR a bit (changing the flag declaration syntax), so I would like to merge that first. Sounds ok?

carlpett · 2018-03-30T07:29:30Z

collector/textfile.go

+
+var (
+	textFileDirectory = flag.String("collector.textfile.directory", "C:\\Program Files\\wmi_exporter\\textfile_inputs", "Directory to read text files with metrics from.")
+	textFileAddOnce   sync.Once


This doesn't seem to be used?

carlpett · 2018-03-30T07:30:14Z

@poblahblahblah The flags PR is now merged! You'll need to

Change the definition of your flag to use kingpin (and add another - to the flag in the README)
Revert the log-flag removal and update with the new flag format in the wxs

I found an (I think) unused variable in my last pass over this, but apart from that, looks good!

This adds a slightly modified textfile collector from the official node_exporter project.

poblahblahblah · 2018-04-03T18:12:36Z

@carlpett ok - I think this is ready to go. Tested locally and everything worked like expected.

edit:

To address your comment about the unused variable - that is there to ensure that the textfile gatherer should only be added to the to the gatherer list once. See prometheus/node_exporter#738

carlpett · 2018-04-03T18:24:51Z

Great!
Regarding the variable - yes, that is the idea, but the piece of code which only adds it once doesn't seem to have come along with the translation? Also, I don't think we have the feature which made node_exporter need that part anyway?

poblahblahblah · 2018-04-03T18:46:46Z

Ah - you are correct. Fix incoming.

carlpett · 2018-04-03T19:36:48Z

LGTM!

carlpett · 2018-04-03T19:37:23Z

Thanks @poblahblahblah!

…unity#174) Bring the textfile collector over from node_exporter This adds a slightly modified textfile collector from the official node_exporter project.

poblahblahblah commented Mar 20, 2018

View reviewed changes

carlpett requested changes Mar 20, 2018

View reviewed changes

poblahblahblah force-pushed the add-textfile-collector branch from a6975ea to d537631 Compare March 20, 2018 20:59

poblahblahblah force-pushed the add-textfile-collector branch from 56def6c to 23ca54b Compare March 20, 2018 22:01

poblahblahblah force-pushed the add-textfile-collector branch from 23ca54b to b9607e6 Compare March 22, 2018 17:24

poblahblahblah force-pushed the add-textfile-collector branch 2 times, most recently from 5956389 to 8c66c64 Compare March 22, 2018 17:32

carlpett mentioned this pull request Mar 29, 2018

Use Kingpin for flags #112

Merged

carlpett reviewed Mar 30, 2018

View reviewed changes

poblahblahblah added 2 commits April 3, 2018 10:46

Bring the textfile collector over from node_exporter

92f783f

This adds a slightly modified textfile collector from the official node_exporter project.

wxs file fixes

ec739ea

poblahblahblah force-pushed the add-textfile-collector branch from 8c66c64 to aea89f1 Compare April 3, 2018 17:52

converting to kingpin

80002f3

poblahblahblah force-pushed the add-textfile-collector branch from aea89f1 to 80002f3 Compare April 3, 2018 17:59

removing unused variable

51470bf

poblahblahblah force-pushed the add-textfile-collector branch from cf5f1b6 to 51470bf Compare April 3, 2018 18:55

carlpett approved these changes Apr 3, 2018

View reviewed changes

carlpett merged commit cf79239 into prometheus-community:master Apr 3, 2018

mbibinski-ttd mentioned this pull request Apr 4, 2018

Release Request (to include Pull Request #174) #182

Closed

carlpett mentioned this pull request Apr 6, 2018

[featurerequest] file exporter #33

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring the textfile collector over from node_exporter #174

Bring the textfile collector over from node_exporter #174

poblahblahblah commented Mar 20, 2018

poblahblahblah commented Mar 20, 2018

poblahblahblah Mar 20, 2018

poblahblahblah Mar 20, 2018

carlpett left a comment

carlpett Mar 20, 2018

carlpett Mar 20, 2018

carlpett Mar 20, 2018

carlpett Mar 20, 2018

carlpett Mar 20, 2018

carlpett Mar 20, 2018

carlpett Mar 20, 2018

carlpett Mar 20, 2018

carlpett commented Mar 20, 2018

carlpett commented Mar 20, 2018

poblahblahblah commented Mar 20, 2018

poblahblahblah commented Mar 20, 2018

poblahblahblah commented Mar 20, 2018

poblahblahblah commented Mar 21, 2018

martinlindhe commented Mar 21, 2018 •

edited

Loading

poblahblahblah commented Mar 21, 2018

pete-leese commented Mar 21, 2018

carlpett commented Mar 22, 2018

poblahblahblah commented Mar 22, 2018

poblahblahblah commented Mar 28, 2018

carlpett commented Mar 29, 2018

carlpett Mar 30, 2018

carlpett commented Mar 30, 2018

poblahblahblah commented Apr 3, 2018 •

edited

Loading

carlpett commented Apr 3, 2018

poblahblahblah commented Apr 3, 2018

carlpett commented Apr 3, 2018

carlpett commented Apr 3, 2018

Bring the textfile collector over from node_exporter #174

Bring the textfile collector over from node_exporter #174

Conversation

poblahblahblah commented Mar 20, 2018

poblahblahblah commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlpett 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

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

carlpett commented Mar 20, 2018

carlpett commented Mar 20, 2018

poblahblahblah commented Mar 20, 2018

poblahblahblah commented Mar 20, 2018

poblahblahblah commented Mar 20, 2018

poblahblahblah commented Mar 21, 2018

martinlindhe commented Mar 21, 2018 • edited Loading

poblahblahblah commented Mar 21, 2018

pete-leese commented Mar 21, 2018

carlpett commented Mar 22, 2018

poblahblahblah commented Mar 22, 2018

poblahblahblah commented Mar 28, 2018

carlpett commented Mar 29, 2018

Choose a reason for hiding this comment

carlpett commented Mar 30, 2018

poblahblahblah commented Apr 3, 2018 • edited Loading

carlpett commented Apr 3, 2018

poblahblahblah commented Apr 3, 2018

carlpett commented Apr 3, 2018

carlpett commented Apr 3, 2018

martinlindhe commented Mar 21, 2018 •

edited

Loading

poblahblahblah commented Apr 3, 2018 •

edited

Loading