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 Metric Explorer fields #43322

Merged
merged 16 commits into from
Sep 18, 2019

Conversation

simianhacker
Copy link
Member

@simianhacker simianhacker commented Aug 14, 2019

Summary

This PR adds an attribute (displayable) to the list of fields returned by the source status GraphQL endpoint. This value is set to true under the following conditions:

  • The field is included in the ECS_ALLOWED_LIST
  • The field prefix matches one of the prefixes returned in from a terms aggregation for event.dataset
  • The field matches a field in one of the special allowed field lists (like PROMETHEUS_ALLOWED_LIST) only if the event.modules aggregation also includes one of the special prefixes (like prometheus)

The displayable attribute is used to filter the fields displayed in the Metrics Explorer. For the graph per field list, the list is filtered by the prefix of any selected metrics. Along with the other matching prefixes it also displays any ECS fields or matching "special" fields.

image

Related Issues

Meta Issue #40277
Closes #41090
Closes #39613

Caveats

There will still be a lot of fields that are empty OR are missing data but that list should now be significantly smaller. To really fix this problem we need Metricbeat and Filebeat to stop shipping FAT mappings that include every possible field. Instead they should only be setting mappings with only the fields they actually ship. Once that happens, this solution will work even better.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- 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 simianhacker added release_note:fix Feature:Metrics UI Metrics UI feature v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.4.0 labels Aug 14, 2019
@simianhacker simianhacker self-assigned this Aug 14, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-logs-ui

@simianhacker simianhacker marked this pull request as ready for review August 14, 2019 22:32
@simianhacker simianhacker requested a review from a team as a code owner August 14, 2019 22:32
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@simianhacker
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link

@sorantis sorantis left a comment

Choose a reason for hiding this comment

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

Checked out the PR. The metrics search field as well as group-by show fewer metrics. Some fields are still empty, but as @simianhacker mentioned, this can be properly fixed in Metricbeat. LGTM.

@simianhacker
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@spalger
Copy link
Contributor

spalger commented Sep 10, 2019

Please merge master

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ruflin
Copy link
Member

ruflin commented Sep 11, 2019

@simianhacker Haven't played with it yet, one question: Even if a field is not shown but I know it's exact name, can I still filter on it by manually typing it in? If that is the case, +1 on moving forward with this and trying it out.

Copy link
Contributor

@phillipb phillipb left a comment

Choose a reason for hiding this comment

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

Looks good. A few tiny changes.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@simianhacker
Copy link
Member Author

@phillipb This is ready for another review.

Copy link
Contributor

@phillipb phillipb left a comment

Choose a reason for hiding this comment

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

LGTM

@simianhacker simianhacker merged commit 70166a0 into elastic:master Sep 18, 2019
simianhacker added a commit to simianhacker/kibana that referenced this pull request 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 pull request 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
@roncohen
Copy link
Contributor

hey @simianhacker. Thanks for getting this rolling.

I see that we still show @timestamp, ephemeral_id etc.
image

I imagine these are only relevant if you have selected cardinality? I suggest we get rid of the cardinality option and with that, also get rid of any non numeric fields, i.e. only show long/int/float/double fields

@simianhacker
Copy link
Member Author

@roncohen We should filter out the string fields for the metric style aggregations. I don't think we should get rid of cardinality, I use it quite a bit for stuff like this (when I'm exploring data).

image

@roncohen
Copy link
Contributor

That could also work. It could be a problem that people don’t understand in which situations keyword fields show up and in which they don’t, but I’m OK to try it out. Thought @tbragin @sorantis?

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 release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.5.0 v8.0.0
Projects
None yet
7 participants