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

Add field network_names of hosts and virtual machines to vsphere module #5732

Merged
merged 1 commit into from
Nov 29, 2017
Merged

Add field network_names of hosts and virtual machines to vsphere module #5732

merged 1 commit into from
Nov 29, 2017

Conversation

amandahla
Copy link

@amandahla amandahla commented Nov 28, 2017

Add a new field "network_names" to metrics "host" and "virtualmachine".

Closes #5646

@elasticmachine
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Contributor

@exekias exekias 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! I left some comments 😄

@@ -68,6 +68,9 @@ https://github.com/elastic/beats/compare/v6.0.0-beta2...master[Check the HEAD di
- Fix error `datastore '*' not found` in Vsphere module. {issue}4879[4879]
- Fix error `NotAuthenticated` in Vsphere module. {issue}4673[4673]
- Fix connection leak in mongodb module. {issue}5688[5688]
- Support `npipe` protocol (Windows) in Docker module. {pull}4751[4751]
- Added missing mongodb configuration file to the `modules.d` folder. {pull}4870[4870]
- Fix wrong MySQL CRUD queries timelion visualization {pull}4857[4857]
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like these items are spurious, can you remove them?

// limitations under the License.

// Package pbutil provides record length-delimited Protocol Buffer streaming.
package pbutil
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to remove this?

// See the License for the specific language governing permissions and
// limitations under the License.

package types
Copy link
Contributor

Choose a reason for hiding this comment

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

probably this is an accidental removal

@@ -35,3 +35,8 @@
description: >
Free Memory in bytes
format: bytes
- name: network_names
type: object
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you go for object? I'm wondering if this should be keyword as this will hold an array?

@amandahla
Copy link
Author

@exekias I think that I did something really crazy with rebase...But now it's fixed (I hope so) and I changed fields.yml like you suggested.

@ruflin ruflin changed the title Fix #5646 Add field network_names of hosts and virtual machines to vsphere module #5646 Nov 28, 2017
@ruflin ruflin changed the title Add field network_names of hosts and virtual machines to vsphere module #5646 Add field network_names of hosts and virtual machines to vsphere module Nov 28, 2017
}

for _, net := range nets {
name := strings.Replace(net.Name, ".", "_", -1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably should make this a helper function for in beats to "normalise" keys.

Copy link
Author

Choose a reason for hiding this comment

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

This would be great!

Copy link
Collaborator

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Change looks good to me. I had to try if we set it to keyword if it still works if one entry has only 1 network_names and others have multiple. It seems to work as expected. 👍

@ruflin
Copy link
Collaborator

ruflin commented Nov 28, 2017

@exekias I leave this one up to you again to approve / merge.

Copy link
Contributor

@exekias exekias 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 again for all your contributions @amandahla , you rock! 🚀 🎸

@exekias exekias merged commit e70be71 into elastic:master Nov 29, 2017
@amandahla
Copy link
Author

Thaaanks! 💃 😄

@adelbot
Copy link

adelbot commented Dec 4, 2017

How modify ulimit for metricbeat ?
sockets : Too many open files

Many messages : [vsphere] error retrieving network from virtual machine: object references must have the same type

So, then network name is ok like :
"network_names": [
"TRZ_1009_ADMI001-G",
"TRZ_0514_ADMV001-G"
]

@ruflin
Copy link
Collaborator

ruflin commented Dec 5, 2017

@adelbot How is ulimit related to the issue above? Should we take this to discuss https://discuss.elastic.co/c/beats/metricbeat ?

@amandahla Perhaps you know more about the error Many messages : [vsphere] error retrieving network from virtual machine: object references must have the same type?

@amandahla
Copy link
Author

@ruflin Sorry for the delay, I was traveling on vacation.
I opened the issue #5961 to fix this problem. I made it in "host" but forgot to replicate on "virtualmachine"

About "sockets : Too many open files", I'm afraid that vsphere module open too many connections to collect. It shouldn't be a problem because we use "defer client.Logout(ctx)" and the definition says that "Close any idle connections after logging out."
@adelbot Can you tell if this is happening? Are too many open connections to your vsphere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants