[plugin-aws-ec2-ebs] fix misuse of period

[lufia]

I fixed period-misuse bugs in mackerel-plugin-aws-ec2-ebs.

The plugin retrieves recent values of the metrics each minutes, and then it elects only a most recent value each metrics from them. Thus these metric values are aggregated by a minute.

In addition, some metric's graph definition such as ec2.ebs.bandwidth.#.read or ec2.ebs.throughput.#.read are designed with per-second units. Thus the plugin should convert these Sumed values to the values avarage per second, like val / 60.

mackerel-agent-plugins/mackerel-plugin-aws-ec2-ebs/lib/aws-ec2-ebs.go

Lines 230 to 243 in 1be28e2

	resp, err := p.CloudWatch.GetMetricStatistics(&cloudwatch.GetMetricStatisticsInput{
	Dimensions: []*cloudwatch.Dimension{
	{
	Name: aws.String("VolumeId"),
	Value: vol.VolumeId,
	},
	},
	StartTime: &start,
	EndTime: &now,
	MetricName: &metricName,
	Period: aws.Int64(60),
	Statistics: []*string{&statType},
	Namespace: aws.String("AWS/EBS"),
	})

However, in the current implementaton, that formula is wrong.
For instance, getLastPoint method returns a metric value and its period that are aggregated each period, but this period is metricPeriodDefault = 300 (but if not io1).

mackerel-agent-plugins/mackerel-plugin-aws-ec2-ebs/lib/aws-ec2-ebs.go

Lines 288 to 298 in 1be28e2

	val, period, err := p.getLastPoint(vol, cloudwatchdef.MetricName, cloudwatchdef.Statistics)
	if err != nil {
	retErr := errors.New(volumeID + " " + err.Error() + ":" + cloudwatchdef.MetricName)
	if err == errNoDataPoint {
	// nop
	} else {
	return nil, retErr
	}
	} else {
	stat[strings.Replace(metricKey, "#", volumeID, -1)] = cloudwatchdef.CalcFunc(val, float64(period))
	}

[lufia]

mackerel-agent-plugins/mackerel-plugin-aws-ec2-ebs/lib/aws-ec2-ebs.go

Lines 105 to 112 in 1be28e2

	"ec2.ebs.bandwidth.#": {
	Label: "EBS Bandwidth",
	Unit: "bytes/sec",
	Metrics: []mp.Metrics{
	{Name: "read", Label: "Read", Diff: false},
	{Name: "write", Label: "Write", Diff: false},
	},
	},

[lufia]

mackerel-agent-plugins/mackerel-plugin-aws-ec2-ebs/lib/aws-ec2-ebs.go

Lines 113 to 120 in 1be28e2

	"ec2.ebs.throughput.#": {
	Label: "EBS Throughput (op/s)",
	Unit: "iops",
	Metrics: []mp.Metrics{
	{Name: "read", Label: "Read", Diff: false},
	{Name: "write", Label: "Write", Diff: false},
	},
	},

[lufia]

VolumeTotalReadTime and VolumeTotalWriteTime is in seconds, but this plugin's graph defs is labeled (ms/op).

mackerel-agent-plugins/mackerel-plugin-aws-ec2-ebs/lib/aws-ec2-ebs.go

Lines 129 to 136 in 1be28e2

	"ec2.ebs.latency.#": {
	Label: "EBS Avg Latency (ms/op)",
	Unit: "float",
	Metrics: []mp.Metrics{
	{Name: "read", Label: "Read", Diff: false},
	{Name: "write", Label: "Write", Diff: false},
	},
	},

[lufia]

% Time Spent Idle Sum(VolumeIdleTime) / Period × 100

https://docs.aws.amazon.com/en_us/AWSEC2/latest/WindowsGuide/using_cloudwatch_ebs.html

[susisu]

[note] Maybe the simplest fix is returning 60 (or aggregationPeriod) here instead of period?

[susisu]

[off-topic] I'm not sure, but because the statistics is Sum, this metric value is per-aggregationPeriod, right?
If so, to output per-minute value consistently, CalcFunc should be something like val / aggregationPeriod * 60?

[lufia]

hmm...

[susisu]

I think the second argument (period) was good, because each CalcFunc could be implemented without knowing how getLastPoint works.
Is there any intention you've omitted this?

[lufia]

I'm curious is there valuable reason to specify other numbers to the period? If nothing, I think it is sufficient to refers aggregationPeriod directly in each CalcFunc.

[susisu]

It's OK!
I feel like it's a bit tightly-coupled and hard to understand (you can't tell why CalcFunc divides value by aggregationPeriod unless you read the whole program), but it will be no problem because the program itself is fairy small.

[susisu]

👍

[lufia]

Thanks for your reviewing!