Skip to content

top_metrics stabilization #51813

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

Closed
nik9000 opened this issue Feb 3, 2020 · 7 comments · Fixed by #53521
Closed

top_metrics stabilization #51813

nik9000 opened this issue Feb 3, 2020 · 7 comments · Fixed by #53521
Assignees
Labels

Comments

@nik9000
Copy link
Member

nik9000 commented Feb 3, 2020

We're getting pretty close to merging the new top_metrics aggregation (#51155). It'll be marked "experimental" because we will be looking into a few features soon that, if we can't implement them, will change the response format:

  • Support a size parameter to return the top few metrics. Not many though.
  • Support returning more than a single metric field.
@nik9000 nik9000 added the :Analytics/Aggregations Aggregations label Feb 3, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

@nik9000 nik9000 self-assigned this Feb 3, 2020
@nik9000 nik9000 added the >docs General docs changes label Feb 20, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs (>docs)

nik9000 added a commit to nik9000/elasticsearch that referenced this issue Feb 21, 2020
This adds support for returning the top "n" metrics instead of just the
very top.

Relates to elastic#51813
nik9000 added a commit that referenced this issue Feb 27, 2020
This adds support for returning the top "n" metrics instead of just the
very top.

Relates to #51813
@nik9000
Copy link
Member Author

nik9000 commented Feb 27, 2020

Another question: should top_metrics preserve the type of the metrics or continue to cast them all to double? It does preserve the type of the sort, but not the metrics.

@polyfractal
Copy link
Contributor

Hmmmm. That's a good question.

Preserve type

  • Most equivalent to top_hits (e.g. you want the original value from the doc), albeit it potentially a little different due to docvalues vs source

Cast to Double

  • Plays nice with pipeline aggs
  • Consistent with rest of aggs

Perhaps we could have something similar to "_as_string" but for the type? foo_as_original_type or whatever, alongside with the value cast to a double? Then pipeline aggs could fetch the double value, and consumers of the response could choose?

@nik9000
Copy link
Member Author

nik9000 commented Feb 27, 2020

The pipeline agg can fetch the double either way, I think. The trick is whether we preserve the type when we accumulate them and when we render them. Without that, for example, dates will look like millis since epoch. OTOH, no other agg does this. But it sounds like a good thing to do.

I'm happy to wait some on that too, but I figure it'd be nice to decide while we've got the experimental label.

nik9000 added a commit to nik9000/elasticsearch that referenced this issue Feb 27, 2020
This adds support for returning the top "n" metrics instead of just the
very top.

Relates to elastic#51813
nik9000 added a commit that referenced this issue Feb 27, 2020
This adds support for returning the top "n" metrics instead of just the
very top.

Relates to #51813
@nik9000
Copy link
Member Author

nik9000 commented Mar 4, 2020

I'm happy to wait some on that too, but I figure it'd be nice to decide while we've got the experimental label.

I talked to folks a bit and decided that, yes, I should return variables in their original type. In this case that means preserving their "formatter" and deciding between double and long. I don't support keyword fields so there is nothing to do there.

@nik9000
Copy link
Member Author

nik9000 commented Mar 9, 2020

I've opened #53288 for preserving types. If we're good with that I think all that is left for this is for me to take a fresh eye to the docs.

nik9000 added a commit to nik9000/elasticsearch that referenced this issue Mar 12, 2020
* Removes experimental.
* Replaces `"v"` (for value) with `"m"` (for metric).
* Move the note about tiebreaking into the list of limitations of the
  sort.
* Explain how you ask for `metrics`.
* Clean up some wording.
* Link to the docs from `top_metrics`.

Closes elastic#51813
nik9000 added a commit that referenced this issue Mar 16, 2020
* Removes experimental.
* Replaces `"v"` (for value) with `"m"` (for metric).
* Move the note about tiebreaking into the list of limitations of the
  sort.
* Explain how you ask for `metrics`.
* Clean up some wording.
* Link to the docs from `top_metrics`.

Closes #51813
nik9000 added a commit to nik9000/elasticsearch that referenced this issue Mar 16, 2020
* Removes experimental.
* Replaces `"v"` (for value) with `"m"` (for metric).
* Move the note about tiebreaking into the list of limitations of the
  sort.
* Explain how you ask for `metrics`.
* Clean up some wording.
* Link to the docs from `top_metrics`.

Closes elastic#51813
nik9000 added a commit that referenced this issue Mar 16, 2020
* Removes experimental.
* Replaces `"v"` (for value) with `"m"` (for metric).
* Move the note about tiebreaking into the list of limitations of the
  sort.
* Explain how you ask for `metrics`.
* Clean up some wording.
* Link to the docs from `top_metrics`.

Closes #51813
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants