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

Ensure tcp connection value is provided for all states, even when count is 0 #1329

Merged
merged 1 commit into from
Jul 13, 2020
Merged

Ensure tcp connection value is provided for all states, even when count is 0 #1329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 30 additions & 13 deletions receiver/hostmetricsreceiver/internal/scraper/networkscraper/network_scraper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package networkscraper

import (
"context"
"strings"
"time"

"github.com/shirou/gopsutil/host"
Expand All @@ -30,11 +29,15 @@ import (
type scraper struct {
config *Config
startTime pdata.TimestampUnixNano

// for mocking gopsutil net.IOCounters & net.Connections
ioCounters func(bool) ([]net.IOCountersStat, error)
connections func(string) ([]net.ConnectionStat, error)
}

// newNetworkScraper creates a set of Network related metrics
func newNetworkScraper(_ context.Context, cfg *Config) *scraper {
return &scraper{config: cfg}
return &scraper{config: cfg, ioCounters: net.IOCounters, connections: net.Connections}
}

// Initialize
Expand All @@ -59,12 +62,12 @@ func (s *scraper) ScrapeMetrics(_ context.Context) (pdata.MetricSlice, error) {

var errors []error

err := scrapeAndAppendNetworkCounterMetrics(metrics, s.startTime)
err := s.scrapeAndAppendNetworkCounterMetrics(metrics, s.startTime)
if err != nil {
errors = append(errors, err)
}

err = scrapeAndAppendNetworkTCPConnectionsMetric(metrics)
err = s.scrapeAndAppendNetworkTCPConnectionsMetric(metrics)
if err != nil {
errors = append(errors, err)
}
Expand All @@ -76,9 +79,9 @@ func (s *scraper) ScrapeMetrics(_ context.Context) (pdata.MetricSlice, error) {
return metrics, nil
}

func scrapeAndAppendNetworkCounterMetrics(metrics pdata.MetricSlice, startTime pdata.TimestampUnixNano) error {
func (s *scraper) scrapeAndAppendNetworkCounterMetrics(metrics pdata.MetricSlice, startTime pdata.TimestampUnixNano) error {
// get total stats only
networkStatsSlice, err := net.IOCounters( /*perNetworkInterfaceController=*/ false)
networkStatsSlice, err := s.ioCounters( /*perNetworkInterfaceController=*/ false)
if err != nil {
return err
}
Expand Down Expand Up @@ -111,26 +114,40 @@ func initializeNetworkDataPoint(dataPoint pdata.Int64DataPoint, startTime pdata.
dataPoint.SetValue(value)
}

func scrapeAndAppendNetworkTCPConnectionsMetric(metrics pdata.MetricSlice) error {
connections, err := net.Connections("tcp")
func (s *scraper) scrapeAndAppendNetworkTCPConnectionsMetric(metrics pdata.MetricSlice) error {
connections, err := s.connections("tcp")
if err != nil {
return err
}

connectionStatusCounts := getConnectionStatusCounts(connections)
connectionStatusCounts := getTCPConnectionStatusCounts(connections)

startIdx := metrics.Len()
metrics.Resize(startIdx + 1)
initializeNetworkTCPConnectionsMetric(metrics.At(startIdx), connectionStatusCounts)
return nil
}

func getConnectionStatusCounts(connections []net.ConnectionStat) map[string]int64 {
connectionStatuses := make(map[string]int64, len(connections))
func getTCPConnectionStatusCounts(connections []net.ConnectionStat) map[string]int64 {
var tcpStatuses = map[string]int64{
Copy link
Member

Choose a reason for hiding this comment

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

Why not an array and a enum for status? Seems better for me

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 not totally sure what you mean by this.

Do you mean enum as in?

type tcpStatus string

const (
	closeWait   tcpStatus = "CLOSE_WAIT"
	closed      tcpStatus = "CLOSED"
	...
)

Also I think a map should outperform an array here since we need to iterate through potentially 100s of connections to record the counts. Using an array would require O(n^2)?

Copy link
Member

Choose a reason for hiding this comment

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

I mean an enum as in:

type tcpStatus int

const (
	closeWait   tcpStatus = iota
	closed      tcpStatus
	...
)

Copy link
Member

Choose a reason for hiding this comment

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

You may need a switch for the status. Not 100% sure so your call

Copy link
Member Author

@james-bebbington james-bebbington Jul 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oh okay yea - ofc 🤦‍♂️

We would need a switch to convert strings to indexes, so I suspect performance would be pretty similar either way. Annoyingly gopsutil actually converts these from ints to strings before returning the values 😞

If you don't feel too strongly about this I will probably leave it as is. This has the arguably useful propery that if we ever receive an unexpected status value, it will get added to the map.

"CLOSE_WAIT": 0,
"CLOSED": 0,
"CLOSING": 0,
"DELETE": 0,
"ESTABLISHED": 0,
"FIN_WAIT_1": 0,
"FIN_WAIT_2": 0,
"LAST_ACK": 0,
"LISTEN": 0,
"SYN_SENT": 0,
"SYN_RECEIVED": 0,
"TIME_WAIT": 0,
}

for _, connection := range connections {
connectionStatuses[strings.ToLower(connection.Status)]++
tcpStatuses[connection.Status]++
}
return connectionStatuses
return tcpStatuses
}

func initializeNetworkTCPConnectionsMetric(metric pdata.Metric, connectionStateCounts map[string]int64) {
Expand Down
88 changes: 60 additions & 28 deletions receiver/hostmetricsreceiver/internal/scraper/networkscraper/network_scraper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,51 +16,83 @@ package networkscraper

import (
"context"
"errors"
"testing"

"github.com/shirou/gopsutil/net"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"go.opentelemetry.io/collector/consumer/pdata"
"go.opentelemetry.io/collector/receiver/hostmetricsreceiver/internal"
)

type validationFn func(*testing.T, pdata.MetricSlice)

func TestScrapeMetrics(t *testing.T) {
createScraperAndValidateScrapedMetrics(t, &Config{}, func(t *testing.T, metrics pdata.MetricSlice) {
// expect 5 metrics
assert.Equal(t, 5, metrics.Len())
type testCase struct {
name string
ioCountersFunc func(bool) ([]net.IOCountersStat, error)
connectionsFunc func(string) ([]net.ConnectionStat, error)
expectedErr string
}

testCases := []testCase{
{
name: "Standard",
},
{
name: "IOCounters Error",
ioCountersFunc: func(bool) ([]net.IOCountersStat, error) { return nil, errors.New("err1") },
expectedErr: "err1",
},
{
name: "Connections Error",
connectionsFunc: func(string) ([]net.ConnectionStat, error) { return nil, errors.New("err2") },
expectedErr: "err2",
},
}

for _, test := range testCases {
t.Run(test.name, func(t *testing.T) {
scraper := newNetworkScraper(context.Background(), &Config{})
if test.ioCountersFunc != nil {
scraper.ioCounters = test.ioCountersFunc
}
if test.connectionsFunc != nil {
scraper.connections = test.connectionsFunc
}

err := scraper.Initialize(context.Background())
require.NoError(t, err, "Failed to initialize network scraper: %v", err)
defer func() { assert.NoError(t, scraper.Close(context.Background())) }()

// for network packets, dropped packets, errors, & bytes metrics, expect a transmit & receive datapoint
assertNetworkMetricMatchesDescriptorAndHasTransmitAndReceiveDataPoints(t, metrics.At(0), networkPacketsDescriptor)
assertNetworkMetricMatchesDescriptorAndHasTransmitAndReceiveDataPoints(t, metrics.At(1), networkDroppedPacketsDescriptor)
assertNetworkMetricMatchesDescriptorAndHasTransmitAndReceiveDataPoints(t, metrics.At(2), networkErrorsDescriptor)
assertNetworkMetricMatchesDescriptorAndHasTransmitAndReceiveDataPoints(t, metrics.At(3), networkIODescriptor)
metrics, err := scraper.ScrapeMetrics(context.Background())
if test.expectedErr != "" {
assert.EqualError(t, err, test.expectedErr)
return
}
require.NoError(t, err, "Failed to scrape metrics: %v", err)

// for tcp connections metric, expect at least one datapoint with a state label
networkTCPConnectionsMetric := metrics.At(4)
internal.AssertDescriptorEqual(t, networkTCPConnectionsDescriptor, networkTCPConnectionsMetric.MetricDescriptor())
internal.AssertInt64MetricLabelExists(t, networkTCPConnectionsMetric, 0, stateLabelName)
assert.GreaterOrEqual(t, networkTCPConnectionsMetric.Int64DataPoints().Len(), 1)
})
assert.Equal(t, 5, metrics.Len())

assertNetworkIOMetricValid(t, metrics.At(0), networkPacketsDescriptor)
assertNetworkIOMetricValid(t, metrics.At(1), networkDroppedPacketsDescriptor)
assertNetworkIOMetricValid(t, metrics.At(2), networkErrorsDescriptor)
assertNetworkIOMetricValid(t, metrics.At(3), networkIODescriptor)

assertNetworkTCPConnectionsMetricValid(t, metrics.At(4))
})
}
}

func assertNetworkMetricMatchesDescriptorAndHasTransmitAndReceiveDataPoints(t *testing.T, metric pdata.Metric, expectedDescriptor pdata.MetricDescriptor) {
internal.AssertDescriptorEqual(t, expectedDescriptor, metric.MetricDescriptor())
func assertNetworkIOMetricValid(t *testing.T, metric pdata.Metric, descriptor pdata.MetricDescriptor) {
internal.AssertDescriptorEqual(t, descriptor, metric.MetricDescriptor())
assert.Equal(t, 2, metric.Int64DataPoints().Len())
internal.AssertInt64MetricLabelHasValue(t, metric, 0, directionLabelName, transmitDirectionLabelValue)
internal.AssertInt64MetricLabelHasValue(t, metric, 1, directionLabelName, receiveDirectionLabelValue)
}

func createScraperAndValidateScrapedMetrics(t *testing.T, config *Config, assertFn validationFn) {
scraper := newNetworkScraper(context.Background(), config)
err := scraper.Initialize(context.Background())
require.NoError(t, err, "Failed to initialize network scraper: %v", err)
defer func() { assert.NoError(t, scraper.Close(context.Background())) }()

metrics, err := scraper.ScrapeMetrics(context.Background())
require.NoError(t, err, "Failed to scrape metrics: %v", err)

assertFn(t, metrics)
func assertNetworkTCPConnectionsMetricValid(t *testing.T, metric pdata.Metric) {
internal.AssertDescriptorEqual(t, networkTCPConnectionsDescriptor, metric.MetricDescriptor())
internal.AssertInt64MetricLabelExists(t, metric, 0, stateLabelName)
assert.Equal(t, 12, metric.Int64DataPoints().Len())
}