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

[Infra UI] Limit "group by" choices for Metrics Explorer UI based on metric selection #39613

Closed
tbragin opened this issue Jun 25, 2019 · 18 comments · Fixed by #43322
Closed

[Infra UI] Limit "group by" choices for Metrics Explorer UI based on metric selection #39613

tbragin opened this issue Jun 25, 2019 · 18 comments · Fixed by #43322
Assignees
Labels
Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services

Comments

@tbragin
Copy link
Contributor

tbragin commented Jun 25, 2019

Describe UX challenge:
In the Metrics Explorer, we auto-populate all available fields in the "group per" control. However, depending on the metric selection, only a small subset of fields will be relevant.

Describe recommended change in experience:
Only populate relevant selections for "group per". For Metricbeat, for instance, these will include fields that are captured for all beats and fields specific to that metricset. However, we likely will need to arrive at determining what fields are relevant dynamically because of how some modules work, and the fact that users have options to enable different processors when the use Metricbeat.

Options:

  1. Perform an "exists" query within the given time range for the selected metric fields, grab a small number of documents from this set (5?), then de-duplicate the fields from these documents to create a list of suggested group-by fields
  2. Hard-code a list of commonly grouped by ECS fields that we always show, along with any of the top-level module fieldsets included in the selected metrics (e.g. filtering on system.load.1 would result in all of the system.* fields being included. (This option could eventually get smarter and also include recently used fields per user, etc.)

Both options would have a "show all" escape hatch built in that would allow a user to get the experience that happens today.

Screenshots below show the current behavior we are trying to improve.

Screen Shot 2019-06-25 at 11 56 43 AM

Uploading Screen Shot 2019-06-25 at 11.56.49 AM.png…

@tbragin tbragin added discuss Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services labels Jun 25, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-logs-ui

@tbragin tbragin added the Feature:Metrics UI Metrics UI feature label Jun 25, 2019
@tbragin
Copy link
Contributor Author

tbragin commented Jun 25, 2019

Possibly related to more general "discuss PR" @simianhacker started, but limiting the discussion only to the "group per" selection: #36843

@jasonrhodes
Copy link
Member

@roncohen @exekias correct me if I'm wrong, but I think this work will have the same issues as the general metric field we want to limit, because even once we know which fields the user has selected in the metrics, we won't easily know the full set of possible "group by" fields that would correspond there, right?

I wonder if another first iteration approach is to do the best we can and move those fields to the top of the drop down, and then leave the rest below, in a sort of "Suggested Fields" / "All Fields" split…

@exekias
Copy link
Contributor

exekias commented Jun 26, 2019

That sounds correct @jasonrhodes, but once you have decided on a metric, the fields available for grouping should be pretty stable. Reason for this is that normally metrics always have the same set of labels, just different values on them.

Having the rest of fields available somehow could make sense, as you point out, as we may miss some fields with this trick. The only situation I can think of where this could happen is with docker/kubernetes labels. These are free form, so you can expect different set of labels depending on the service being monitored.

That said, if you check a good amount of documents in the last couple of minutes you should probably get most of the available fields.

@roncohen
Copy link
Contributor

roncohen commented Jun 26, 2019

@exekias what's the status on getting the timeseries.instance into metricbeat documents by default? we could use that to make sure we get one of each

@exekias
Copy link
Contributor

exekias commented Jun 26, 2019

It went in already as experimental, should be in 7.2, more details here: elastic/beats#10293

It needs to be enabled with timeseries.enabled: true. Rationale for that: let's play with it and make sure it's what we need in the UI side before hard-coding it as a mandatory field.

@jasonrhodes
Copy link
Member

Great, so my thought for an approach here is this:

  • Take the current list of selected metrics (e.g. system.load.15, system.load.1) and do an "exists" query on those fields, for documents in the currently selected time range (maybe with something likee "top_hits" so you get a few example docs?)
  • From the sample of docs, grab all of the fields
  • Use that list of fields to populate the "group_by" drop down

Does that sound like a sane first approach?

@roncohen
Copy link
Contributor

roncohen commented Jun 26, 2019

more or less, just making sure:

From the sample of docs, grab all of the fields

when the user (or we, on load) have selected a metric, then we need to do another query to get a sample of documents that have that specific field. We can't use the same sample of docs that we used to determine the "on-load" metric.

@exekias
Copy link
Contributor

exekias commented Jun 26, 2019

SGTM

@jasonrhodes
Copy link
Member

when the user (or we, on load) have selected a metric, then we need to do another query to get a sample of documents that have that specific field.

@roncohen yes, that's the first bullet point in the above approach, I think? If I understand correctly. :) The user visits the page and has a list of metrics to choose from (how to make that experience better is handled in another ticket). As they select metrics, we do an "exists" query for those fields to get a sample of docs that have those fields, then use the other fields in those docs to drive the group by dropdown.

@jasonrhodes
Copy link
Member

jasonrhodes commented Jun 26, 2019

Another solution suggested by @skh:

Have a list of fields that are always available to group by (ECS fields we expect in all documents -- do these exist?), then add all other fields that start with the same top level key, e.g. for server.load.1 we show all of the system.**.* fields.

@exekias
Copy link
Contributor

exekias commented Jun 27, 2019

That could be another option, just take into account that ECS has a ton of fields in it, and most of them will have no data. So while the situation would improve a bit, we would still have many fields that make no sense there.

I would say the initial proposed solution would provide the right field set in most cases. Perhaps a Show all button in the drop-down would cover the other 1%

@tbragin
Copy link
Contributor Author

tbragin commented Jun 27, 2019

++ on first trying the approach described by Jason here, and having a "show all" button as a fallback. we can evaluate and see. if we notice we are missing too many fields, we can go to the approach Sonja suggested. How does that sound?

@roncohen
Copy link
Contributor

OK. I suggest we make sure to add telemetry to track if the "show all" is ever used

@jasonrhodes
Copy link
Member

This should be included in the meta issue that @simianhacker is looking into re: field availability

@jasonrhodes
Copy link
Member

I just added a brief summary of the two options we've discussed to this ticket's description. We should choose one and move forward.

@simianhacker
Copy link
Member

I would do a combination of the two options. If the metric field is prefixed with kubernetes, docker or prometheus I would include all the string fields from those modules along with the sample fields from the last 5 documents.

simianhacker added a commit to simianhacker/kibana that referenced this issue Aug 14, 2019
- Closes elastic#41090
- Closes elastic#39613
- Adds allowed list for ECS, Promehteus, Kubernetes, and Docker fields
- Filters "graph pre" fields by selected metrics
- Only displays allowed fields for metric selection, graph per, and
Kquery bar
simianhacker added a commit that referenced this issue Sep 18, 2019
* [Infra UI] Limit Metric Explorer fields

- Closes #41090
- Closes #39613
- Adds allowed list for ECS, Promehteus, Kubernetes, and Docker fields
- Filters "graph pre" fields by selected metrics
- Only displays allowed fields for metric selection, graph per, and
Kquery bar

* Fixing test

* Changing all caps to camel case

* Fixing logic to be more clear and handle null use cases

* Changing to singular
simianhacker added a commit to simianhacker/kibana that referenced this issue Sep 18, 2019
* [Infra UI] Limit Metric Explorer fields

- Closes elastic#41090
- Closes elastic#39613
- Adds allowed list for ECS, Promehteus, Kubernetes, and Docker fields
- Filters "graph pre" fields by selected metrics
- Only displays allowed fields for metric selection, graph per, and
Kquery bar

* Fixing test

* Changing all caps to camel case

* Fixing logic to be more clear and handle null use cases

* Changing to singular
simianhacker added a commit that referenced this issue Sep 18, 2019
* [Infra UI] Limit Metric Explorer fields

- Closes #41090
- Closes #39613
- Adds allowed list for ECS, Promehteus, Kubernetes, and Docker fields
- Filters "graph pre" fields by selected metrics
- Only displays allowed fields for metric selection, graph per, and
Kquery bar

* Fixing test

* Changing all caps to camel case

* Fixing logic to be more clear and handle null use cases

* Changing to singular
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants