Replaced ioutil.ReadAll with bufio.Scanner

[chicknsoup]

When /proc/net/tcp and /proc/net/tcp6 contain hundreds thousand of lines Prometheus will not be able to scrape node_exporter with tcpstat collector enabled (timeout exceeded).

Using bufio.Scanner resolves the issue.

Signed-off-by: chinhnc chicknsoupuds@gmail.com

[SuperQ]

This test should probably not be removed. We want to make sure we catch the error.

[chicknsoup]

Because these lines are replaced

		return nil, err
	}

So that test is no longer valid

[SuperQ]

The test is not obsolete, you need to check the scanner for errors.

[chicknsoup]

OK, i forgot to check scanner.Err().

[SuperQ]

This was explicitly changed away from bufio.Scanner in #1380 in order to make sure we read the file in one syscall. Using a scanner leads to poor interaction with the kernel, which allows the contents of the file to change while you read it.

If you have hundreds of thousands of entries in this file, that's going to be many megabytes of output. Is it fast enough to be practical to read this much data? Do you have any benchmarks for this?

[chicknsoup]

This was explicitly changed away from bufio.Scanner in #1380 in order to make sure we read the file in one syscall. Using a scanner leads to poor interaction with the kernel, which allows the contents of the file to change while you read it.

If you have hundreds of thousands of entries in this file, that's going to be many megabytes of output. Is it fast enough to be practical to read this much data? Do you have any benchmarks for this?

Actually the file was around 60MB (due to application's connection leak), plus the server was under heavy load. However, now it is normal again, so I cannot do any benchmark. I will try to do it when I see any of our servers in the same situation.

[SuperQ]

It sounds like this was working as intended. If the collector started to fail, it should report this via node_scrape_collector_success.

In situations where the node is failing, we don't want the exporter to contribute to the failure.

We're intentionally short-circuiting the read in order to avoid situations exactly like what you're describing.

[SuperQ]

It looks like this hasn't been converted to the new functions available in the procfs library. We should probably do that.

The procfs library uses an io.LimitReder() to gather the data. The only down side is the limit is currently 4GiB. This would be a lot of memory in situations like you describe.

[chicknsoup]

It sounds like this was working as intended. If the collector started to fail, it should report this via node_scrape_collector_success.

In situations where the node is failing, we don't want the exporter to contribute to the failure.

We're intentionally short-circuiting the read in order to avoid situations exactly like what you're describing.

In my situation tcpstat collector did not fail but took too long (exceeding scrape timeout) and the whole scraping process failed.

It looks like this hasn't been converted to the new functions available in the procfs library. We should probably do that.

The procfs library uses an io.LimitReder() to gather the data. The only down side is the limit is currently 4GiB. This would be a lot of memory in situations like you describe.

I will try with this when I have a server with high load.

[SuperQ]

In my situation tcpstat collector did not fail but took too long (exceeding scrape timeout) and the whole scraping process failed.

This is still working-as-intended for Prometheus. We can't predict all failures, so by design, we prefer to hard fail and be as noisy as possible.

[chicknsoup]

In my situation tcpstat collector did not fail but took too long (exceeding scrape timeout) and the whole scraping process failed.

This is still working-as-intended for Prometheus. We can't predict all failures, so by design, we prefer to hard fail and be as noisy as possible.

I see, so closing this PR.