Add metricset.period to Metricbeat events #13242
Conversation
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.
jsoriano
added
enhancement
in progress
jsoriano
force-pushed
the
add-period
branch
2 times, most recently
from
ac8ef56 to
f403bd1
Compare
jsoriano
added
review
[zube]: In Review
and removed
[zube]: In Progress
in progress
|
jenkins, test this again please |
metricbeat/module/apache/status/_meta/testdata/docs.plain-expected.json
Outdated
Show resolved
Hide resolved
| "p99": 0, | ||
| "p999": 0, | ||
| "stddev": 0 | ||
| "my_counter": { |
There was a problem hiding this comment.
Why does it modify this one?
There was a problem hiding this comment.
Order of events depends on a hash of the whole event. For source files that generate multiple events is very likely that the order changes if anything in the content changes, so diffs are bigger.
metricbeat/mb/event.go
Outdated
| @@ -141,6 +150,9 @@ func AddMetricSetInfo(module, metricset string, event *Event) { | |||
| if event.Took > 0 { | |||
| e.Put("event.duration", event.Took/time.Nanosecond) | |||
| } | |||
| if event.Period > 0 { | |||
| e.Put("metricset.period", event.Period/time.Microsecond) | |||
There was a problem hiding this comment.
I understand why you want here with microseconds instead of nano seconds as it might be overkill. At the same time I wonder if we should standardise the unit we use?
Otherwise, do we expected / recommend anyhting lower then 1s?
There was a problem hiding this comment.
I understand why you want here with microseconds instead of nano seconds as it might be overkill. At the same time I wonder if we should standardise the unit we use?
I don't have a strong opinion about this, it comes from the original PR 🙂
By standardizing do you mean to use nanoseconds as in event.duration? I would be ok with that, but I don't think they need to be in the same unit as they have different needings.
Otherwise, do we expected / recommend anyhting lower then 1s?
I don't think it will be recommendable to have a period lower than the second, but there are services that can very well respond in milliseconds, so it should be possible.
Another option can be to store it in seconds, but as a float, this way we would need less space in events and in store (32 bits for a float, 64 for a long), and we could still support the case of periods under the second.
There was a problem hiding this comment.
I definitively prefer the long over the float as the float gets inaccurate in ES for large values (not that we will really hit those hopefully). I'm still holding my breath for getting duration support in ES one day ;-)
I'm good with Micro or either Milli and we solve it in case it ever becomes an issue.
There was a problem hiding this comment.
Ok, I think I am going to change this to Milliseconds, it should be more than enough for this. And then also integer should be enough to store the values.
|
jenkins, test this again please |
|
I also feel that the change to milliseconds is the way to go. Reviewed again. Everything looks ok |
Add metricset.period to Metricbeat events so this information
can be used for more accurate visualizations in Kibana UIs.
This field is not added to push fetchers, as they don't do
periodic fetching.
Continues with #6929
Fixes #12616
Co-authored-by: ruflin spam@ruflin.com