InfluxDB Storage driver sends network stats of only the first interface found

[cirocosta]

Hey,

I was comparing some metrics from both influxdb and prometheus and it looked weird to me that influx wasn't reporting per-interface statistics.

cadvisor/storage/influxdb/influxdb.go

Lines 206 to 209 in 2835c35

	points = append(points, makePoint(serRxBytes, stats.Network.RxBytes))
	points = append(points, makePoint(serRxErrors, stats.Network.RxErrors))
	points = append(points, makePoint(serTxBytes, stats.Network.TxBytes))
	points = append(points, makePoint(serTxErrors, stats.Network.TxErrors))

This seems wrong as that data is coming from a single iface:

cadvisor/container/libcontainer/helpers.go

Lines 138 to 141 in c2f8191

	// For backwards compatibility.
	if len(stats.Network.Interfaces) > 0 {
	stats.Network.InterfaceStats = stats.Network.Interfaces[0]
	}

Am I missing something?

If that's really incorrect, how should we approach this? Should we add a new metric like rx_bytes_per_iface to not change expectations of those that already rely on rx_bytes or do you think it's ok to just add an extra tag on rx_bytes with the interface field (with values for each interface)?

I like the second case (_per_iface) as people that were relying on the old behavior could find things a bit off when updating (suddenly there's loopback and other ifaces being counted).

Thanks!

[cirocosta]

Note.: this can probably be expanded to other storage drivers - I just looked at influxdb for now.

[dashpole]

I believe we currently only support a single network interface per container.

[cirocosta]

hmmm in the collection you mean? Under libcontainer/helpers.go it fetches all of them from /proc/<pid>/net/dev:

cadvisor/container/libcontainer/helpers.go

Lines 146 to 155 in c2f8191

	func networkStatsFromProc(rootFs string, pid int) ([]info.InterfaceStats, error) {
	netStatsFile := path.Join(rootFs, "proc", strconv.Itoa(pid), "/net/dev")
	ifaceStats, err := scanInterfaceStats(netStatsFile)
	if err != nil {
	return []info.InterfaceStats{}, fmt.Errorf("couldn't read network stats: %v", err)
	}
	return ifaceStats, nil
	}

I was willing to then do something like how it's done for cpu:

cadvisor/storage/influxdb/influxdb.go

Lines 187 to 194 in 2835c35

	// CPU usage per CPU
	for i := 0; i < len(stats.Cpu.Usage.PerCpu); i++ {
	point := makePoint(serCpuUsagePerCpu, stats.Cpu.Usage.PerCpu[i])
	tags := map[string]string{"instance": fmt.Sprintf("%v", i)}
	addTagsToPoint(point, tags)
	points = append(points, point)
	}

[dashpole]

Sure, the real question here is what API changes you are suggesting. How do we plan to represent this in the API in a way that is backwards compatible?

[cirocosta]

Got it, at first I thought about just adding new 4 fields:

	serRxBytes string = "rx_bytes"
	// Cumulative count of receive errors encountered.
	serRxErrors string = "rx_errors"
	// Cumulative count of bytes transmitted.
	serTxBytes string = "tx_bytes"
	// Cumulative count of transmit errors encountered.
	serTxErrors string = "tx_errors"

	serRxBytesPerIface string = "rx_bytes_per_iface"
	// Cumulative count of receive errors encountered.
	serRxErrorsPerIface string = "rx_errors_per_iface"
	// Cumulative count of bytes transmitted.
	serTxBytesPerIface string = "tx_bytes_per_iface"
	// Cumulative count of transmit errors encountered.
	serTxErrorsPerIface string = "tx_errors_per_iface"

but that caries the problem of having 4 fields (rx_bytes, rx_errors, tx_bytes, tx_errors) being transmitted only for backwards compatibility (someone using *_per_iface would certainly not be using) - this is something that already goes on for those that are using straight serialization of containerStats (as the fields are included there.

A second option that I see is introducing a flag that enables the new behavior (-collect_network_stats_per_interface?) but ... yet another flag that would be affecting only some storage drivers - those that don't serialize and are not prometheus: redis, statsd, stdout and influxDb.

For my use case the second option (flag) would be fine even though it's yet another one.

Wdyt?

thx!

[cirocosta]

Hey,

another idea is to add a flag that triggers the behavior of aggregating the values reported for each stat:

--influx-db-aggregate-network-stats

which would imply on (having an if aggregateNetworkStats...)

 	if len(stats.Network.Interfaces) > 0 {
-		stats.Network.InterfaceStats = stats.Network.Interfaces[0]
+		for _, iface := range stats.Network.Interfaces {
+			stats.Network.Name = "all"
+
+			stats.Network.TxBytes += iface.TxBytes
+			stats.Network.TxErrors += iface.TxErrors
+			stats.Network.TxDropped += iface.TxDropped
+			stats.Network.TxPackets += iface.TxPackets
+
+			stats.Network.RxBytes += iface.RxBytes
+			stats.Network.RxErrors += iface.RxErrors
+			stats.Network.RxDropped += iface.RxDropped
+			stats.Network.RxPackets += iface.RxPackets
+		}
 	}

Wdyt? Those relying on the old behavior (stats from the first iface found) would still have what they expect and we can move forward with a value that includes all interfaces.

Maybe an extra flag would emit those _per_iface metrics to influx.

ps.: for my use-case the first solution is enough as we don't segregate the interfaces and counting loopback is valid but maybe 2 is also useful for others.

Thx!