Expose NVMe Volume Metrics

[torredil]

Is this a bug fix or adding new feature?

New feature.

What is this PR about? / Why do we need it?

This PR adds support for collecting detailed performance statistics for CSI managed devices. The log page for each device is retrieved at gather time, transformed into a prometheus-compatible format, and exposed at a /metrics HTTP endpoint.

What testing is done?

make verify && make test

ginkgo run --focus='should create a stateful pod and read NVMe metrics from node plugin' -v
Test Suite Passed

curl 127.0.0.1:3302/metrics

...
# HELP volume_queue_length Current volume queue length
# TYPE volume_queue_length gauge
volume_queue_length{instance_id="i-0725b88fb342cd1c9",volume_id="vol-0aa37802a7c970c8e"} 0
# HELP write_io_latency_histogram Histogram of write I/O latencies (in microseconds)
# TYPE write_io_latency_histogram histogram
write_io_latency_histogram_bucket{instance_id="i-0725b88fb342cd1c9",volume_id="vol-0aa37802a7c970c8e",le="1"} 0
write_io_latency_histogram_bucket{instance_id="i-0725b88fb342cd1c9",volume_id="vol-0aa37802a7c970c8e",le="2"} 0
write_io_latency_histogram_bucket{instance_id="i-0725b88fb342cd1c9",volume_id="vol-0aa37802a7c970c8e",le="4"} 0
write_io_latency_histogram_bucket{instance_id="i-0725b88fb342cd1c9",volume_id="vol-0aa37802a7c970c8e",le="8"} 0
write_io_latency_histogram_bucket{instance_id="i-0725b88fb342cd1c9",volume_id="vol-0aa37802a7c970c8e",le="16"} 0
write_io_latency_histogram_bucket{instance_id="i-0725b88fb342cd1c9",volume_id="vol-0aa37802a7c970c8e",le="32"} 0
write_io_latency_histogram_bucket{instance_id="i-0725b88fb342cd1c9",volume_id="vol-0aa37802a7c970c8e",le="64"} 0
write_io_latency_histogram_bucket{instance_id="i-0725b88fb342cd1c9",volume_id="vol-0aa37802a7c970c8e",le="128"} 0
write_io_latency_histogram_bucket{instance_id="i-0725b88fb342cd1c9",volume_id="vol-0aa37802a7c970c8e",le="256"} 58
write_io_latency_histogram_bucket{instance_id="i-0725b88fb342cd1c9",volume_id="vol-0aa37802a7c970c8e",le="512"} 14260
write_io_latency_histogram_bucket{instance_id="i-0725b88fb342cd1c9",volume_id="vol-0aa37802a7c970c8e",le="1024"} 83115
...

[github-actions]

Code Coverage Diff

File	Old Coverage	New Coverage	Delta
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/metrics/metrics.go	66.7%	63.8%	-2.9
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/metrics/nvme.go	Does not exist	21.7%

[ConnorJC3]

/lgtm

[AndrewSirenko]

Looks great, left a few non-blocking unit test comments/questions.

[AndrewSirenko]

/hold

For @ElijahQuinones's review

[ConnorJC3]

/lgtm

[torredil]

/label tide/merge-method-squash

[AndrewSirenko]

/lgtm

[ElijahQuinones]

/unhold
/approve

LGTM thank you for also adding the service in this PR :) !

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AndrewSirenko, ElijahQuinones

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [AndrewSirenko,ElijahQuinones]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[SuperQ]

Hey, thanks for adding these metrics.

Unfortunately, there are a few things about this change that don't follow Prometheus best practices.

https://prometheus.io/docs/practices/naming/

Would you be willing to accept changes to adapt the data to meet best practices?

[torredil]

Hi @SuperQ, thank you for reaching out about this and engaging with the community on Prometheus best practices.

We are aware of that documentation and its one of the things our team carefully evaluated when implementing this feature. While we generally strive to follow best practices, in this specific case we had a requirement and intentionally decided to maintain consistency with the underlying EBS metrics output to allow for direct mapping with the AWS documentation and CloudWatch metrics.

That said, could you share an example of the change you'd propose? I'd like to discuss this with the team if there is an opportunity to incorporate best practices for naming conventions while maintaining the consistency requirement.

[SuperQ]

There's basically no way to make the underlying metric match best practices without breaking changes.

My changes would be:

• Prefix the metrics with aws_ebs_csi... to namespace the metrics as specific to this driver.
• Map the unit from microseconds to seconds to match unit best practices.
• Rename the metric to match best practices.

I also reviewed the code and found that the upstream data is missing the histogram cumulative sum data. ☹️

[torredil]

That makes sense @SuperQ, I'll bring up this feedback for discussion during our next weekly sync and provide a response here shortly after. We also need to consider existing metrics exported by the controller plugin when adding/changing prefixes if the team decides to move forward with the proposal.