Introduce event.duration and event.dataset for backward compatiblity #9393
Conversation
ruflin
added
v5.6.0
v6.6.0
Team:Integrations
|
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
ruflin
force-pushed
the
introduce-dataset-fields
branch
from
5279894
to
09e70f8
Compare
| 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 :-)
| @@ -131,6 +132,8 @@ https://github.com/elastic/beats/compare/v6.5.0...6.x[Check the HEAD diff] | |||
|
|
|||
| *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.
| @@ -38,3 +39,13 @@ | |||
| example: elasticsearch | |||
| 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