server, build: add time series for disk and net IO stats #27137

vilterp · 2018-07-03T22:08:37Z

Use new library gopsutil to gather IO stats from the host kernel and save them as time series.

Caveat: these counters are for the entire host over its lifetime, not the cockroach process's lifetime. As such, the first value cockroach records may be very high, and if we plot these charts as derivatives, the derivative from zero to the first recorded data point will be much higher than subsequent derivatives, making a plot where it's hard to see changes after that.

A future commit will gather per-process IO stats, but gopsutil only provides that on Linux.

Release note (admin ui change): gather disk and network IO statistics and
save them as time series.

cockroach-teamcity · 2018-07-03T22:08:44Z

This change is

vilterp · 2018-07-16T21:05:25Z

Ready to go now; PTAL @couchand

couchand

seems generally fine, one copy-pasta bug

Reviewed 2 of 2 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/server/status/runtime.go, line 138 at r2 (raw file):

	}

	metaHostDiskReadCount = metric.Metadata{

why "host" on all of these? what host?

pkg/server/status/runtime.go, line 139 at r2 (raw file):

	metaHostDiskReadCount = metric.Metadata{
		Name:        "sys.disk.read.count.host",

suggest removing .host from all of these. if it's an important organizing detail (per above comment), maybe put it immediately after (or instead of?) sys?

pkg/server/status/runtime.go, line 141 at r2 (raw file):

		Name:        "sys.disk.read.count.host",
		Unit:        metric.Unit_COUNT,
		Measurement: "Disk read operations",

all of these Measurement fields should be pithier. suggestions:

Operations or Reads and Writes
Disk Time
Throughput
Packets
etc.

pkg/server/status/runtime.go, line 142 at r2 (raw file):

		Unit:        metric.Unit_COUNT,
		Measurement: "Disk read operations",
		Help:        "Disk read operations across all disks since this process started",

suggest removing the "since this process started"

pkg/server/status/runtime.go, line 154 at r2 (raw file):

		Unit:        metric.Unit_BYTES,
		Measurement: "Bytes read from disk",
		Help:        "Bytes read to all disks since this process started",

s/to/from/

pkg/server/status/runtime.go, line 254 at r2 (raw file):

	CPUSysNS       *metric.Gauge
	CPUSysPercent  *metric.GaugeFloat64
	// Memory stat.

it is nice to organize these... perhaps all four ...Bytes metrics should go down here?

pkg/server/status/runtime.go, line 260 at r2 (raw file):

	FDSoftLimit *metric.Gauge
	// Disk and network stats.
	// The values here are counters of the number of operations, packets, bytes, and cumulative time

This comment seems a bit out of place. It's useful information, it just seems weird to put here.

pkg/server/status/runtime.go, line 320 at r2 (raw file):

		CPUSysPercent:      metric.NewGaugeFloat64(metaCPUSysPercent),
		Rss:                metric.NewGauge(metaRSS),
		HostDiskReadTime:   metric.NewGauge(metaHostDiskReadTime),

three different places these have to be repeated and they're in three different orders! please sort consistently.

pkg/server/status/runtime.go, line 341 at r2 (raw file):

		log.Errorf(ctx, "could not get initial disk IO counters: %v", err)
	}
	rsr.initialDiskCounters = diskCounters

maybe initialize these above so you can continue to just return the struct?

pkg/server/status/runtime.go, line 424 at r2 (raw file):

	goTotal := ms.Sys - ms.HeapReleased

	if err := rsr.recordDiskStats(ctx); err != nil {

it seems a little bit odd to stick these lines here...

pkg/server/status/runtime.go, line 484 at r2 (raw file):

	rsr.HostDiskReadCount.Update(int64(summedDiskCounters.ReadCount))
	rsr.HostDiskWriteBytes.Update(int64(summedDiskCounters.WriteBytes))
	rsr.HostDiskWriteCount.Update(int64(summedDiskCounters.WriteCount))

is one missing?

pkg/server/status/runtime.go, line 567 at r2 (raw file):

	from.PacketsRecv -= sub.PacketsRecv
	from.PacketsSent -= sub.PacketsSent
}

maybe a test or two?

vilterp

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/server/status/runtime.go, line 138 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

why "host" on all of these? what host?

Done.

pkg/server/status/runtime.go, line 139 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

suggest removing .host from all of these. if it's an important organizing detail (per above comment), maybe put it immediately after (or instead of?) sys?

Done. Changed to sys.host.

pkg/server/status/runtime.go, line 141 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

all of these Measurement fields should be pithier. suggestions:

Operations or Reads and Writes

Disk Time

Throughput

Packets
etc.

Done.

pkg/server/status/runtime.go, line 142 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

suggest removing the "since this process started"

This seems pretty important to interpreting the numbers. Is it enough to just say it above in a comment instead of duplicating it in each metric?

pkg/server/status/runtime.go, line 154 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

s/to/from/

Done.

pkg/server/status/runtime.go, line 254 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

it is nice to organize these... perhaps all four ...Bytes metrics should go down here?

You're suggesting putting the disk and net IO metrics under memory? Not following.

pkg/server/status/runtime.go, line 260 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

This comment seems a bit out of place. It's useful information, it just seems weird to put here.

Wasn't sure where to put it… Moved to alongside metric metadata structs.

pkg/server/status/runtime.go, line 320 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

three different places these have to be repeated and they're in three different orders! please sort consistently.

Done.

pkg/server/status/runtime.go, line 341 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

maybe initialize these above so you can continue to just return the struct?

Done.

pkg/server/status/runtime.go, line 424 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

it seems a little bit odd to stick these lines here...

Done. Hm yeah. Moved them to the bottom.

pkg/server/status/runtime.go, line 484 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

is one missing?

Done. Yes, WriteTime was missing. Whoops!

vilterp

PTAL @couchand

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/server/status/runtime.go, line 567 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

maybe a test or two?

Done, although they are pretty silly...

mrtracy · 2018-07-19T20:20:44Z

pkg/server/status/runtime.go

@@ -280,4 +450,118 @@ func (rsr *RuntimeStatSampler) SampleEnvironment(ctx context.Context) {
 	rsr.FDSoftLimit.Update(int64(fds.SoftLimit))
 	rsr.Rss.Update(int64(mem.Resident))
 	rsr.Uptime.Update((now - rsr.startTimeNanos) / 1e9)
+
+	if err := rsr.recordDiskStats(ctx); err != nil {


Nit: these methods aren't actually recording disk stats (which typically means writing to disks), they're sampling these statistics. The word "sample" is used for other methods on this class (e.g. "SampleEnvironment"), so I would suggest using "sample" as the prefix instead to avoid confusion.

couchand

Reviewed 2 of 5 files at r3, 1 of 3 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/server/status/runtime.go, line 142 at r2 (raw file):

Previously, vilterp (Pete Vilter) wrote…

This seems pretty important to interpreting the numbers. Is it enough to just say it above in a comment instead of duplicating it in each metric?

You're right, it's good as-is.

pkg/server/status/runtime.go, line 254 at r2 (raw file):

Previously, vilterp (Pete Vilter) wrote…

You're suggesting putting the disk and net IO metrics under memory? Not following.

Sorry, no, not your new ones, I mean the four other memory stats from up above. It's only relevant here since you added the headings.

pkg/server/status/runtime.go, line 567 at r2 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Done, although they are pretty silly...

would it have caught the copy-paste bug above, though?

couchand

LGTM

Reviewable status: complete! 0 of 0 LGTMs obtained

Use new library `gopsutil` to gather IO stats from the host kernel and save them as time series. Caveat: these counters are for the entire host over its lifetime. As such, the first value cockroach records may be very high, and if we plot these charts as derivatives, the derivative from zero to the first recorded data point will be much higher than subsequent derivatives, making a plot where it's hard to see changes after that. A future commit will gather per-process IO stats, but gopsutil only provides that on Linux. Release note (admin ui change): gather disk and network IO statistics and save them as time series.

Release note: None

vilterp · 2018-07-23T14:48:02Z

TFTR! (Reviewable/GH still thinks you haven't approved… Must be that new disposition stuff…)

bors r+

reviewable

craig · 2018-07-23T15:41:22Z

Build failed (retrying...)

GitHub CI (Cockroach)

27137: server, build: add time series for disk and net IO stats r=vilterp a=vilterp Use new library `gopsutil` to gather IO stats from the host kernel and save them as time series. Caveat: these counters are for the entire host over its lifetime, not the cockroach process's lifetime. As such, the first value cockroach records may be very high, and if we plot these charts as derivatives, the derivative from zero to the first recorded data point will be much higher than subsequent derivatives, making a plot where it's hard to see changes after that. A future commit will gather per-process IO stats, but gopsutil only provides that on Linux. Release note (admin ui change): gather disk and network IO statistics and save them as time series. Co-authored-by: Pete Vilter <[email protected]>

craig · 2018-07-23T16:07:54Z

Build succeeded

GitHub CI (Cockroach)

27626: ui: add hardware metrics to new "Hardware" dashboard r=vilterp a=vilterp First two commits are from #27137; that PR will go in first. ![image](https://user-images.githubusercontent.com/7341/42787591-2166749e-8929-11e8-886b-fa89ff580d88.png) TODO - [ ] tooltips - [ ] format CPU percentage (currently 100% of one core = 1) Co-authored-by: Pete Vilter <[email protected]>

vilterp requested review from couchand and a team July 3, 2018 22:08

vilterp force-pushed the host-io-stats branch from 0ed00e5 to 0c2b57a Compare July 3, 2018 22:19

vilterp changed the title ~~server: add time series for disk and net IO stats~~ server, build: add time series for disk and net IO stats Jul 3, 2018

vilterp force-pushed the host-io-stats branch from 0c2b57a to 734b6b8 Compare July 3, 2018 23:00

vilterp changed the title ~~server, build: add time series for disk and net IO stats~~ [wip] server, build: add time series for disk and net IO stats Jul 9, 2018

vilterp added the do-not-merge bors won't merge a PR with this label. label Jul 9, 2018

vilterp force-pushed the host-io-stats branch 2 times, most recently from 26ad28b to e1c0fdd Compare July 16, 2018 21:03

vilterp changed the title ~~[wip] server, build: add time series for disk and net IO stats~~ server, build: add time series for disk and net IO stats Jul 16, 2018

vilterp removed the do-not-merge bors won't merge a PR with this label. label Jul 16, 2018

vilterp force-pushed the host-io-stats branch from e1c0fdd to 5b69c67 Compare July 16, 2018 22:04

This was referenced Jul 16, 2018

ui, server: hardware stats (tracking issue) #27085

Closed

ui: add hardware metrics to new "Hardware" dashboard #27626

Merged

[wip] server, ui: show latest disk and net IO stats in node list #27627

Closed

[WIP] ui: show IO metrics in node list #27660

Closed

vilterp requested a review from mrtracy July 18, 2018 14:52

couchand suggested changes Jul 18, 2018

View reviewed changes

vilterp force-pushed the host-io-stats branch from 5b69c67 to 9fe401b Compare July 19, 2018 18:10

vilterp commented Jul 19, 2018

View reviewed changes

vilterp force-pushed the host-io-stats branch from 29e1139 to ebeeee2 Compare July 19, 2018 19:13

vilterp commented Jul 19, 2018

View reviewed changes

mrtracy reviewed Jul 19, 2018

View reviewed changes

vilterp force-pushed the host-io-stats branch from ebeeee2 to 104805c Compare July 19, 2018 21:46

couchand previously requested changes Jul 23, 2018

View reviewed changes

couchand reviewed Jul 23, 2018

View reviewed changes

vendor: add github.com/shirou/gopsutil dep

a20b48f

Release note: None

vilterp force-pushed the host-io-stats branch from 104805c to a20b48f Compare July 23, 2018 14:47

craig bot merged commit a20b48f into cockroachdb:master Jul 23, 2018

vilterp deleted the host-io-stats branch July 23, 2018 16:11

vilterp mentioned this pull request Jul 23, 2018

server: disk IO stat gathering crashes in tests on macOS #27867

Closed

mberhault mentioned this pull request Aug 15, 2018

metrics: Monitor /proc/diskstats #10533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server, build: add time series for disk and net IO stats #27137

server, build: add time series for disk and net IO stats #27137

vilterp commented Jul 3, 2018 •

edited

Loading

cockroach-teamcity commented Jul 3, 2018

vilterp commented Jul 16, 2018

couchand left a comment

vilterp left a comment

vilterp left a comment

mrtracy Jul 19, 2018

vilterp Jul 19, 2018

couchand left a comment

couchand left a comment

vilterp commented Jul 23, 2018 •

edited

Loading

craig bot commented Jul 23, 2018

craig bot commented Jul 23, 2018

server, build: add time series for disk and net IO stats #27137

server, build: add time series for disk and net IO stats #27137

Conversation

vilterp commented Jul 3, 2018 • edited Loading

cockroach-teamcity commented Jul 3, 2018

vilterp commented Jul 16, 2018

couchand left a comment

Choose a reason for hiding this comment

vilterp left a comment

Choose a reason for hiding this comment

vilterp left a comment

Choose a reason for hiding this comment

mrtracy Jul 19, 2018

Choose a reason for hiding this comment

vilterp Jul 19, 2018

Choose a reason for hiding this comment

couchand left a comment

Choose a reason for hiding this comment

couchand left a comment

Choose a reason for hiding this comment

vilterp commented Jul 23, 2018 • edited Loading

craig bot commented Jul 23, 2018

Build failed (retrying...)

craig bot commented Jul 23, 2018

Build succeeded

vilterp commented Jul 3, 2018 •

edited

Loading

vilterp commented Jul 23, 2018 •

edited

Loading