-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix Metricbeat/vsphere - Error "datastore '*' not found" #4879 #4883
Conversation
Can one of the admins verify this patch? |
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically on build-eu-00. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
ok to test |
Could you please add a CHANGELOG entry? Also, do you know if the new client version may help on this? #4673, have you experienced any issue like that? |
jenkins, test it |
@exekias I updated the govmomi (0.15.0 now) and changed the way to collect. For the issue #4673 , I think that is possible to use something like this vmware/govmomi#224 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is looking good, as I commented on the issue, I think it may make sense to switch from Client to Session. Let me know what you think
ok to test. @amandahla is this ready for a new review? |
@exekias Yes. Just had that issue with the flag 'version'. |
code looks good to me, there are some issues building it for other systems, do you have any idea of what's going on there? https://travis-ci.org/elastic/beats/jobs/265294456 |
I don't have idea why but...
I didn't know that. What are the options now? To not use session? :-| |
I'll have a look into the issue |
|
||
for _, hs := range hst { | ||
totalCpu := int64(hs.Summary.Hardware.CpuMhz) * int64(hs.Summary.Hardware.NumCpuCores) | ||
freeCpu := int64(totalCpu) - int64(hs.Summary.QuickStats.OverallCpuUsage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var freeCpu should be freeCPU
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved.
} | ||
|
||
for _, hs := range hst { | ||
totalCpu := int64(hs.Summary.Hardware.CpuMhz) * int64(hs.Summary.Hardware.NumCpuCores) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var totalCpu should be totalCPU
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved.
@amandahla @exekias What is the status here? @amandahla Any chance you could rebase this on master and then we could have a look again. |
I had a look at the issue but couldn't get it working, looks like the client lacks support for many architectures :/ |
Using session we were getting errors with architectures but now opening a new client on every fetch seems ok. It's the same way like before but instead doing a new connection only once, we do it for every fetch. |
CHANGELOG.asciidoc
Outdated
@@ -65,6 +65,11 @@ https://github.com/elastic/beats/compare/v6.0.0-beta2...master[Check the HEAD di | |||
- Fix the loading of 5.x dashboards. {issue}5277[5277] | |||
- Fix the fetching of process information when some data is missing under MacOS X. {issue}5337[5337] | |||
- Change `MySQL active connections` visualization title to `MySQL total connections`. {issue}4812[4812] | |||
- Support `npipe` protocol (Windows) in Docker module. {pull}4751[4751] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need a rebase / cleanup.
@amandahla Could you rebase this one again for the changelog? |
@amandahla I did 2 commits myself to see if I can fix the CHANGELOG issue and getting this in. |
There was a problem hiding this 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.
@exekias Could you have a look again and merge if ok?
@ruflin Thanks! |
Awesome contribution @amandahla, I'm glad this got finally in!! 🎉 🎉 |
@exekias Thank you, I'm very happy with that! 😄 |
We needed to change the way we use govmomi for collect metrics and now it works with versions 6.0/6.5.
We have to remove 'datacenter' field also.
Config.yml was corrected.