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

Adding sass lint to Monitoring #44148

Closed
wants to merge 30 commits into from

Conversation

andreadelrio
Copy link
Contributor

@andreadelrio andreadelrio commented Aug 27, 2019

Summary

Light mode (before)
shard_old_light
Light mode (after)
image

Dark mode (before)
shard_old_dark
Dark mode (after)
image

Checklist

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

For maintainers

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

andreadelrio and others added 3 commits September 10, 2019 16:15
…th null filterQuery (elastic#45218)

## Summary

Changes the ML drill-downs to use the tabs and re-direct to the Anomalies table when drilled down. 

elastic#45080

Tests for this were both by playing with the Anomalies as well as hand testing that these clickable links below do what I would expect them to do based on the conditional rules of:

* Split comma separated values into OR clauses within KQL.
* Redirect from multiple hosts/ips on the details page to the host over view/detail overview page with a new KQL added as a filter since comma separated values on details would just be errors.
* Remove/Replace any $value$ dollar values that did not have a value as before.

Manual testing is from either the test cases below or by using the ML Anomalies explorerand clicking on the drill down links using the action menu items from Host or IP jobs which look like this:

<img width="352" alt="Screen Shot 2019-09-06 at 4 17 05 PM" src="https://user-images.githubusercontent.com/1151048/64576200-c1852780-d334-11e9-8270-ef97569a2e78.png">


URL manual test cases I used:

Testing conditional ml-network links:
-----

Single IP with a null for the KQL:
http://localhost:5601/app/siem#/ml-network/ip/127.0.0.1?kqlQuery=(filterQuery:!n,queryLocation:network.details,type:details)&timerange=(global:(linkTo:!(timeline),timerange:(from:'2019-08-28T11:00:00.000Z',kind:absolute,to:'2019-08-28T13:59:59.999Z')),timeline:(linkTo:!(global),timerange:(from:'2019-08-28T11:00:00.000Z',kind:absolute,to:'2019-08-28T13:59:59.999Z')))


Single IP with kqlQuery:
http://localhost:5601/app/siem#/ml-network/ip/127.0.0.1?kqlQuery=(filterQuery:(expression:'process.name%20:%20%22conhost.exe,sc.exe%22',kind:kuery),queryLocation:network.details,type:details)&timerange=(global:(linkTo:!(timeline),timerange:(from:'2019-08-28T11:00:00.000Z',kind:absolute,to:'2019-08-28T13:59:59.999Z')),timeline:(linkTo:!(global),timerange:(from:'2019-08-28T11:00:00.000Z',kind:absolute,to:'2019-08-28T13:59:59.999Z')))


Multiple IP's with a null for the filterQuery:
http://localhost:5601/app/siem#/ml-network/ip/127.0.0.1,127.0.0.2?kqlQuery=(filterQuery:!n,queryLocation:network.details,type:details)&timerange=(global:(linkTo:!(timeline),timerange:(from:'2019-08-28T11:00:00.000Z',kind:absolute,to:'2019-08-28T13:59:59.999Z')),timeline:(linkTo:!(global),timerange:(from:'2019-08-28T11:00:00.000Z',kind:absolute,to:'2019-08-28T13:59:59.999Z')))


Multiple IP's with a value for the filterQuery:
http://localhost:5601/app/siem#/ml-network/ip/127.0.0.1,127.0.0.2?kqlQuery=(filterQuery:(expression:'process.name%20:%20%22conhost.exe,sc.exe%22',kind:kuery),queryLocation:network.details,type:details)&timerange=(global:(linkTo:!(timeline),timerange:(from:'2019-08-28T11:00:00.000Z',kind:absolute,to:'2019-08-28T13:59:59.999Z')),timeline:(linkTo:!(global),timerange:(from:'2019-08-28T11:00:00.000Z',kind:absolute,to:'2019-08-28T13:59:59.999Z')))


Undefined/null IP and a null filterQuery:
http://localhost:5601/app/siem#/ml-network/ip/$ip$?kqlQuery=(filterQuery:!n,queryLocation:network.details,type:details)&timerange=(global:(linkTo:!(timeline),timerange:(from:'2019-08-28T11:00:00.000Z',kind:absolute,to:'2019-08-28T13:59:59.999Z')),timeline:(linkTo:!(global),timerange:(from:'2019-08-28T11:00:00.000Z',kind:absolute,to:'2019-08-28T13:59:59.999Z')))


Undefined/null IP but a value for the filterQuery:
http://localhost:5601/app/siem#/ml-network/ip/$ip$?kqlQuery=(filterQuery:(expression:'process.name%20:%20%22conhost.exe,sc.exe%22',kind:kuery),queryLocation:network.details,type:details)&timerange=(global:(linkTo:!(timeline),timerange:(from:'2019-08-28T11:00:00.000Z',kind:absolute,to:'2019-08-28T13:59:59.999Z')),timeline:(linkTo:!(global),timerange:(from:'2019-08-28T11:00:00.000Z',kind:absolute,to:'2019-08-28T13:59:59.999Z')))


Testing conditional host links:


Single host name with a null for the KQL:
http://localhost:5601/app/siem#/ml-hosts/siem-windows?_g=()&kqlQuery=(filterQuery:!n,queryLocation:hosts.details,type:details)&timerange=(global:(linkTo:!(timeline),timerange:(from:'2019-06-06T06:00:00.000Z',kind:absolute,to:'2019-06-07T05:59:59.999Z')),timeline:(linkTo:!(global),timerange:(from:'2019-06-06T06:00:00.000Z',kind:absolute,to:'2019-06-07T05:59:59.999Z')))


Single host name with a variable left in the KQL
http://localhost:5601/app/siem#/ml-hosts/siem-windows?_g=()&kqlQuery=(filterQuery:(expression:'process.name%20:%20%22$process.name$%22',kind:kuery),queryLocation:hosts.details,type:details)&timerange=(global:(linkTo:!(timeline),timerange:(from:'2019-06-06T06:00:00.000Z',kind:absolute,to:'2019-06-07T05:59:59.999Z')),timeline:(linkTo:!(global),timerange:(from:'2019-06-06T06:00:00.000Z',kind:absolute,to:'2019-06-07T05:59:59.999Z')))


Single host name with a value for filterQuery:
http://localhost:5601/app/siem#/ml-hosts/siem-windows?_g=()&kqlQuery=(filterQuery:(expression:'process.name%20:%20%22conhost.exe,sc.exe%22',kind:kuery),queryLocation:hosts.details,type:details)&timerange=(global:(linkTo:!(timeline),timerange:(from:'2019-06-06T06:00:00.000Z',kind:absolute,to:'2019-06-07T05:59:59.999Z')),timeline:(linkTo:!(global),timerange:(from:'2019-06-06T06:00:00.000Z',kind:absolute,to:'2019-06-07T05:59:59.999Z')))


Multiple host names with null for filterQuery

http://localhost:5601/app/siem#/ml-hosts/siem-windows,siem-suricata?_g=()&kqlQuery=(filterQuery:!n,queryLocation:hosts.details,type:details)&timerange=(global:(linkTo:!(timeline),timerange:(from:'2019-06-06T06:00:00.000Z',kind:absolute,to:'2019-06-07T05:59:59.999Z')),timeline:(linkTo:!(global),timerange:(from:'2019-06-06T06:00:00.000Z',kind:absolute,to:'2019-06-07T05:59:59.999Z')))

Multiple host names with a value for filterQuery
http://localhost:5601/app/siem#/ml-hosts/siem-windows,siem-suricata?_g=()&kqlQuery=(filterQuery:(expression:'process.name%20:%20%22conhost.exe,sc.exe%22',kind:kuery),queryLocation:hosts.details,type:details)&timerange=(global:(linkTo:!(timeline),timerange:(from:'2019-06-06T06:00:00.000Z',kind:absolute,to:'2019-06-07T05:59:59.999Z')),timeline:(linkTo:!(global),timerange:(from:'2019-06-06T06:00:00.000Z',kind:absolute,to:'2019-06-07T05:59:59.999Z')))


Undefined/null host name with a null for the KQL:
http://localhost:5601/app/siem#/ml-hosts/$host.name$?_g=()&kqlQuery=(filterQuery:!n,queryLocation:hosts.details,type:details)&timerange=(global:(linkTo:!(timeline),timerange:(from:'2019-06-06T06:00:00.000Z',kind:absolute,to:'2019-06-07T05:59:59.999Z')),timeline:(linkTo:!(global),timerange:(from:'2019-06-06T06:00:00.000Z',kind:absolute,to:'2019-06-07T05:59:59.999Z')))


Undefined/null host name but with a value for filterQuery
http://localhost:5601/app/siem#/ml-hosts/$host.name$?_g=()&kqlQuery=(filterQuery:(expression:'process.name%20:%20%22conhost.exe,sc.exe%22',kind:kuery),queryLocation:hosts.details,type:details)&timerange=(global:(linkTo:!(timeline),timerange:(from:'2019-06-06T06:00:00.000Z',kind:absolute,to:'2019-06-07T05:59:59.999Z')),timeline:(linkTo:!(global),timerange:(from:'2019-06-06T06:00:00.000Z',kind:absolute,to:'2019-06-07T05:59:59.999Z')))

----



Extra misc tests:

3 host names
http://localhost:5601/app/siem#/ml-hosts/suricata-iowa,siem-windows,siem-fake?_g=()&kqlQuery=(filterQuery:(expression:'process.name%20:%20%22snapd%22',kind:kuery),queryLocation:hosts.details,type:details)&timerange=(global:(linkTo:!(timeline),timerange:(from:'2019-09-09T18:00:00.000Z',kind:absolute,to:'2019-09-09T20:59:59.999Z')),timeline:(linkTo:!(global),timerange:(from:'2019-09-09T18:00:00.000Z',kind:absolute,to:'2019-09-09T20:59:59.999Z')))


3 ips
http://localhost:5601/app/siem#/ml-network/ip/127.0.0.1,127.0.0.2,127.0.0.3?_g=()&kqlQuery=(filterQuery:!n,queryLocation:network.details,type:details)&timerange=(global:(linkTo:!(timeline),timerange:(from:'2019-08-28T06:00:00.000Z',kind:absolute,to:'2019-08-29T05:59:59.999Z')),timeline:(linkTo:!(global),timerange:(from:'2019-08-28T06:00:00.000Z',kind:absolute,to:'2019-08-29T05:59:59.999Z')))





### Checklist

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

- [x] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility)
- [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
- [x] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials
- [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios
- [ ] This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist)

### For maintainers

- [x] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)
- [x] This includes a feature addition or change that requires a release note and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)
@andreadelrio andreadelrio added release_note:skip Skip the PR/issue when compiling release notes EUI Team:Monitoring Stack Monitoring team labels Sep 10, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@andreadelrio
Copy link
Contributor Author

New layout for Shard Legend following @snide's proposal. One big advantage of this layout is that using the same UI to represent health (i.e. red/yellow/green dot) which adds consistency.

@snide I stuck with EuiBadge instead of making custom made blocks because the legend is using EuiBadges and I wanted to be able to use the same colors (primary, warning, etc) for both. Let me know if you think creating custom made blocks would make more sense.

image

image

@andreadelrio andreadelrio requested a review from a team as a code owner September 27, 2019 01:09
@elasticmachine
Copy link
Contributor

💔 Build Failed

@andreadelrio andreadelrio requested a review from snide October 2, 2019 14:35
@elasticmachine
Copy link
Contributor

💔 Build Failed

@chrisronline
Copy link
Contributor

@elastic/stack-monitoring

We need to assist @andreadelrio here. Based on the most recent screenshots, it look like there will be a scalability problem. Currently, we show a group of boxes for each shard that is on the node/in the index (based on which page we're looking at) and that group is theoretically unbounded since I don't think there is an upper limit to the amount of shards per index.

I'm proposing a change to this behavior in the shard legend.

Instead of showing a group of boxes for each shard, why don't we always/only show 6 boxes - a box for each shard state. Then, inside of the box, we show the number of shards in this state. If the user hovers over the box, they will see the list of shards that make up that number in that state.

The only downside to this is a user can't easily scan the legend to see where all the individual shards exist, but I'm not sure how valuable that really is. My guess is most users just care about the state of the shards and how many shards are in that state.

Thoughts?

BTW, this will be a change in behavior, as the number inside the box will represent something different than it currently does so that will need to be communicated somehow.

@igoristic
Copy link
Contributor

@chrisronline

Instead of showing a group of boxes for each shard, why don't we always/only show 6 boxes - a box for each shard state

Yeah, that's not a bad alternative.

I was thinking maybe it would be easier to leave it the way it is now and if the amount is over a specific threshold (lets say 50) then we show another legend that states something like:
[ ... ] and clicking it loads the next 50 and so on...

@cachedout
Copy link
Contributor

Instead of showing a group of boxes for each shard, why don't we always/only show 6 boxes - a box for each shard state.

I really like this idea and I'm wondering if we can't take it even further. There are two types of shards (primary/replica) and those shards can be in one of three states (relocating/initializing/unassigned). Could we do something like two boxes for the two types of shards and in each, just show the number of shards in each state? I could do a quick sketch of what I'm thinking if that's helpful.

@chrisronline
Copy link
Contributor

Maybe we should take a step back and talk through why a user would be looking at this shard legend, and what they are looking for.

I'd imagine the most common thing is to spot any anomalies - which would be unallocated shards. Next, maybe the other important state is relocating. The other state(s) suggest everything is fine and nothing is really happening that requires the user's attention.

So a priority order might be:

  1. Unallocated primary shards
  2. Unallocated replica shards
  3. Relocating primary shards
  4. Relocating replica shards
  5. Everything else

If we agree on that, maybe that will help us figure out what we should be highlighting in this UI.

But, I don't want to hold up this PR with a larger discussion. I don't think we can merge this PR as is because I think the UI will completely break at some small number of shards (sounds like maybe 5 or 6).

@cachedout What do you think is a good stop-gap solution for this?

@cachedout
Copy link
Contributor

But, I don't want to hold up this PR with a larger discussion.

I agree. I wrote a long comment and then I set it aside because I think it's best to open a new issue to discuss this further.

What do you think is a good stop-gap solution for this?

I propose that we make the following changes to the shard legend on the node page:

  1. For each index in the table, partially adopt Chris' suggestion to display only the number of shards. Therefore, each index should contain an indication that there are X number of primary shards and Y number of replica shards. (This is a change in behavior.) These would be displayed in two boxes, one blue and one green, to the left of the index name, somewhat similar to how it is now.

  2. As we have it in this PR, display a green dot to the right indicating a lack of problems. Concretely, this means that no shards in the index are unassigned and no shards are relocating or initializing.

  3. If any of the conditions in step 2 do exist for shards in the index, remove the green dot. If the index contains unassigned shards, display a red dot. Hovering over a dot shows the number of shards in a given state. Additionally, add a colored bar below the box indicating the number of shards of a given type, indicating that some of the shards of that type are unassigned.

  4. If one or more shards in an index are relocating, do not display the green dot and display a purple dot indicating that at least one shard is relocating. Hovering over the dot shows the user how many shards are relocating. Add a colored bar similar to the approach in step 3.

  5. If one or more shards are initializing, do not display the green dot and display a purple dot. Add a colored bar similar to the approach in step 3.

  6. Hovering over any dot or colored bar would give details about the number of shards in that state.

Forgive my terrible mock-up skills, but here's a rough idea of what I mean:

rough

What that would show would indicate that the first index (.apm-agent-configuration) has one or more unassigned replica shard.

For .monitoring-es-7-2019.10.10, there are one or more replica shards which are relocating and also one one or more replica shards which are unassigned.

I think we can implement this relatively quickly on top of this PR and that would give the user the ability to quickly scan and see any signs of unusual activity.

That said, I think the entire shard legend could use a refresh and this is only a stopgap proposal to get this PR out the door. Let me know if this seems reasonable. If it's just much easier to go with Chris' proposal of six boxes, that's fine by me, but I wanted to toss this out there is another path that might get us what we're looking for without too many immediate changes being necessary.

@chrisronline
Copy link
Contributor

@cachedout I think that makes sense, but I'm not sure @andreadelrio feels confident making these types of changes (please correct me if I'm making a bad assumption here). I think one of us will have to step in and write this logic inside of this PR. Did you want to take a stab at it @cachedout since you have a pretty clear picture of what we should do (which sounds right to me too)?

@cachedout
Copy link
Contributor

Did you want to take a stab at it @cachedout since you have a pretty clear picture of what we should do (which sounds right to me too)?

@chrisronline Can do!

@andreadelrio Do you have a timetable for these changes? I'm just trying to gauge the priority in relation to other tasks.

@andreadelrio
Copy link
Contributor Author

Chris is right that I’d prefer assistance implementing those changes. Once they’re in I can do a design cleanup if needed.

I’d like to be able to merge this PR as soon as possible as it’s been under review for a while but we don't a have a strict timetable.

@chrisronline
Copy link
Contributor

Quick ping here. This PR is getting old/stale quickly. @cachedout Do you still have plans to address the work here? Perhaps we should close out this PR, and re-open it once we're ready to begin again?

@cachedout
Copy link
Contributor

@chrisronline Thanks for the bump. This is on my list but it's not at the top right now. I would be OK if we closed this for the time being.

@andreadelrio andreadelrio mentioned this pull request Nov 19, 2019
2 tasks
@chrisronline
Copy link
Contributor

Closing this due to inactivity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EUI release_note:skip Skip the PR/issue when compiling release notes Team:Monitoring Stack Monitoring team v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants