-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
feat: add win_wmi input plugin #11250
Conversation
Why do the linux-based tests complain with |
Hi! Thanks for this PR!
The build constraint as-is will only build for Windows, yet the module is always imported with all.go. To get around this we typically create a |
That worked! Thank you. All tests are passing now. |
Hi @julesroussel3, thanks for looking into this. Does your new plugin gather different data than telegraf's existing inputs/win_perf_counters? That plugin uses Windows's PDH API which seems to grab the same counters as WMI. I ran into a few mentions that say PDH is a lower level API so it performs better than WMI: |
Hi @reimda ! Thank you for the question. I’m happy to clarify.
It is true that some performance counters are exposed through WMI, and it is true that accessing performance counters directly with the win_perf_counters plugin is optimal. However, WMI provides access to lots of other information which is not available through performance counters. Configuration and other metadata can be gathered using WMI. For example:
Furthermore, WMI provides ultimate control. Some of the basic input plug-ins (e.g. cpu, mem) provide abstraction layers to WMI-based data, but do not provide the ultimate flexibility and control that some administrators desire. This plug-in enables an administrator to granularly select each property of a class which they desire to include in their metrics. It's possible that most administrators will choose to use the stock plugins and win_perf_counters, but the lack of a flexible WMI plug-in was a major roadblock in the enterprise environment I support, which is how this plug-in came to be. |
@julesroussel3 AFAIK (but not sure) you can gather data remotely with WMI so if this plugin also provides that functionality, it will definitely have an edge over existing plugins. |
@cemremengu That is a good idea for a feature enhancement and is certainly possible. Similar to #11090, for that to work, either the computer object or service account running telegraf would need to have administrative permissions on the remote machine. Unfortunately, when this plug-in was created, remote support wasn't in scope for my requirements. |
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.
I took a quick look at the code and had some thoughts/questions before getting into a close review.
Hello! I am closing this issue due to inactivity. I hope you were able to resolve your problem, if not please try posting this question in our Community Slack or Community Page. Thank you! |
any way to re-open and get this capability merged? |
@julesroussel3 feel free to implement the requested changes and reply to the questions, we will then reopen and review. |
Hello! I am closing this issue due to inactivity. I hope you were able to resolve your problem, if not please try posting this question in our Community Slack or Community Page. Thank you! |
inactivity? really? |
Can this get reopened/merged? |
If @julesroussel3 has interest in continue working on it sure |
I am actively working toward addressing review feedback. |
Thanks I will take a look tomorrow
A rebase on master should have a change that only tests the READMEs that are touched. Otherwise, we can ignore. |
Thanks for seeing this through. I'll let Sven take one final look at this. |
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.
Thanks for the nice plugin @julesroussel3! I do have some smaller comments though. Can you please take a look!?
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
@julesroussel3 any updates here? I really would love to see this PR merged... |
Hi folks! I made another pass at review comments. Let me know if this is satisfactory for merge. |
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.
All good, just a minor improvement.
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.
@julesroussel3 I think we are almost there. I have three minor comments for the test and I agree with @Hipska about the example. Can you please fix those so we get that nice plugin in?
I've addressed the latest feedback. Let me know what's next. |
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
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.
Looks good to me. Thanks for your great work and for keeping this going @julesroussel3!
@powersj as we have quite some changes, please take another look and merge if you are fine with the changes.
Required for all PRs:
resolves #1857
This PR creates a plugin to obtain metrics from Windows Management Instrumentation (WMI).
This plugin queries Windows Management Instrumentation (WMI) classes on Windows
operating systems. An administrator can query and filter for specific
properties in any WMI namespace and class. While some system information is
already exposed through regular plugins like [[inputs.cpu]] and [[inputs.mem]],
this plugin provides ultimate control and flexibility, allowing an
administrator to query classes not exposed through other plugins.
Full details of the plugin are available in the README.