-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
ProfileResult and CollectorResult should print machine readable timing information #22638
ProfileResult and CollectorResult should print machine readable timing information #22638
Conversation
…g information Currently both ProfileResult and CollectorResult print the timing field in a human readable string format (e.g. "time": "55.20315000ms"). When trying to parse this back to a long value, for example to use in the planned high level java rest client, we can lose precision because of conversion and rounding issues. This change introduces the additional field `time_in_nanos` that prints the raw timing value in nanoseconds.
1deb6f3
to
1ba2692
Compare
This is a follow up to #22561 against the 5.x branch. Here we want to keep the old " |
of all children. | ||
|
||
The `"breakdown"` field will give detailed stats about how the time was spent, we'll look at | ||
that in a moment. Finally, the `"children"` array lists any sub-queries that may be present. Because we searched for two | ||
values ("search test"), our BooleanQuery holds two children TermQueries. They have identical information (type, time, | ||
breakdown, etc). Children are allowed to have their own children. | ||
|
||
NOTE: The `"time"` field is only intended for human consumption. If you need exact timing values use `"time_in nanos"`. The `"time"` |
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.
does it make sense to have the field names between both backticks and double quote?
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'm only following the way the field names are written in the previous paragraph here. It's not completely consitent in this part of the docs I think. Not sure if I should make this change bigger by changing it one way or the other in the whole document?
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 hand't noticed, don't bother, fine with me as-is
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'd prefer just backticks - it's what we do in the rest of the docs
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.
@clintongormley okay, I opened #22653 doing this the profile documentation.
of all children. | ||
|
||
The `"breakdown"` field will give detailed stats about how the time was spent, we'll look at | ||
that in a moment. Finally, the `"children"` array lists any sub-queries that may be present. Because we searched for two | ||
values ("search test"), our BooleanQuery holds two children TermQueries. They have identical information (type, time, | ||
breakdown, etc). Children are allowed to have their own children. | ||
|
||
NOTE: The `"time"` field is only intended for human consumption. If you need exact timing values use `"time_in nanos"`. The `"time"` | ||
field is currently printed by default, but this will change with the next major version where `"time_in_nanos"` will be |
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.
shall we state what the next major version is? 6.0.0 ?
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.
okay.
of all children. | ||
|
||
The `"breakdown"` field will give detailed stats about how the time was spent, we'll look at | ||
that in a moment. Finally, the `"children"` array lists any sub-queries that may be present. Because we searched for two | ||
values ("search test"), our BooleanQuery holds two children TermQueries. They have identical information (type, time, | ||
breakdown, etc). Children are allowed to have their own children. | ||
|
||
NOTE: The `"time"` field is only intended for human consumption. If you need exact timing values use `"time_in nanos"`. The `"time"` | ||
field is currently printed by default, but this will change with the next major version where `"time_in_nanos"` will be | ||
printed by default. The `"time"` field needs to be switched on by using the `?human=false` <<common-options, common options>> flag then. |
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 think this last sentence may be confusing as it's about how to migrate to 6.0.0. That should only be in the 6.0.0 migrate guide I think.
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.
agreed
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.
left a couple of comments in docs, LGTM otherwise
Currently both ProfileResult and CollectorResult print the timing field in a
human readable string format (e.g. "time": "55.20315000ms"). When trying to
parse this back to a long value, for example to use in the planned high level
java rest client, we can lose precision because of conversion and rounding
issues. This change introduces the additional field
time_in_nanos
that printsthe raw timing value in nanoseconds.