-
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
Send available perfmon data on error #6542
Send available perfmon data on error #6542
Conversation
When the windows/perfmon metricset encountered any error it would discard any valid metrics it had read and send a single error event. With this change it will send one event per counter. If an individual counter returns an error then it will send an event for that counter containing the error.message field and a zero-value measurement field. Depending on your use case you may want to update any metric aggregations you have to ignore error events. I'm also adding more documentation for the configuration options.
}, nil | ||
} | ||
|
||
func (m *MetricSet) Fetch() ([]common.MapStr, error) { | ||
data, err := m.reader.Read() | ||
func (m *MetricSet) Fetch(report mb.ReporterV2) { |
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.
exported method MetricSet.Fetch should have comment or be unexported
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.
For my understanding: As long as all counters exist, nothing changes from the previous behaviour. In case 2 counters out of 10 have an error, 2 error events and 8 normal events are sent now?
Could you add a CHANGELOG entry?
} | ||
|
||
if val.Instance != "" { | ||
event.MetricSetFields.Put(r.instanceLabel[counterPath], val.Instance) |
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.
@exekias I wonder if this would also be a place to use safteput. Also below.
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 have much knowledge about Windows perfom metrics, in general safemapstr.Put
would be required when we know the field may contain dots and these don't necessarily imply nesting. Basically when field names can cause a nesting issue like this:
field1.total: 3
field1: "foo"
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 believe safemapstr.Put
is very applicable here (I did consider it during this fix). The user has full control over the field names plus there is only one metric in an event.
Updated with a CHANGELOG entry. |
Correct. And if a counter does not exist, instead of getting a single error event you will get one error event per counter PLUS an event per each valid counter. |
When the windows/perfmon metricset encountered any error it would discard any valid metrics it had read and send a single error event. With this change it will send one event per counter. If an individual counter returns an error then it will send an event for that counter containing the error.message field and a zero-value measurement field. Depending on your use case you may want to update any metric aggregations you have to ignore error events.
I'm also adding more documentation for the configuration options.