[vSphere][network] Add support for new metrics in network metricset

[kush-elastic]

• Enhancement

Description

Added following new metrics for the Network datastream in the vSphere metricbeat module.

Metrics Type	Metrics	API	Field Mappings
Network/DistributedVirtualPort	Network.OverallStatus	Summary	status
	Network.Summary.Accessible	Summary	accessible
	Network.ConfigStatus	Summary	config.status
	Network.type	Summary	type
	Network.Vm	ExtraApi	vm.names
		ExtraApi	vm.count
	Network.Host	ExtraApi	host.names
		ExtraApi	host.count

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have made corresponding change to the default configuration files
• I have added tests that prove my fix is effective or that my feature works
• I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

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

[mergify]

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @kush-elastic? 🙏.
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

[mergify]

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b 27-vsphere-implement-the-network-metricset upstream/27-vsphere-implement-the-network-metricset
git merge upstream/main
git push upstream 27-vsphere-implement-the-network-metricset

[lucian-ioan]

More of a question here @kush-elastic. Could it be possible for the Fetch method to hit a bottleneck depending on the size of the vSphere environment?

The fix that we should keep in mind should this ever happen would be to implement some sort of pagination using a batch size.

[kush-elastic]

We are only fetching the necessary information, so I believe we won't encounter this problem. However, I will still investigate pagination options.

[lucian-ioan]

LGTM after the CI is fixed, minor nits.

[mergify]

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b 27-vsphere-implement-the-network-metricset upstream/27-vsphere-implement-the-network-metricset
git merge upstream/main
git push upstream 27-vsphere-implement-the-network-metricset

[shmsr]

Similar review comments (like in other vsphere PRs). You can directly apply the diff patch.

diff --git a/metricbeat/module/vsphere/network/_meta/fields.yml b/metricbeat/module/vsphere/network/_meta/fields.yml
index 25bf49cda0..13bf09ac27 100644
--- a/metricbeat/module/vsphere/network/_meta/fields.yml
+++ b/metricbeat/module/vsphere/network/_meta/fields.yml
@@ -2,43 +2,43 @@
   type: group
   release: beta
   description: >
-    network
+    Network-related information.
   fields:
     - name: accessible
       type: boolean
       description: >
-        At least one host is configured to provide this network.
+        Indicates whether at least one host is configured to provide this network.
     - name: config.status
       type: keyword
       description: >
-        The configStatus indicates whether or not the system has detected a configuration issue.
+        Indicates whether the system has detected a configuration issue.
     - name: host
       type: group
       fields:
         - name: names
           type: keyword
           description: >
-            Names of the hosts on the network.
+            Names of the hosts connected to this network.
         - name: count
           type: long
           description: >
-            Number of hosts on the network.
+            Number of hosts connected to this network.
     - name: name
       type: keyword
       description: >
-        Name of Network.
+        Name of the network.
     - name: status
       type: keyword
       description: >
-        General health of Network.
+        Overall health status of the network.
     - name: vm
       type: group
       fields:
         - name: names
           type: keyword
           description: >
-            Names of the virtual machines on the network.
+            Names of the virtual machines connected to this network.
         - name: count
           type: long
           description: >
-            Number of virtual machines on the network.
+            Number of virtual machines connected to this network.
diff --git a/metricbeat/module/vsphere/network/data.go b/metricbeat/module/vsphere/network/data.go
index cab569ef89..568b624f9e 100644
--- a/metricbeat/module/vsphere/network/data.go
+++ b/metricbeat/module/vsphere/network/data.go
@@ -1,3 +1,20 @@
+// Licensed to Elasticsearch B.V. under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Elasticsearch B.V. licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
 package network
 
 import (
diff --git a/metricbeat/module/vsphere/network/data_test.go b/metricbeat/module/vsphere/network/data_test.go
index 90c45e949b..3e5cdb7019 100644
--- a/metricbeat/module/vsphere/network/data_test.go
+++ b/metricbeat/module/vsphere/network/data_test.go
@@ -26,8 +26,8 @@ import (
 )
 
 func TestEventMapping(t *testing.T) {
-	var m *MetricSet
-	var NetworkTest = mo.Network{
+	m := &MetricSet{}
+	networkTest := mo.Network{
 		Summary: &types.NetworkSummary{
 			Accessible: true,
 		},
@@ -44,23 +44,23 @@ func TestEventMapping(t *testing.T) {
 		},
 	}
 
-	event := m.eventMapping(NetworkTest, &metricDataTest)
+	event := m.eventMapping(networkTest, &metricDataTest)
 
 	name, _ := event.GetValue("name")
 	assert.NotNil(t, name)
 
 	status, _ := event.GetValue("status")
-	assert.EqualValues(t, "green", status)
+	assert.Equal(t, "green", status)
 
-	ConfigStatus, _ := event.GetValue("config.status")
-	assert.EqualValues(t, "green", ConfigStatus)
+	configStatus, _ := event.GetValue("config.status")
+	assert.Equal(t, "green", configStatus)
 
-	Accessible, _ := event.GetValue("accessible")
-	assert.EqualValues(t, true, Accessible)
+	accessible, _ := event.GetValue("accessible")
+	assert.True(t, accessible.(bool))
 
-	Hostname, _ := event.GetValue("host.names")
-	assert.EqualValues(t, metricDataTest.assetsName.outputHostNames, Hostname)
+	hostname, _ := event.GetValue("host.names")
+	assert.Equal(t, metricDataTest.assetsName.outputHostNames, hostname)
 
-	Vmname, _ := event.GetValue("vm.names")
-	assert.EqualValues(t, metricDataTest.assetsName.outputVmNames, Vmname)
+	vmname, _ := event.GetValue("vm.names")
+	assert.Equal(t, metricDataTest.assetsName.outputVmNames, vmname)
 }
diff --git a/metricbeat/module/vsphere/network/network.go b/metricbeat/module/vsphere/network/network.go
index 79a017aca9..0dee754306 100644
--- a/metricbeat/module/vsphere/network/network.go
+++ b/metricbeat/module/vsphere/network/network.go
@@ -1,3 +1,20 @@
+// Licensed to Elasticsearch B.V. under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Elasticsearch B.V. licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
 package network
 
 import (
@@ -5,12 +22,13 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/elastic/beats/v7/metricbeat/mb"
-	"github.com/elastic/beats/v7/metricbeat/module/vsphere"
 	"github.com/vmware/govmomi"
 	"github.com/vmware/govmomi/property"
 	"github.com/vmware/govmomi/view"
 	"github.com/vmware/govmomi/vim25/mo"
+
+	"github.com/elastic/beats/v7/metricbeat/mb"
+	"github.com/elastic/beats/v7/metricbeat/module/vsphere"
 )
 
 // init registers the MetricSet with the central registry as soon as the program
@@ -33,7 +51,7 @@ type MetricSet struct {
 func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
 	ms, err := vsphere.NewMetricSet(base)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to create vSphere metricset: %w", err)
 	}
 	return &MetricSet{ms}, nil
 }
@@ -51,7 +69,6 @@ type assetNames struct {
 // format. It publishes the event which is then forwarded to the output. In case
 // of an error set the Error field of mb.Event or simply call report.Error().
 func (m *MetricSet) Fetch(ctx context.Context, reporter mb.ReporterV2) error {
-
 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()
 
@@ -62,16 +79,13 @@ func (m *MetricSet) Fetch(ctx context.Context, reporter mb.ReporterV2) error {
 
 	defer func() {
 		if err := client.Logout(ctx); err != nil {
-			m.Logger().Errorf("error trying to log out from vSphere: %w", err)
+			m.Logger().Errorf("error trying to logout from vSphere: %w", err)
 		}
 	}()
 
 	c := client.Client
 
-	// Create a view of Network objects.
-	mgr := view.NewManager(c)
-
-	v, err := mgr.CreateContainerView(ctx, c.ServiceContent.RootFolder, []string{"Network"}, true)
+	v, err := view.NewManager(c).CreateContainerView(ctx, c.ServiceContent.RootFolder, []string{"Network"}, true)
 	if err != nil {
 		return fmt.Errorf("error in CreateContainerView: %w", err)
 	}
@@ -83,41 +97,38 @@ func (m *MetricSet) Fetch(ctx context.Context, reporter mb.ReporterV2) error {
 	}()
 
 	// Retrieve property for all networks
-	var nets []mo.Network
-	err = v.Retrieve(ctx, []string{"Network"}, []string{"summary", "name", "overallStatus", "configStatus", "vm", "host", "name"}, &nets)
+	var networks []mo.Network
+	err = v.Retrieve(ctx, []string{"Network"}, []string{"summary", "name", "overallStatus", "configStatus", "vm", "host", "name"}, &networks)
 	if err != nil {
 		return fmt.Errorf("error in Retrieve: %w", err)
 	}
 
 	pc := property.DefaultCollector(c)
-	for i := range nets {
-		assetNames, err := getAssetNames(ctx, pc, &nets[i])
+	for _, network := range networks {
+		assetNames, err := getAssetNames(ctx, pc, &network)
 		if err != nil {
-			m.Logger().Errorf("Failed to retrieve object from network %s: %w", nets[i].Name, err)
+			m.Logger().Errorf("Failed to retrieve object from network %s: %v", network.Name, err)
+			continue
 		}
 
 		reporter.Event(mb.Event{
-			MetricSetFields: m.eventMapping(nets[i], &metricData{
-				assetsName: assetNames,
-			}),
+			MetricSetFields: m.eventMapping(network, &metricData{assetsName: assetNames}),
 		})
 	}
 
 	return nil
 }
 
-func getAssetNames(ctx context.Context, pc *property.Collector, net *mo.Network) (assetNames, error) {
-	referenceList := append(net.Host, net.Vm...)
-
+func getAssetNames(ctx context.Context, pc *property.Collector, network *mo.Network) (assetNames, error) {
 	var objects []mo.ManagedEntity
-	if len(referenceList) > 0 {
-		if err := pc.Retrieve(ctx, referenceList, []string{"name"}, &objects); err != nil {
-			return assetNames{}, err
+	if len(network.Vm) > 0 {
+		if err := pc.Retrieve(ctx, network.Vm, []string{"name"}, &objects); err != nil {
+			return assetNames{}, fmt.Errorf("failed to retrieve managed entities: %w", err)
 		}
 	}
 
-	outputHostNames := make([]string, 0, len(net.Host))
-	outputVmNames := make([]string, 0, len(net.Vm))
+	outputHostNames := make([]string, 0, len(network.Host))
+	outputVmNames := make([]string, 0, len(network.Vm))
 	for _, ob := range objects {
 		name := strings.ReplaceAll(ob.Name, ".", "_")
 		switch ob.Reference().Type {
diff --git a/metricbeat/module/vsphere/network/network_test.go b/metricbeat/module/vsphere/network/network_test.go
index 8a0cd5d57e..3848a791fd 100644
--- a/metricbeat/module/vsphere/network/network_test.go
+++ b/metricbeat/module/vsphere/network/network_test.go
@@ -1,3 +1,8 @@
+// Licensed to Elasticsearch B.V. under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Elasticsearch B.V. licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
 // not use this file except in compliance with the License.
 // You may obtain a copy of the License at
 //
@@ -15,21 +20,23 @@ package network
 import (
 	"testing"
 
-	mbtest "github.com/elastic/beats/v7/metricbeat/mb/testing"
-	"github.com/elastic/elastic-agent-libs/mapstr"
-
 	"github.com/stretchr/testify/assert"
 	"github.com/vmware/govmomi/simulator"
+
+	mbtest "github.com/elastic/beats/v7/metricbeat/mb/testing"
+	"github.com/elastic/elastic-agent-libs/mapstr"
 )
 
 func TestFetchEventContents(t *testing.T) {
 	model := simulator.VPX()
-	if err := model.Create(); err != nil {
+	err := model.Create()
+	if err != nil {
 		t.Fatal(err)
 	}
+	t.Cleanup(func() { model.Remove() })
 
 	ts := model.Service.NewServer()
-	defer ts.Close()
+	t.Cleanup(func() { ts.Close() })
 
 	f := mbtest.NewReportingMetricSetV2WithContext(t, getConfig(ts))
 	events, errs := mbtest.ReportingFetchV2WithContext(f)
@@ -43,7 +50,7 @@ func TestFetchEventContents(t *testing.T) {
 
 	t.Logf("%s/%s event: %+v", f.Module().Name(), f.Name(), event.StringToPrint())
 
-	assert.NotNil(t, event["name"])
+	assert.NotEmpty(t, event["name"])
 	assert.EqualValues(t, true, event["accessible"])
 	assert.EqualValues(t, "green", event["status"])
 
@@ -53,24 +60,26 @@ func TestFetchEventContents(t *testing.T) {
 	host, ok := event["host"].(mapstr.M)
 	if ok {
 		assert.GreaterOrEqual(t, host["count"], 0)
-		assert.NotNil(t, host["names"])
+		assert.NotEmpty(t, host["names"])
 	}
 
 	vm, ok := event["vm"].(mapstr.M)
 	if ok {
 		assert.GreaterOrEqual(t, vm["count"], 0)
-		assert.NotNil(t, vm["names"])
+		assert.NotEmpty(t, vm["names"])
 	}
 }
 
 func TestData(t *testing.T) {
 	model := simulator.ESX()
-	if err := model.Create(); err != nil {
+	err := model.Create()
+	if err != nil {
 		t.Fatal(err)
 	}
+	t.Cleanup(func() { model.Remove() })
 
 	ts := model.Service.NewServer()
-	defer ts.Close()
+	t.Cleanup(func() { ts.Close() })
 
 	f := mbtest.NewReportingMetricSetV2WithContext(t, getConfig(ts))
 

Also, make sure to not leave the LICENSE text

[devamanv]

Looks good to me! Just left a minor suggestion.

[harnish-elastic]

LGTM! 🚀

[ishleenk17]

Address one open comment about data.json.
Otherwise, change looks good!