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.

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

[Metricbeat] add a network_summary metricset #15196

Merged
merged 24 commits into from
Jan 13, 2020

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented Dec 18, 2019

See #12210

This adds a new system metricset, network_summary, that reports global network counters under Linux. This is a fairly small code addition, as most of the complexity is in go-sysinfo. The large diff is because we need to update go-sysinfo, and also golang.org/x/sys due to this: elastic/go-sysinfo#67

This is a draft as there's two things we need to sort out:

  • Mapping. I tried to find a happy medium between how netstat -s does things, and how the raw data coming off of /proc/net/{snmp,netstat} is shown. netstat provides human-readable descriptions and key values, but the actual counters that we get are hard-coded, meaning we don't get 100% of the data the kernel presents. On the other side, the /proc/net data is complete, but the key values aren't particularly helpful. My philosophy was that we should present as much of the data as we can get in a mapping that's reasonably intuitive, and then maybe provide a dashboard to make the data more human-readable

Test plan:

  • Start metricbeat with this metricset enabled on a linux system
  • Check for output matching the _meta/data.json

@fearful-symmetry fearful-symmetry requested a review from a team December 18, 2019 21:07
@fearful-symmetry fearful-symmetry self-assigned this Dec 18, 2019
@fearful-symmetry
Copy link
Contributor Author

jenkins, test this

@fearful-symmetry fearful-symmetry marked this pull request as ready for review December 30, 2019 18:07
@kvch
Copy link
Contributor

kvch commented Dec 31, 2019

I believe it is going to be fixed by this PR: #15265

NOTICE.txt Outdated Show resolved Hide resolved
metricbeat/module/system/network_summary/_meta/fields.yml Outdated Show resolved Hide resolved
}

// combineMap concatinates two given maps
func combineMap(map1, map2 map[string]int64) map[string]int64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any designated utils package for such functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I could find any, but someone else might know.

Copy link
Contributor

@ycombinator ycombinator Jan 6, 2020

Choose a reason for hiding this comment

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

This looks a lot like:

// MapStrUnion creates a new MapStr containing the union of the
// key-value pairs of the two maps. If the same key is present in
// both, the key-value pairs from dict2 overwrite the ones from dict1.
func MapStrUnion(dict1 MapStr, dict2 MapStr) MapStr {
dict := MapStr{}
for k, v := range dict1 {
dict[k] = v
}
for k, v := range dict2 {
dict[k] = v
}
return dict
}
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, considering that takes a MapStr and I have a map[string]int64 I'm not sure whatever memory/performance overhead of copying/casting is worth it.

Copy link

Choose a reason for hiding this comment

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

The memory layout for MapStr would be quite different and we currently have no functions to convert any type into MapStr (in the filebeat-new-registry-file feature branch we have some functionality that can convert between abritrary types). We do not have a designated package for custom map types. Plus MapStr would be a little overkill here.

It's tempting to have a package dealing with common cases in the future once we have generics :), but I'm not keen to use tools that rely on code generation (unless it's very local in another package).

How much of a problem is this in metricbeat? Do we have 10+ modules repeating this piece of code, or is it the first one we ever need this?

If it is no "real" problem, I'd rather keep this function here as is. Small optimization tip: var compMap = make(map[string]int64, len(map1) + len(map2)).

If we find this to be a real problem, we should discuss it again in a broader context.

metricbeat/module/system/network_summary/_meta/fields.yml Outdated Show resolved Hide resolved
@willemdh
Copy link

willemdh commented Jan 6, 2020

netstat -su

Mon Jan 6 15:27:01 CET 2020
IcmpMsg:
    InType3: 6
    InType8: 854
    OutType0: 854
    OutType3: 17601
Udp:
    2155688 packets received
    98531 packets to unknown port received.
    462927 packet receive errors
    504223 packets sent
    462927 receive buffer errors
    0 send buffer errors
UdpLite:
IpExt:
    InNoRoutes: 120
    InMcastPkts: 2
    InBcastPkts: 20909
    InOctets: 143911332374
    OutOctets: 264831045679
    InMcastOctets: 72
    InBcastOctets: 2344579
    InNoECTPkts: 48921561
    InECT0Pkts: 192

What info would you gather from netstat? It would be really nice to be able to graph udp:

   2155688 packets received
     462927 packet receive errors

In order to troubleshoot udp performance issues on Logstash.

@fearful-symmetry fearful-symmetry requested a review from a team January 6, 2020 17:26
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Thank you for addressing all comments. If there are no more comments, feel free to ship it!

"system": {
"network_summary": {
"icmp": {
"InAddrMaskReps": 0,
Copy link

Choose a reason for hiding this comment

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

is it common practice in metricbeat to use camel case for a set of 'undocumented' metrics? Do we want to convert to snake case (e.g. github.com/stoewer/go-strcase).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, my thinking was that since we're inserting "blind" metrics with no real opinionated manipulating of the data, keeping the key values the same as the their OS values might be a good idea. Let me hunt around the system module to see if we do this in other places.

Copy link

Choose a reason for hiding this comment

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

@exekias @jsoriano Do we have any precedence on using Pascal/CamelCase in events in metricbeat?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have done this when ingesting metrics in raw from a source, to avoid confusion. AWS cloudwatch is an example. That said, I don't have a strong opinion here

@fearful-symmetry fearful-symmetry added the needs_backport PR is waiting to be backported to other branches. label Jan 13, 2020
compMap[k] = checkMaxConn(k, v)
}
return compMap
}
Copy link

Choose a reason for hiding this comment

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

Now we've got a type similar to common.MapStr. Consider to split normalizing and merging into separate functions like:

func combineMap(m1, m2 map[string]uint64) common.MapStr {
  return common.MapStrUnion(normalizeMap(m1), normalizeMap(m2))
}

func normalizeMap(m map[string]uint64) common.MapStr {
  to := common.MapStr{}
  for k, v := range {
    to[k] = normalize(k, v)
  }
  return to
}

func normalize(k string, v uint64) interface{} {
  if k == "MacConn" {
    return int64(v)
  }
  return v
}

@fearful-symmetry fearful-symmetry merged commit 8b9ffbd into elastic:master Jan 13, 2020
fearful-symmetry added a commit to fearful-symmetry/beats that referenced this pull request Jan 13, 2020
* commit of system/network_summary

(cherry picked from commit 8b9ffbd)
@fearful-symmetry fearful-symmetry removed the needs_backport PR is waiting to be backported to other branches. label Jan 13, 2020
fearful-symmetry added a commit that referenced this pull request Jan 13, 2020
…et (#15517)

* [Metricbeat] add a network_summary metricset  (#15196)

* commit of system/network_summary

(cherry picked from commit 8b9ffbd)

* update fields
@fearful-symmetry fearful-symmetry added the test-plan Add this PR to be manual test plan label Jan 15, 2020
@kvch kvch assigned kvch and unassigned fearful-symmetry Jan 17, 2020
@kvch kvch added the test-plan-regression Manually testing this PR found a regression label Jan 17, 2020
@kvch
Copy link
Contributor

kvch commented Jan 17, 2020

Missing from reference config: #15637
Otherwise, awesomesauce.

@kvch kvch added test-plan-ok This PR passed manual testing and removed test-plan-regression Manually testing this PR found a regression labels Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Team:Integrations Label for the Integrations team test-plan Add this PR to be manual test plan test-plan-ok This PR passed manual testing v7.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants