Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from Jul 29, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8d5513b
[plugin-aws-ec2-ebs] use array style syntax
lufia Jul 27, 2022
b2133a5
[plugin-aws-ec2-ebs] fix misuse of denominator
lufia Jul 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion mackerel-plugin-aws-ec2-ebs/README.md
View file
Open in desktop
Expand Up @@ -20,5 +20,5 @@ the credential provided manually or fetched automatically with IAM Role, should

```
[plugin.metrics.aws-ec2_ebs]
command = "/path/to/mackerel-plugin-aws-ec2-ebs"
command = ["/path/to/mackerel-plugin-aws-ec2-ebs"]
```
71 changes: 45 additions & 26 deletions mackerel-plugin-aws-ec2-ebs/lib/aws-ec2-ebs.go
View file
Open in desktop
Expand Up @@ -16,7 +16,11 @@ import (
mp "github.com/mackerelio/go-mackerel-plugin-helper"
)

var metricPeriodDefault = 300
const (
metricPeriodDefault = 300
aggregationPeriod = 60
)

var metricPeriodByVolumeType = map[string]int{
"io1": 60,
}
Expand All @@ -42,62 +46,74 @@ var io1Graphs = append([]string{
type cloudWatchSetting struct {
MetricName string
Statistics string
CalcFunc func(float64, float64) float64
CalcFunc func(float64) float64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

func value(val float64) float64 {
return val
}

func valuePerSec(val float64) float64 {
return val / aggregationPeriod
}

func sec2msec(val float64) float64 {
return val * 1000
}

// http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/monitoring-volume-status.html
var cloudwatchdefs = map[string](cloudWatchSetting){
"ec2.ebs.bandwidth.#.read": cloudWatchSetting{
MetricName: "VolumeReadBytes", Statistics: "Sum",
CalcFunc: func(val float64, period float64) float64 { return val / period },
CalcFunc: valuePerSec,
},
"ec2.ebs.bandwidth.#.write": cloudWatchSetting{
MetricName: "VolumeWriteBytes", Statistics: "Sum",
CalcFunc: func(val float64, period float64) float64 { return val / period },
CalcFunc: valuePerSec,
},
Comment on lines 66 to 73
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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},
},
},

"ec2.ebs.throughput.#.read": cloudWatchSetting{
MetricName: "VolumeReadOps", Statistics: "Sum",
CalcFunc: func(val float64, period float64) float64 { return val / period },
CalcFunc: valuePerSec,
},
"ec2.ebs.throughput.#.write": cloudWatchSetting{
MetricName: "VolumeWriteOps", Statistics: "Sum",
CalcFunc: func(val float64, period float64) float64 { return val / period },
CalcFunc: valuePerSec,
},
Comment on lines 74 to 81
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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},
},
},

"ec2.ebs.size_per_op.#.read": cloudWatchSetting{
MetricName: "VolumeReadBytes", Statistics: "Average",
CalcFunc: func(val float64, period float64) float64 { return val },
CalcFunc: value,
},
"ec2.ebs.size_per_op.#.write": cloudWatchSetting{
MetricName: "VolumeWriteBytes", Statistics: "Average",
CalcFunc: func(val float64, period float64) float64 { return val },
CalcFunc: value,
},
"ec2.ebs.latency.#.read": cloudWatchSetting{
MetricName: "VolumeTotalReadTime", Statistics: "Average",
CalcFunc: func(val float64, period float64) float64 { return val * 1000 },
CalcFunc: sec2msec,
},
"ec2.ebs.latency.#.write": cloudWatchSetting{
MetricName: "VolumeTotalWriteTime", Statistics: "Average",
CalcFunc: func(val float64, period float64) float64 { return val * 1000 },
CalcFunc: sec2msec,
},
Comment on lines 90 to 97
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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},
},
},

"ec2.ebs.queue_length.#.queue_length": cloudWatchSetting{
MetricName: "VolumeQueueLength", Statistics: "Average",
CalcFunc: func(val float64, period float64) float64 { return val },
CalcFunc: value,
},
"ec2.ebs.idle_time.#.idle_time": cloudWatchSetting{
MetricName: "VolumeIdleTime", Statistics: "Sum",
CalcFunc: func(val float64, period float64) float64 { return val / period * 100 },
CalcFunc: func(val float64) float64 { return val / aggregationPeriod * 100.0 },
},
Comment on lines 102 to 105
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

"ec2.ebs.throughput_delivered.#.throughput_delivered": cloudWatchSetting{
MetricName: "VolumeThroughputPercentage", Statistics: "Average",
CalcFunc: func(val float64, period float64) float64 { return val },
CalcFunc: value,
},
"ec2.ebs.consumed_ops.#.consumed_ops": cloudWatchSetting{
MetricName: "VolumeConsumedReadWriteOps", Statistics: "Sum",
CalcFunc: func(val float64, period float64) float64 { return val },
CalcFunc: value,
Copy link
Contributor

@susisu susisu Jul 28, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm...

},
"ec2.ebs.burst_balance.#.burst_balance": cloudWatchSetting{
MetricName: "BurstBalance", Statistics: "Average",
CalcFunc: func(val float64, period float64) float64 { return val },
CalcFunc: value,
},
}

Expand Down Expand Up @@ -175,14 +191,17 @@ var stderrLogger *log.Logger

// EBSPlugin mackerel plugin for ebs
type EBSPlugin struct {
// command line options
Region string
AccessKeyID string
SecretAccessKey string
InstanceID string
Credentials *credentials.Credentials
EC2 *ec2.EC2
CloudWatch *cloudwatch.CloudWatch
Volumes []*ec2.Volume

// internal states
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Credentials *credentials.Credentials
EC2 *ec2.EC2
CloudWatch *cloudwatch.CloudWatch
Volumes []*ec2.Volume
}

func (p *EBSPlugin) prepare() error {
Expand Down Expand Up @@ -218,7 +237,7 @@ func (p *EBSPlugin) prepare() error {

var errNoDataPoint = errors.New("fetched no datapoints")

func (p EBSPlugin) getLastPoint(vol *ec2.Volume, metricName string, statType string) (float64, int, error) {
func (p EBSPlugin) getLastPoint(vol *ec2.Volume, metricName string, statType string) (float64, error) {
now := time.Now()

period := metricPeriodDefault
Expand All @@ -237,17 +256,17 @@ func (p EBSPlugin) getLastPoint(vol *ec2.Volume, metricName string, statType str
StartTime: &start,
EndTime: &now,
MetricName: &metricName,
Period: aws.Int64(60),
Period: aws.Int64(aggregationPeriod),
Statistics: []*string{&statType},
Namespace: aws.String("AWS/EBS"),
})
if err != nil {
return 0, 0, err
return 0, err
}

datapoints := resp.Datapoints
if len(datapoints) == 0 {
return 0, 0, errNoDataPoint
return 0, errNoDataPoint
}

latest := time.Unix(0, 0)
Expand All @@ -266,7 +285,7 @@ func (p EBSPlugin) getLastPoint(vol *ec2.Volume, metricName string, statType str
}
}

return latestVal, period, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return latestVal, nil
}

// FetchMetrics fetch the metrics
Expand All @@ -285,7 +304,7 @@ func (p EBSPlugin) FetchMetrics() (map[string]interface{}, error) {
for _, metric := range graphdef[graphName].Metrics {
metricKey := graphName + "." + metric.Name
cloudwatchdef := cloudwatchdefs[metricKey]
val, period, err := p.getLastPoint(vol, cloudwatchdef.MetricName, cloudwatchdef.Statistics)
val, err := p.getLastPoint(vol, cloudwatchdef.MetricName, cloudwatchdef.Statistics)
if err != nil {
retErr := errors.New(volumeID + " " + err.Error() + ":" + cloudwatchdef.MetricName)
if err == errNoDataPoint {
Expand All @@ -294,7 +313,7 @@ func (p EBSPlugin) FetchMetrics() (map[string]interface{}, error) {
return nil, retErr
}
} else {
stat[strings.Replace(metricKey, "#", volumeID, -1)] = cloudwatchdef.CalcFunc(val, float64(period))
stat[strings.Replace(metricKey, "#", volumeID, -1)] = cloudwatchdef.CalcFunc(val)
}
}
}
Expand Down