Skip to content
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

Merged
merged 2 commits into from
Jan 16, 2017

Conversation

cbuescher
Copy link
Member

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.

@cbuescher cbuescher added :Search/Search Search-related issues that do not fall into other categories >enhancement review v5.3.0 labels Jan 16, 2017
…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.
@cbuescher cbuescher force-pushed the machineReadableProfileTime_5x branch from 1deb6f3 to 1ba2692 Compare January 16, 2017 14:36
@cbuescher
Copy link
Member Author

This is a follow up to #22561 against the 5.x branch. Here we want to keep the old "time" field format and always add the addtional "time_in_nanos" field. Adding a note to the docs that the time field will not be printed by default anymore with the next major version.

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"`
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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

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

Copy link
Member Author

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
Copy link
Member

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 ?

Copy link
Member Author

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.
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

Copy link
Member

@javanna javanna left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories v5.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants