Socket summary module #6782
Merged
Socket summary module #6782
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
houndci-bot
reviewed
Apr 5, 2018
| @@ -0,0 +1,76 @@ | |||
| package socket_summary | |||
There was a problem hiding this comment.
don't use an underscore in package name
| @@ -0,0 +1,87 @@ | |||
| package socket_summary | |||
There was a problem hiding this comment.
don't use an underscore in package name
|
How to make hound-ci happy? |
ruflin
reviewed
Apr 6, 2018
| // the MetricSet for each host defined in the module's configuration. After the | ||
| // MetricSet has been created then Fetch will begin to be called periodically. | ||
| func init() { | ||
| mb.Registry.MustAddMetricSet("system", "socket_summary", New) |
There was a problem hiding this comment.
A new feature is that the namespace can now also be set during registration time: https://github.com/elastic/beats/blob/master/metricbeat/module/logstash/node_stats/node_stats.go#L22
|
@agathver For the hound_ci on the package names, ignore it. |
|
@ruflin Made changes as requested |
jsoriano
requested changes
Apr 6, 2018
| @@ -0,0 +1,17 @@ | |||
| The System `socket_summary` metricset provides the summary of open sockets in the. | |||
There was a problem hiding this comment.
Oh! How did that happen
|
|
||
| This metricset is available on: | ||
|
|
||
| - Linux |
There was a problem hiding this comment.
The net.Connections method used to obtain stats seems supported in more operating systems https://github.com/shirou/gopsutil/tree/master/net
| // MetricSet has been created then Fetch will begin to be called periodically. | ||
| func init() { | ||
| mb.Registry.MustAddMetricSet("system", "socket_summary", New, | ||
| mb.WithNamespace("system.socket.summary"), |
There was a problem hiding this comment.
@ruflin can this cause some kind of collision with the socket metricset or with its fields definition?
There was a problem hiding this comment.
In case we would also use summary in the socket metricset it would so we shouldn't. We do the same trick in some other metricsets to make it easier for the user to access the metrics.
Should we create a collision on day by accident, CI should detect multiple definitions of the same field.
| title: Socket summary | ||
| type: group | ||
| description: > | ||
| ss |
There was a problem hiding this comment.
ss -s, but users will probably appreciate a description that doesn't require to know the command 🙂
| "udp": common.MapStr{ | ||
| "connections": udpConns, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Maybe it'd be interesting to have also the total number of connections, and we could also get different stats for ipv4 and ipv6.
There was a problem hiding this comment.
@jsoriano I like that idea a lot. We don't have to add these necessarly in this PR but we should make sure the data structure is ready to be extended with these.
There was a problem hiding this comment.
Agree, this can be left for future enhancements
| // of an error set the Error field of mb.Event or simply call report.Error(). | ||
| func (m *MetricSet) Fetch(report mb.ReporterV2) { | ||
|
|
||
| conns, err := net.Connections("inet") |
There was a problem hiding this comment.
If we allow to parametrize this we could use this module with the possible kinds of connections we could use the same module to have stats also on unix sockets. This can be left for a future PR in any case.
|
I use this structure now: "system": {
"socket": {
"summary": {
"tcp": {
"all": {
"connections": 36,
"listening": 13
}
},
"udp": {
"all": {
"connections": 14
}
},
"all": {
"connections": 50,
"listening": 13
}
}
}Using this structure, I think we can easily extend to any other protocol we want. We can even group by We can add new protocols like |
|
Failing tests seem related, please check https://travis-ci.org/elastic/beats/jobs/364709748#L4444 Regarding the new fields structure, I think it's fine, maybe I'd use |
|
jenkins, test it |
|
I think it errored due to one of the new fields returning an unexpected value in the integration tests. I'll take a look at them. |
|
@agathver Could you also rebased on the most recent master. We had some issue on CI yesterday. |
jsoriano
approved these changes
Apr 13, 2018
|
jenkins, test it please |
|
@agathver let's revive this PR 🙂 |
|
@jsoriano Done! |
|
It seems since this is merged, the windows tests have become flaky: https://beats-ci.elastic.co/job/elastic+beats+master+multijob-windows/beat=metricbeat,label=windows/1321/console |
|
Here is the log message output: |
|
I opened #7868 to track this. |
|
Skipping the test on Windows as the required gopsutil method is indeed not implemented for Windows (#7869) |
jsoriano
pushed a commit
to jsoriano/beats
that referenced
this pull request
Aug 6, 2018
(cherry picked from commit c9f2e19)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #4474
#6646
Added new socket summary metrics under
system.socket.summary.*