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

Improve Top Hit aggregation "field" names #54460

Closed
timroes opened this issue Jan 10, 2020 · 9 comments
Closed

Improve Top Hit aggregation "field" names #54460

timroes opened this issue Jan 10, 2020 · 9 comments
Labels
Feature:Vis Editor Visualization editor issues good first issue low hanging fruit patch-worthy Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@timroes
Copy link
Contributor

timroes commented Jan 10, 2020

Currently the top hit aggregation in the visualize editor has two fields, labeled "Field", without any further explanation. This can be very confusing and it would make sense improving this form slightly:

image (2)

The first "Field" selector allows selecting which field the value should be taken from (and then also how to aggregate the value in case we get the last n (in the screenshot 100) values from this field. For numerical fields this can be an aggregation like avg/sum, for strings it can be concatenated.

The second "Field" is belonging with the "Order" and actually defining by which field the documents are sorted to retrieve the top n from it (often users will use a timestamp here descending to get the last x documents matching).

I would suggest we rename those labels slightly, to something like "Order Field" and "Value Field", and give them a help icon with a tooltip, giving some short description how they behave?

@cchaos @gchaps either of you have some input for this?

@timroes timroes added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Vis Editor Visualization editor issues labels Jan 10, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@tsullivan
Copy link
Member

The second "Field" is belonging with the "Order" and actually defining by which field the documents are sorted to retrieve the top n from it (often users will use a timestamp here descending to get the last x documents matching).

Thanks, I never would have guessed this!

@gchaps
Copy link
Contributor

gchaps commented Jan 10, 2020

Here's a suggestion:

Value field
The field to use to calculate the metrics aggregation.

Order field
The field from which to retrieve and order the top documents.

@timroes timroes added the good first issue low hanging fruit label Jan 18, 2020
@sulemanof
Copy link
Contributor

Hey!
This was broken in #42130 after introducing the FieldParamType, which set the FieldParamEditor as a component for all of field type params.
But after merging the #55612 this will be fixed back:

image

If the above suggestion will be still relevant, I'll create a PR after merging #55612

@wylieconlon
Copy link
Contributor

Looks like this is affecting users in 7.5, if I'm reading this correctly: https://discuss.elastic.co/t/kibanas-top-hits-aggregation-sort-by-field-broken/216971

@flash1293
Copy link
Contributor

@sulemanof can this issue be closed? Seems like #56367 fixed it, right?

@sulemanof
Copy link
Contributor

@flash1293
If the design team is OK with labels, and the suggestion to rename labels is not relevant after the fix:

Here's a suggestion:
Value field
The field to use to calculate the metrics aggregation.
Order field
The field from which to retrieve and order the top documents.

Then yes, it can be closed.

@flash1293
Copy link
Contributor

@gchaps Do you think the current state is fine?

@gchaps
Copy link
Contributor

gchaps commented Feb 27, 2020

@flash1293 Yes, I think the current state is ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Vis Editor Visualization editor issues good first issue low hanging fruit patch-worthy Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

No branches or pull requests

7 participants