Add new awsebsnvmereceiver #1603

duhminick · 2025-03-18T14:54:55Z

Note to Reviewers

This PR does not enable this new receiver. This is strictly just adding a new receiver that will be enabled later.

Changes 2:

Use fmt.Sscanf instead of the custom parsing logic for the device file name
Enable only one metric by default. This is for the upcoming translation changes -- this just simplifies some of the logic. We need to keep at least one metric enabled by default or else the generated tests from mdatagen fail
Rename Resources to Devices in the receiver config

Changes 3:

Refactor to use collections.Set instead of custom map implementation for device tracking
Remove unnecessary work in device processing pipeline by skipping EBS devices and serial retrieval if it's already been retrieved
Remove cleanupString function to simplify string handling
Rename DeviceInfoProvider for better code organization
Don't unnecessarily export constants used only within the package
Rename stub implementation to notunix for better platform distinction

Description of the issue

Elastic Block Storage (EBS) exposes performance statistics for EBS volumes attached to EC2 instances as NVMe devices in a vendor unique log page. The log page can be retrieved by making a system call to the NVMe device. CloudWatch Agent (CWA) is going to collect the retrieved metrics and emit them to CloudWatch.

Description of changes

Main Scraper Implementation (scraper.go):
- Implements the core scraping logic through the nvmeScraper struct
- Uses a MetricsBuilder to construct standardized metrics
  - MetricsBuilder is generated using mdatagen with the schema defined in metadata.yaml
- Handles device discovery and metric collection for EBS NVMe devices
  - Includes the ability to scrape all devices, or specific devices.
- Implements safety checks for integer overflow protection
NVMe Metrics Collection (internal/nvme):
- Implements low-level NVMe device interaction through Linux ioctl calls
- Provides structured metric collection through EBSMetrics struct
- Collects key metrics including:
  - Read/Write operations and bytes
  - Total read/write times
  - IOPS and throughput exceeded counters
  - Queue length
  - Read/Write latency histograms (though this is not being collected atm)
Generated components (internal/metadata);

All generated by mdatagen using the schema defined in metadata.yaml

The receiver collects the following metrics from EBS NVMe devices:

diskio_ebs_total_read_ops
diskio_ebs_total_write_ops
diskio_ebs_total_read_bytes
diskio_ebs_total_write_bytes
diskio_ebs_total_read_time
diskio_ebs_total_write_time
diskio_ebs_volume_performance_exceeded_iops
diskio_ebs_volume_performance_exceeded_tp
diskio_ebs_ec2_instance_performance_exceeded_iops
diskio_ebs_ec2_instance_performance_exceeded_tp
diskio_ebs_volume_queue_length

License

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Tests

Manually updated the YAML config to have a new instance of the EBS receiver as well as update the existing host delta metrics pipeline to have the new receiver.

Attached an EBS volume to the instance

The EC2 instance that the manual tests were ran on have two EBS volumes attached (nvme0 and nvme1)

Resource is explicitly empty

2025-03-18T19:35:03Z D! {"caller":"awsebsnvmereceiver/scraper.go:54","msg":"Began scraping for NVMe metrics","kind":"receiver","name":"awsebsnvmereceiver","data_type":"metrics"}
2025-03-18T19:35:03Z D! {"caller":"awsebsnvmereceiver/scraper.go:133","msg":"skipping un-allowed device","kind":"receiver","name":"awsebsnvmereceiver","data_type":"metrics","device":"nvme0"}
2025-03-18T19:35:03Z D! {"caller":"awsebsnvmereceiver/scraper.go:133","msg":"skipping un-allowed device","kind":"receiver","name":"awsebsnvmereceiver","data_type":"metrics","device":"nvme0n1"}

One device (nvme0) is in resources

2025-03-18T19:36:24Z D! {"caller":"awsebsnvmereceiver/scraper.go:54","msg":"Began scraping for NVMe metrics","kind":"receiver","name":"awsebsnvmereceiver","data_type":"metrics"}
2025-03-18T19:36:24Z D! {"caller":"awsebsnvmereceiver/scraper.go:133","msg":"skipping un-allowed device","kind":"receiver","name":"awsebsnvmereceiver","data_type":"metrics","device":"nvme0n1"}
2025-03-18T19:36:24Z D! {"caller":"awsebsnvmereceiver/scraper.go:133","msg":"skipping un-allowed device","kind":"receiver","name":"awsebsnvmereceiver","data_type":"metrics","device":"nvme0n1p1"}
2025-03-18T19:36:24Z D! {"caller":"awsebsnvmereceiver/scraper.go:101","msg":"emitted metrics for nvme device with controller id","kind":"receiver","name":"awsebsnvmereceiver","data_type":"metrics","controllerID":0}

`*` for resources

2025-03-18T19:38:30Z D! {"caller":"awsebsnvmereceiver/scraper.go:54","msg":"Began scraping for NVMe metrics","kind":"receiver","name":"awsebsnvmereceiver","data_type":"metrics"}
2025-03-18T19:38:30Z D! {"caller":"awsebsnvmereceiver/scraper.go:101","msg":"emitted metrics for nvme device with controller id","kind":"receiver","name":"awsebsnvmereceiver","data_type":"metrics","controllerID":0}
2025-03-18T19:38:30Z D! {"caller":"awsebsnvmereceiver/scraper.go:101","msg":"emitted metrics for nvme device with controller id","kind":"receiver","name":"awsebsnvmereceiver","data_type":"metrics","controllerID":1}

Sample Config

receivers:
    awsebsnvmereceiver:
        collection_interval: 1m0s
        initial_delay: 1s
        metrics:
            diskio_ebs_ec2_instance_performance_exceeded_iops:
                enabled: false
            diskio_ebs_ec2_instance_performance_exceeded_tp:
                enabled: false
            diskio_ebs_total_read_bytes:
                enabled: true
            diskio_ebs_total_read_ops:
                enabled: false
            diskio_ebs_total_read_time:
                enabled: false
            diskio_ebs_total_write_bytes:
                enabled: true
            diskio_ebs_total_write_ops:
                enabled: false
            diskio_ebs_total_write_time:
                enabled: false
            diskio_ebs_volume_performance_exceeded_iops:
                enabled: false
            diskio_ebs_volume_performance_exceeded_tp:
                enabled: false
            diskio_ebs_volume_queue_length:
                enabled: false
        resource_attributes:
            VolumeId:
                enabled: true
        devices:
            - nvme0n1
        timeout: 0s

Requirements

Before commit the code, please do the following steps.

Run make fmt and make fmt-sh
Run make lint

receiver/awsebsnvmereceiver/config.go

receiver/awsebsnvmereceiver/internal/metadata/generated_config.go

receiver/awsebsnvmereceiver/internal/metadata/testdata/config.yaml

receiver/awsebsnvmereceiver/internal/nvme/device_file_attributes.go

receiver/awsebsnvmereceiver/internal/nvme/ebs_metrics.go

receiver/awsebsnvmereceiver/scraper.go

lisguo · 2025-03-25T13:53:01Z

"This PR should be merged into the ebs branch"

Is this still true? We are just adding the receiver not enabling it so putting it to main seems fine

duhminick · 2025-03-25T14:59:02Z

Is this still true? We are just adding the receiver not enabling it so putting it to main seems fine

Oh, got it. Then I'll leave the destination branch as is

duhminick · 2025-03-25T16:04:17Z

agent.json
output.txt

Attaching the agent config and translated configs

lisguo

Can you also provide a sample yaml config of the receiver?

receiver/awsebsnvmereceiver/internal/nvme/device_file_attributes.go

receiver/awsebsnvmereceiver/scraper.go

receiver/awsebsnvmereceiver/internal/nvme/util_stub.go

jefchien · 2025-03-28T19:18:13Z

receiver/awsebsnvmereceiver/internal/nvme/util.go

+	IsEbsDevice(device *DeviceFileAttributes) (bool, error)
+}
+
+type Util struct {


nit: I know we use Util as a name all over the place, but it doesn't help me understand what this is. Would rather it be called like DeviceInfoProvider or something more descriptive.

nit: Why is this a struct if it doesn't have a state? Why not just have the functions exported as package level functions?

nit: I know we use Util as a name all over the place, but it doesn't help me understand what this is. Would rather it be called like DeviceInfoProvider or something more descriptive.

Will do.

nit: Why is this a struct if it doesn't have a state? Why not just have the functions exported as package level functions?

I made it so it would be a bit easier to mock for unit testing

The common pattern is to have a constructor function like

func NewDeviceInfoProvider() DeviceInfoProvider { return &util{} }

so that only the interface is exposed.

receiver/awsebsnvmereceiver/internal/nvme/util.go

receiver/awsebsnvmereceiver/internal/nvme/util_stub.go

receiver/awsebsnvmereceiver/factory.go

receiver/awsebsnvmereceiver/scraper.go

receiver/awsebsnvmereceiver/internal/nvme/constants.go

receiver/awsebsnvmereceiver/internal/nvme/util_unix.go

jefchien · 2025-03-28T22:43:36Z

receiver/awsebsnvmereceiver/internal/nvme/ebs_metrics_unix.go

+var (
+	ErrInvalidEBSMagic = errors.New("invalid EBS magic number")
+	ErrParseLogPage    = errors.New("failed to parse log page")
+)


nit: If they aren't used outside of the package, don't export them.

Similar to the comment below about keeping the code as similar as possible to what is found in the EBS CSI driver code

nit: Could put CSI driver code into a separate package (e.g. internal/nvme/csidriver). Are we planning on trying to keep this in sync with the CSI driver code or are we willing to diverge?

jefchien · 2025-03-28T22:45:05Z

receiver/awsebsnvmereceiver/internal/nvme/ebs_metrics_unix.go

+func nvmeReadLogPage(fd uintptr, logID uint8) ([]byte, error) {
+	data := make([]byte, 4096) // 4096 bytes is the length of the log page.
+	bufferLen := len(data)
+
+	if bufferLen > math.MaxUint32 {
+		return nil, errors.New("nvmeReadLogPage: bufferLen exceeds MaxUint32")
+	}


I don't get this check. If we define the length of the slice, how would this ever return an error?

This comes directly from the EBS CSI driver code. I did see some things I could change, but I do have a preference to keep it the same as much as possible.

lisguo · 2025-04-02T15:10:11Z

receiver/awsebsnvmereceiver/scraper.go

-	volumeID   string
+type ebsDevices struct {
+	volumeID    string
+	deviceNames []string


one volume can have multiple devices?

So it's all based on the controller ID. For example: nvme0, nvme0n1, nvme0n2, nvme0n1p1 will all have the same serial/volume ID, and metrics. Whereas nvme1 would be different

jefchien · 2025-04-02T14:50:44Z

receiver/awsebsnvmereceiver/internal/nvme/util_unix.go

+	return devices, nil
+}
+
+func (u *Util) GetDeviceSerial(device *DeviceFileAttributes) (string, error) {


Validated in what way? device.BaseDeviceName() does the same check.

jefchien · 2025-04-02T19:55:15Z

receiver/awsebsnvmereceiver/internal/nvme/device_file_attributes_test.go

+		t.Run(tt.name, func(t *testing.T) {
+			got, err := ParseNvmeDeviceFileName(tt.device)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("ParseNvmeDeviceFileName() error = %v, wantErr %v", err, tt.wantErr)


nit: For consistency with other tests, consider using github.com/stretchr/testify/assert or github.com/stretchr/testify/require

jefchien · 2025-04-02T19:59:04Z

receiver/awsebsnvmereceiver/internal/nvme/ebs_metrics_unix.go

+var (
+	ErrInvalidEBSMagic = errors.New("invalid EBS magic number")
+	ErrParseLogPage    = errors.New("failed to parse log page")
+)


nit: Could put CSI driver code into a separate package (e.g. internal/nvme/csidriver). Are we planning on trying to keep this in sync with the CSI driver code or are we willing to diverge?

jefchien · 2025-04-02T20:00:38Z

receiver/awsebsnvmereceiver/internal/nvme/util.go

+	IsEbsDevice(device *DeviceFileAttributes) (bool, error)
+}
+
+type Util struct {


The common pattern is to have a constructor function like

func NewDeviceInfoProvider() DeviceInfoProvider { return &util{} }

so that only the interface is exposed.

jefchien · 2025-04-02T20:04:04Z

receiver/awsebsnvmereceiver/scraper.go

+		// retrieving the volume ID and validating if it's an EBS device
+		if entry, seenController := devices[device.Controller()]; seenController {
+			entry.deviceNames = append(entry.deviceNames, deviceName)
+			s.logger.Debug("skipping unnecessary device validation steps", zap.String("device", deviceName))


nit: Some of these logs seem excessive especially if this is going to get logged multiple times during each scrape.

duhminick force-pushed the dominic-nvme-receiver branch from 66fb288 to 546fb3e Compare March 18, 2025 15:04

duhminick changed the base branch from main to ebs March 18, 2025 15:19

Add new awsebsnvmereceiver

be8e93b

duhminick force-pushed the dominic-nvme-receiver branch from 546fb3e to be8e93b Compare March 18, 2025 15:23

honor resources field

1ba325f

duhminick marked this pull request as ready for review March 18, 2025 19:39

duhminick requested a review from a team as a code owner March 18, 2025 19:39

duhminick changed the base branch from ebs to main March 18, 2025 19:40

duhminick added 2 commits March 18, 2025 19:42

remove stale comment

92aa89a

add resource filtering unit test

4b2fee5

lisguo reviewed Mar 24, 2025

View reviewed changes

use fmt.Sscanf to parse device file name

35b753a

duhminick added 2 commits March 25, 2025 17:38

only enable one metric by default

cf1c59e

rename resources to devices

9d4a053

lisguo reviewed Mar 26, 2025

View reviewed changes

receiver/awsebsnvmereceiver/internal/nvme/device_file_attributes.go Outdated Show resolved Hide resolved

receiver/awsebsnvmereceiver/scraper.go Outdated Show resolved Hide resolved

lisguo previously approved these changes Mar 27, 2025

View reviewed changes

jefchien reviewed Mar 28, 2025

View reviewed changes

duhminick added 10 commits April 1, 2025 11:02

rename stub to notunix

c1684f9

remove filename in error

361fd89

make clear that error is ignored for sscanf

14e840c

refactor to use collections.Set

09d35f2

change info message to debug

d6b946b

dont unnecessarily export constants

cd305a4

dir check

889d93c

deviceinfoprovider rename + lint

10bd4d9

update unit tests

037dbe6

remove cleanupstring func

77d34b4

duhminick added 2 commits April 1, 2025 19:17

store original device name

bf467ce

remove unnecessary work

5c35d16

duhminick dismissed lisguo’s stale review via 5c35d16 April 1, 2025 20:29

duhminick and others added 4 commits April 1, 2025 20:30

fix

1b87c4f

format + lint

cfb30e4

Merge branch 'main' into dominic-nvme-receiver

edb1434

fix miss

6328c25

lisguo reviewed Apr 2, 2025

View reviewed changes

jefchien approved these changes Apr 2, 2025

View reviewed changes

lisguo approved these changes Apr 2, 2025

View reviewed changes

duhminick merged commit ecf2afd into aws:main Apr 2, 2025
7 checks passed

duhminick deleted the dominic-nvme-receiver branch April 2, 2025 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new awsebsnvmereceiver #1603

Add new awsebsnvmereceiver #1603

duhminick commented Mar 18, 2025 •

edited

Loading

lisguo commented Mar 25, 2025

duhminick commented Mar 25, 2025

duhminick commented Mar 25, 2025

lisguo left a comment

jefchien Mar 28, 2025

jefchien Mar 28, 2025

duhminick Apr 1, 2025

jefchien Apr 2, 2025

jefchien Mar 28, 2025

duhminick Apr 1, 2025

jefchien Apr 2, 2025

jefchien Mar 28, 2025

duhminick Apr 1, 2025

lisguo Apr 2, 2025

duhminick Apr 2, 2025 •

edited

Loading

jefchien Apr 2, 2025

jefchien Apr 2, 2025

jefchien Apr 2, 2025

jefchien Apr 2, 2025

jefchien Apr 2, 2025

Add new awsebsnvmereceiver #1603

Add new awsebsnvmereceiver #1603

Conversation

duhminick commented Mar 18, 2025 • edited Loading

Note to Reviewers

Changes 2:

Changes 3:

Description of the issue

Description of changes

License

Tests

Resource is explicitly empty

One device (nvme0) is in resources

* for resources

Sample Config

Requirements

lisguo commented Mar 25, 2025

duhminick commented Mar 25, 2025

duhminick commented Mar 25, 2025

lisguo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duhminick Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duhminick commented Mar 18, 2025 •

edited

Loading

`*` for resources

duhminick Apr 2, 2025 •

edited

Loading