Add metricset.period to each metricbeat event #6929
Conversation
ruflin
added
in progress
|
@andrewkroh Can you have a look and share your thoughts? It also seems to be time to get rid of some of the old interfaces to clean up the code. |
metricbeat/mb/event.go
Outdated
| @@ -27,7 +27,7 @@ type Event struct { | |||
|
|
|||
| // BeatEvent returns a new beat.Event containing the data this Event. It does | |||
| // mutate the underlying data in the Event. | |||
| func (e *Event) BeatEvent(module, metricSet string, modifiers ...EventModifier) beat.Event { | |||
| func (e *Event) BeatEvent(module, metricSet string, period time.Duration, modifiers ...EventModifier) beat.Event { | |||
There was a problem hiding this comment.
Instead of a new parameter here, I think a Period field inside of Event might be better. If Period is not set then the configured period is used.
There was a problem hiding this comment.
Will change it accordingly as it allows every event to set a period if needed.
ruflin
added
review
and removed
in progress
|
@andrewkroh I adjusted the implementation accordingly, rebased and squashed and it's ready for an other review. |
Having the period as part of each event makes it possible for Kibana or ML to predict when the next event is potentially missing or delayed based on the period of the previous events. It can always be that the period changed but as soon as the next event comes in, this can be used as the new expected period. Only the schedule events will contain the `metricset.period` field.
| - name: metricset.period | ||
| type: long | ||
| description: > | ||
| Current data collection period for this events in micro seconds. |
There was a problem hiding this comment.
s/micro seconds/microseconds/
| @@ -97,6 +98,9 @@ func AddMetricSetInfo(module, metricset string, event *Event) { | |||
| if event.Took > 0 { | |||
| info["rtt"] = event.Took / time.Microsecond | |||
| } | |||
| if event.Period > 0 { | |||
There was a problem hiding this comment.
Can you also update the godocs to mention period and show period in the sample JSON.
| @@ -308,6 +308,9 @@ func (r reporterV2) Event(event mb.Event) bool { | |||
| if event.Took == 0 && !r.start.IsZero() { | |||
| event.Took = time.Since(r.start) | |||
| } | |||
| if r.msw.Module().Config().Period > 0 { | |||
There was a problem hiding this comment.
This all LGTM, except that Period will be set to 10s even for the push metricsets (I think). So events from push metricsets will get a period and I'd like for that to not happen.
Can you check if this is the case? And if so, see if there is something else we can check to avoid adding period to push metricset events by default.
There was a problem hiding this comment.
I tried it with the http/server metricset and unfortunately you are right. Need to figure out how we can detect this.
There was a problem hiding this comment.
It should be possible from to determine from the msw value whether or not it's push or pull.
Maybe you could add an enum to the metricSetWrapper that identifies the type as being push vs pull. You'd want to do this rather than doing a type assertion on each event.
|
Set to |
|
@andrewkroh Didn't find here a good automatic solution yet. Best thing I came up with so far is to let every metricset decide if it wants to set the value or not. |
|
@andrewkroh Never had time to investigate this further. Interested to take this on? I think it would be helpful in general to have this info. @ycombinator Also relevant for stack-monitoring I think. |
|
@ruflin I think I could talk someone through the changes to the metricSetWrapper (rather than taking it on myself). There is existing code in that file that checks to see what type of MetricSet it is (push/pull) so it shouldn’t be too bad. |
|
Closing this for now as we didn't need this in the UI so far. We can tackle this later again if needed. |
|
Continuing with this in #13242 |
Having the period as part of each event makes it possible for Kibana or ML to predict when the next event is potentially missing or delayed based on the period of the previous events. It can always be that the period changed but as soon as the next event comes in, this can be used as the new expected period.
Only the schedule events will contain the
metricset.periodfield.