Introduce event.duration and event.dataset for backward compatiblity#9393
Introduce event.duration and event.dataset for backward compatiblity#9393ruflin merged 5 commits intoelastic:6.xfrom
Conversation
|
Pinging @elastic/infrastructure |
|
jenkins, test this |
|
In the description, you meant to say the current |
|
I'm pretty sure the test failures are legit, because they're the same on Travis and on Jenkins. So perhaps related to the incorrect field type. |
|
jenkins, test this |
|
@webmat This should be ready to go now. |
The field event.duration is introduced as a replacement for metricset.rtt. event.duration is in nano seconds, metricset.rtt in nano seconds so the two are not compatible. `metricset.rtt` will be removed in 7.0.
`event.dataset` is introduced as `{module}.{metricset}` for better backward compatiblity in 7.0.
Further changes
* Fix tests
5279894 to
09e70f8
Compare
webmat
left a comment
There was a problem hiding this comment.
Almost got to LGTM, only one missing thing I noticed. The metricset.rtt is not actually marked as deprecated in the documentation.
I also have a nit about an innocuous race condition in setting event.duration vs metricset.rtt.
| if event.Took > 0 { | ||
| // rtt is deprecated and will be removed in 7.0. Replaced by event.duration. | ||
| info.Put("metricset.rtt", event.Took/time.Microsecond) | ||
| info.Put("event.duration", event.Took/time.Nanosecond) |
There was a problem hiding this comment.
Nit: here you have an innocuous race condition. The time.Nanosecond could happen later enough that trunc(event.duration) != metricset.rtt. It would be cleaner to populate event.duration, then truncate it to populate metricset.rtt.
There was a problem hiding this comment.
Not sure i can follow. event.Took is a value and it's not calculated here.
There was a problem hiding this comment.
Ah, didn't look up what event.Took was, sorry. Never mind this :-)
| *Metricbeat* | ||
|
|
||
| - Deprecate field `metricset.rtt`. Replaced by `event.duration` which is in nano instead of micro seconds. | ||
|
|
There was a problem hiding this comment.
There's no update to the definition for metricset.rtt to mark it as deprecated. I think it would make sense to add it in this PR. WDYT?
There was a problem hiding this comment.
Good point, added it to the fields definition.
| description: > | ||
| Name of the service metricbeat fetches the data from. | ||
|
|
||
| - name: event.duration |
There was a problem hiding this comment.
Just curious, why this is not named with the unit, like event.duration.ns?
There was a problem hiding this comment.
This field comes from ECS: https://github.com/elastic/ecs#event
A more general note: I have second thoughts for how we deal with units in our current convention and hope in the future Elasticsearch can partially deal with it: elastic/elasticsearch#31244
|
jenkins, test this |
The field event.duration is introduced as a replacement for metricset.rtt. event.duration is in nano seconds, metricset.rtt in nano seconds so the two are not compatible.
metricset.rttwill be removed in 7.0.event.datasetis introduced as{module}.{metricset}for better backward compatiblity in 7.0.Further changes