Add refresh list of perf counters at every fetch #13344
Conversation
narph
added
Team:Integrations
|
Hi @narph , |
| childQueries, err := r.query.ExpandWildCardPath(counter.Query) | ||
| if err == nil && len(childQueries) >= 1 && !strings.Contains(childQueries[0], "*") { | ||
| for _, v := range childQueries { | ||
| if err := r.query.AddCounter(v, counter, len(childQueries) > 1); err != nil { |
There was a problem hiding this comment.
Should we also remove counters at some moment to avoid leaks?
There was a problem hiding this comment.
In the line below query.Close() I am running the PdhCloseQuery function which should close all counters contained in the specified query, closes all handles related to the query, and should free all memory associated with the query.
https://docs.microsoft.com/en-us/windows/win32/api/pdh/nf-pdh-pdhclosequery
There was a problem hiding this comment.
But query.Close() is only executed if AddCounter fails. Shouldn't we do some cleanup of old counters during the normal operation?
There was a problem hiding this comment.
I have removed the close query and added the option to compare counters and remove older ones.
| childQueries, err := r.query.ExpandWildCardPath(counter.Query) | ||
| if err == nil && len(childQueries) >= 1 && !strings.Contains(childQueries[0], "*") { | ||
| for _, v := range childQueries { | ||
| if err := r.query.AddCounter(v, counter, len(childQueries) > 1); err != nil { |
There was a problem hiding this comment.
But query.Close() is only executed if AddCounter fails. Shouldn't we do some cleanup of old counters during the normal operation?
| if err == nil && len(childQueries) >= 1 && !strings.Contains(childQueries[0], "*") { | ||
| for _, v := range childQueries { | ||
| if err := r.query.AddCounter(v, counter, len(childQueries) > 1); err != nil { | ||
| r.query.Close() |
There was a problem hiding this comment.
RefreshCounterPaths() is run on every fetch after the first one. If AddCounter fails, r.query will be closed and never reopened. Will then all the subsequent fetches fail because the query is closed?
I think that we shouldn't close here, or if we do it, we should have some reopening logic during fetch.
There was a problem hiding this comment.
Good catch @jsoriano , I used to call OpenQuery at each refresh but moved it out, I have removed the close option.
| childQueries, err := r.query.ExpandWildCardPath(counter.Query) | ||
| if err == nil && len(childQueries) >= 1 && !strings.Contains(childQueries[0], "*") { | ||
| for _, v := range childQueries { | ||
| if err := r.query.AddCounter(v, counter, len(childQueries) > 1); err != nil { |
|
jenkins test this |
In the perfmon metricset the list of performance counters is calculated when the metricset starts, then cached.
If users use wildcards in their counter queries they expect to see counter values for the processes/instances that match those queries as well.
For this a refresh of the list should be added (maybe at every Fetch).
Should fix #13091
How to test: