-
Notifications
You must be signed in to change notification settings - Fork 487
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
[windows exporter] Clarify the use of use_api in docs. #6603
Conversation
From a windows perspective looks good, @clayton-cornell can add context around the verbiage itself. |
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 reasonable to me
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.
Some suggestions to expand the text just a little
|
||
The `where_clause` argument can be used to limit the response to the services you specify, reducing the size of the response. | ||
If `use_api` is enabled, 'where_clause' won't be effective. | ||
|
||
The Windows API is more performant than WMI. Set `use_api` to `true` in situations when the WMI takes too long to get the service information. |
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.
The Windows API is more performant than WMI. Set `use_api` to `true` in situations when the WMI takes too long to get the service information. | |
The WMI is slow to query data, but it provides detailed results. The Windows API is fast but you must make several API calls to get the same amount of information. If speed is important and the WMI takes too long to retrieve information, you can set `use_api` to `true` to use the API to query your data. |
I gave it a go at tweaking the text here a little to add a little more contextual info and details. Just saying the API is more performant doesn't tell you very much.
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.
TBH I think the extra text feels a bit vague and creates more questions than answers.
The WMI is slow to query data, but it provides detailed results.
Why is it more detailed?
The Windows API is fast but you must make several API calls to get the same amount of information.
Do the number of API calls matter? And I'm not sure where we got this information from?
I'm personally not an expert on this exporter so I'd prefer not to add too much information that I'm not sure about 😅
|
||
The `where_clause` argument can be used to limit the response to the services you specify, reducing the size of the response. | ||
If `use_api` is enabled, 'where_clause' won't be effective. | ||
|
||
The Windows API is more performant than WMI. Set `use_api` to `true` in situations when the WMI takes too long to get the service information. | ||
Setting `use_api` to `true` does have a few disadvantages compared to using WMI: | ||
* WMI queries in `where_clause` won't work. |
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.
Is there any alternative here? The second bullet provides an alternate solution.. so I'm wondering what happens in this case.
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 don't think there are alternatives. But I also don't think it's worth elaborating too much on this, since it seems natural that the "WMI queries" feature won't work when "WMI" is disabled.
The second bullet point is not related to WMI queries IIUC.
docs/sources/flow/reference/components/prometheus.exporter.windows.md
Outdated
Show resolved
Hide resolved
This PR has not had any activity in the past 30 days, so the |
This PR has not had any activity in the past 30 days, so the |
@mattdurham @ptodev Is this still valid? Can it be merged? |
I am good with this, looks like @ptodev has a few doc changes requested? |
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
fea3554
to
f028491
Compare
This PR has not had any activity in the past 30 days, so the |
I tried to add as much information as possible, based on what I understand from the upstream PR. It would be good if someone with knowledge of the Windows exporter double checks my wording.