Add support of snapshot in vSphere virtualmachine metricset

[ishleenk17]

Add support of adding snapshot details to the Virtualmachine object in Vsphere.
It caters to the snapshot and any childsnapshots as well.

Relates: https://github.com/elastic/obs-integration-team/issues/34

[mergify]

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @ishleenk17? 🙏.
For such, you'll need to label your PR with:

• The upcoming major version of the Elastic Stack
• The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

[ishleenk17]

/test

[niraj-elastic]

@ishleenk17 please add entry in ascii.next file.

[niraj-elastic]

Please update metricbeat/module/vsphere/virtualmachine/_meta/data.json as well.

[ishleenk17]

Please update metricbeat/module/vsphere/virtualmachine/_meta/data.json as well.

There is no way to test this, due to unavailability of snapshots. Hence its not updated.
An option is to put the dummy data in the sample.

[niraj-elastic]

Please update metricbeat/module/vsphere/virtualmachine/_meta/data.json as well.

There is no way to test this, due to unavailability of snapshots. Hence its not updated. An option is to put the dummy data in the sample.

I understand, But the main prepose of data.json is to give idea about event that is going to be produced. I think we should add dummy fields because it would capture structure of our event.

[shmsr]

Please update metricbeat/module/vsphere/virtualmachine/_meta/data.json as well.

There is no way to test this, due to unavailability of snapshots. Hence its not updated. An option is to put the dummy data in the sample.

I understand, But the main prepose of data.json is to give idea about event that is going to be produced. I think we should add dummy fields because it would capture structure of our event.

I think it is possible to write a mock (see example) but given the strict timeline we can take it up in future to properly generate the data.json. For now, we can go with dummy.

[shmsr]

diff --git a/metricbeat/module/vsphere/virtualmachine/data.go b/metricbeat/module/vsphere/virtualmachine/data.go
index 69d0bd1df5..bc29c1f8ef 100644
--- a/metricbeat/module/vsphere/virtualmachine/data.go
+++ b/metricbeat/module/vsphere/virtualmachine/data.go
@@ -72,9 +72,9 @@ func (m *MetricSet) mapEvent(data VMData) mapstr.M {
 	if len(data.DatastoreNames) > 0 {
 		event["datastore.names"] = data.DatastoreNames
 	}
-	if len(data.Snapshots) > 0 {
+	if n := len(data.Snapshots); n > 0 {
 		event["snapshot.info"] = data.Snapshots
-		event["snapshot.count"] = len(data.Snapshots)
+		event["snapshot.count"] = n
 	}
 
 	return event
diff --git a/metricbeat/module/vsphere/virtualmachine/virtualmachine.go b/metricbeat/module/vsphere/virtualmachine/virtualmachine.go
index 81052e1eed..071c4f6cfb 100644
--- a/metricbeat/module/vsphere/virtualmachine/virtualmachine.go
+++ b/metricbeat/module/vsphere/virtualmachine/virtualmachine.go
@@ -61,6 +61,10 @@ type VMData struct {
 }
 
 type VMSnapshotData struct {
+	ID          int32
+	State       string
+	Quiesced    bool
+	Age         time.Duration
 	Name        string
 	Description string
 	CreateTime  time.Time
@@ -286,9 +290,12 @@ func getHostSystem(ctx context.Context, c *vim25.Client, ref types.ManagedObject
 }
 
 func fetchSnapshots(snapshotTree []types.VirtualMachineSnapshotTree) []VMSnapshotData {
-	var snapshots []VMSnapshotData
+	snapshots := make([]VMSnapshotData, 0, len(snapshotTree))
 	for _, snapshot := range snapshotTree {
 		snapshots = append(snapshots, VMSnapshotData{
+			ID:          snapshot.Id,
+			State:       string(snapshot.State),
+			Quiesced:    snapshot.Quiesced,
 			Name:        snapshot.Name,
 			Description: snapshot.Description,
 			CreateTime:  snapshot.CreateTime,

Code added looks good. Just have a couple of things to suggest as you can see from the patch:

• IMO, adding ID and State makes sense for Snapshot as ID is the unique identifier for the Snapshot and State gives the info if the snapshot is powered on/off or suspended. Quiesced seems also important but not sure how much:

Quiesced definition:

// Flag to indicate whether or not the snapshot was created with
// the "quiesce" option, ensuring a consistent state of the file system.

• Also, I did some safe preallocation for the slice; fewer allocations now than before.

[shmsr]

Left one comment to address. Everything else looks good. So, approving!

[ishleenk17]

• IMO, adding ID and State makes sense for Snapshot as ID is the unique identifier for the Snapshot and State gives the info if the snapshot is powered on/off or suspended. Quiesced seems also important but not sure how much:

State and ID could be helpful, so adding those. Quiesced is not required IMO.

[niraj-elastic]

LGTM

[niraj-elastic]

@ishleenk17 I was working on alerts and i found that the way in which we define snapshot field, does not work. It will create field with name snapshot.info also since name in objects are defined with capital letters the dynamic fields will also be capital. For example
"snapshot.info": { "ID": "xxxx" }

We have to change it.

[shmsr]

@ishleenk17 I was working on alerts and i found that the way in which we define snapshot field, does not work. It will create field with name snapshot.info also since name in objects are defined with capital letters the dynamic fields will also be capital. For example "snapshot.info": { "ID": "xxxx" }

We have to change it.

I agree with this point to make it small-case. But I'm curious; if field names are not in small-case, alerts don't work?