-
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
Refactoring config handling for perfmon metricset #3854
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,19 @@ | ||
{ | ||
"@timestamp":"2016-05-23T08:05:34.853Z", | ||
"beat":{ | ||
"hostname":"beathost", | ||
"name":"beathost" | ||
"@timestamp": "2016-05-23T08:05:34.853Z", | ||
"beat": { | ||
"hostname": "beathost", | ||
"name": "beathost" | ||
}, | ||
"metricset":{ | ||
"host":"localhost", | ||
"module":"mysql", | ||
"name":"status", | ||
"rtt":44269 | ||
"metricset": { | ||
"module": "windows", | ||
"name": "perfmon", | ||
"rtt": 0 | ||
}, | ||
"windows":{ | ||
"perfmon":{ | ||
"example": "perfmon" | ||
"type": "metricsets", | ||
"windows": { | ||
"perfmon": { | ||
"processor.processor_performance": 52.522315, | ||
"processor.processor_time": 18.287520 | ||
} | ||
}, | ||
"type":"metricsets" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,26 +1,18 @@ | ||
- name: perfmon | ||
type: group | ||
fields: | ||
- name: counters | ||
- name: perfmon.counters | ||
type: group | ||
description: > | ||
Grouping of different counters | ||
fields: | ||
- name: group | ||
Grouping of different counters | ||
fields: | ||
- name: alias | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the alias and query going to be part of the event? Based on your example object above this is not the case. Means we don't need it in here? I think this file can actually be empty as the name is defined by the namespace. |
||
type: string | ||
description: > | ||
Name of the group. For example `processor` or `disk` | ||
|
||
- name: collectors | ||
type: group | ||
fields: | ||
- name: alias | ||
type: string | ||
description: > | ||
Short form for the query | ||
|
||
- name: query | ||
type: string | ||
description: > | ||
The query. For example `\\Processor Information(_Total)\\% Processor Performance`. Backslashes have to be escaped. | ||
Short form for the query | ||
|
||
- name: query | ||
type: string | ||
description: > | ||
The query. For example `\\Processor Information(_Total)\\% Processor Performance`. Backslashes have to be escaped. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,12 +13,7 @@ type Handle struct { | |
status error | ||
query uintptr | ||
counterType int | ||
counters []CounterGroup | ||
} | ||
|
||
type CounterGroup struct { | ||
GroupName string | ||
Counters []Counter | ||
counters []Counter | ||
} | ||
|
||
type Counter struct { | ||
|
@@ -41,24 +36,21 @@ var errorMapping = map[PdhError]string{ | |
PDH_STATUS_NO_OBJECT: `PDH_STATUS_NO_OBJECT`, | ||
} | ||
|
||
func GetHandle(config []CounterConfig) (*Handle, PdhError) { | ||
func GetHandle(config []CounterConfigGroup) (*Handle, PdhError) { | ||
q := &Handle{} | ||
err := _PdhOpenQuery(0, 0, &q.query) | ||
if err != ERROR_SUCCESS { | ||
return nil, PdhError(err) | ||
} | ||
|
||
counterGroups := make([]CounterGroup, len(config)) | ||
q.counters = counterGroups | ||
counters := make([]Counter, len(config)) | ||
q.counters = counters | ||
|
||
for i, v := range config { | ||
counterGroups[i] = CounterGroup{GroupName: v.Name, Counters: make([]Counter, len(v.Group))} | ||
for j, v1 := range v.Group { | ||
counterGroups[i].Counters[j] = Counter{counterName: v1.Alias, counterPath: v1.Query} | ||
err := _PdhAddCounter(q.query, counterGroups[i].Counters[j].counterPath, 0, &counterGroups[i].Counters[j].counter) | ||
if err != ERROR_SUCCESS { | ||
return nil, PdhError(err) | ||
} | ||
counters[i] = Counter{counterName: v.Alias, counterPath: v.Query} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 It looks less complex with fewer loops. |
||
err := _PdhAddCounter(q.query, counters[i].counterPath, 0, &counters[i].counter) | ||
if err != ERROR_SUCCESS { | ||
return nil, PdhError(err) | ||
} | ||
} | ||
|
||
|
@@ -82,28 +74,24 @@ func (q *Handle) ReadData(firstFetch bool) (common.MapStr, PdhError) { | |
result := common.MapStr{} | ||
|
||
for _, v := range q.counters { | ||
groupVal := make(map[string]interface{}) | ||
for _, v1 := range v.Counters { | ||
err := _PdhGetFormattedCounterValue(v1.counter, PdhFmtDouble, q.counterType, &v1.displayValue) | ||
if err != ERROR_SUCCESS { | ||
switch err { | ||
case PDH_CALC_NEGATIVE_DENOMINATOR: | ||
{ | ||
//Sometimes counters return negative values. We can ignore this error. See here for a good explanation https://www.netiq.com/support/kb/doc.php?id=7010545 | ||
groupVal[v1.counterName] = 0 | ||
continue | ||
} | ||
default: | ||
{ | ||
return nil, PdhError(err) | ||
} | ||
err := _PdhGetFormattedCounterValue(v.counter, PdhFmtDouble, q.counterType, &v.displayValue) | ||
if err != ERROR_SUCCESS { | ||
switch err { | ||
case PDH_CALC_NEGATIVE_DENOMINATOR: | ||
{ | ||
//Sometimes counters return negative values. We can ignore this error. See here for a good explanation https://www.netiq.com/support/kb/doc.php?id=7010545 | ||
result[v.counterName] = 0 | ||
continue | ||
} | ||
default: | ||
{ | ||
return nil, PdhError(err) | ||
} | ||
} | ||
doubleValue := (*float64)(unsafe.Pointer(&v1.displayValue.LongValue)) | ||
groupVal[v1.counterName] = *doubleValue | ||
|
||
} | ||
result[v.GroupName] = groupVal | ||
doubleValue := (*float64)(unsafe.Pointer(&v.displayValue.LongValue)) | ||
|
||
result[v.counterName] = *doubleValue | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should use here |
||
} | ||
return result, 0 | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,11 +10,6 @@ import ( | |
"github.com/elastic/beats/metricbeat/mb" | ||
) | ||
|
||
type CounterConfig struct { | ||
Name string `config:"group" validate:"required"` | ||
Group []CounterConfigGroup `config:"collectors" validate:"required"` | ||
} | ||
|
||
type CounterConfigGroup struct { | ||
Alias string `config:"alias" validate:"required"` | ||
Query string `config:"query" validate:"required"` | ||
|
@@ -34,7 +29,7 @@ func init() { | |
// multiple fetch calls. | ||
type MetricSet struct { | ||
mb.BaseMetricSet | ||
counters []CounterConfig | ||
counters []CounterConfigGroup | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that this in the MetricSet, but do we ever need it outside the New part? It seems to be all in the handle/query anyways? |
||
handle *Handle | ||
firstFetch bool | ||
} | ||
|
@@ -46,22 +41,22 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) { | |
logp.Warn("BETA: The perfmon metricset is beta") | ||
|
||
config := struct { | ||
CounterConfig []CounterConfig `config:"perfmon.counters" validate:"required"` | ||
CounterConfigGroup []CounterConfigGroup `config:"perfmon.counters" validate:"required"` | ||
}{} | ||
|
||
if err := base.Module().UnpackConfig(&config); err != nil { | ||
return nil, err | ||
} | ||
|
||
query, err := GetHandle(config.CounterConfig) | ||
query, err := GetHandle(config.CounterConfigGroup) | ||
|
||
if err != ERROR_SUCCESS { | ||
return nil, errors.New("initialization fails with error: " + err.Error()) | ||
} | ||
|
||
return &MetricSet{ | ||
BaseMetricSet: base, | ||
counters: config.CounterConfig, | ||
counters: config.CounterConfigGroup, | ||
handle: query, | ||
firstFetch: true, | ||
}, nil | ||
|
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.
Not an issue for this PR but I assume this currently manually created? We should create this automatically in the future and make sure in the docs it is made clear that it is only an example.