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

Search on canvas #1429

Merged
merged 17 commits into from
May 12, 2016
Merged

Search on canvas #1429

merged 17 commits into from
May 12, 2016

Conversation

davkal
Copy link
Contributor

@davkal davkal commented May 4, 2016

screen shot 2016-05-04 at 12 29 23

Features:

  • adds a search field next to the topologies
  • highlight results on canvas as you type
  • non-matching nodes are grayed out
  • "prefix:" limits search to field label
  • apply query as filter on Enter
  • match metrics, e.g. cpu > 2 or memory > 4MB
  • store (pinned) searches in URL
  • sanitize search queries

Fixups:

  • check big topologies
  • tweak match overlays to be always at the top
  • sublables overlap labels in focus mode
  • removing all input fast brings previous input back

@davkal davkal force-pushed the search branch 2 times, most recently from dcad43d to 85b7f62 Compare May 4, 2016 13:50
@davkal davkal changed the title Search [WIP] Search May 4, 2016
@davkal davkal added this to the 0.15.0 milestone May 4, 2016
@davkal davkal self-assigned this May 4, 2016
@davkal davkal added component/ui Predominantly a front-end issue; most/all of the work can be completed by a f/e developer dogfood Important for the developer's own use of the project labels May 4, 2016
@tomwilkie tomwilkie removed this from the 0.15.0 milestone May 4, 2016
@tomwilkie
Copy link
Contributor

if I type "python" and press enter, I get a blob/pill. I would like to be able to delete the blob/pill using backspace.

@tomwilkie
Copy link
Contributor

Also I couldn't quite figure out how to do the metric search (less than, greater than etc). Perhaps making the hints clickable (to see next hint) would help with discoverability?

@tomwilkie
Copy link
Contributor

tomwilkie commented May 6, 2016

Also switching between topologies seem to have some bugs. When I search for "python" (which is not in the label or sublabel) is highlights the command (good), but then I switch away from the containers view to the the processes view and back, and they are gone.

(But only briefly - they come back after a few seconds)

@davkal
Copy link
Contributor Author

davkal commented May 6, 2016

(But only briefly - they come back after a few seconds)

This is might be a backend issue. A lot of the time, the backend does not send the metadata field of nodes in the delta.

@davkal davkal changed the title [WIP] Search Search on canvas May 6, 2016
@davkal davkal assigned foot and unassigned davkal May 6, 2016
@2opremio
Copy link
Contributor

2opremio commented May 8, 2016

Fixes #1299

@foot
Copy link
Contributor

foot commented May 9, 2016

I have noticed that the BE doesn't send metadata initially (w/ addd nodes) too.

@@ -1,10 +1,13 @@
import React from 'react';
import ReactDOM from 'react-dom';
import { connect } from 'react-redux';
import classNames from 'classnames';
import cx from 'classnames';

This comment was marked as abuse.

This comment was marked as abuse.

@foot
Copy link
Contributor

foot commented May 9, 2016

  • typing in search box triggers other shorcuts ([<>q?])
  • z-index of labels is a bit funny (not sure if they overlapped other nodes before...). But now that they are nicely wrapped the text can end up behind other nodes when hovering. Possible solution: draw hovered node last..
  • My text-bg hover fix broke search-matched-text bg onHover

Lower-priority

  • There is a funny effect going on when you nodeLeave, where some of the other nodes will pulse, not sure what its indicating. (also pulsing happens in a selected-layout onDelta).
  • Node-details panel not getting any highlighting.

Enhancements

  • Shortcut to get to the search bar. (or clean up our tabindexes so tab gets you there first).
  • esc should perhaps also blur if the search is empty.
  • large-topo-issue Opens up more questions re: to-layout or not-to-layout. onSearchEnter: relayout? Force-layout does the trick. (what do you think about force-layout refocusing/zooming? + maybe a shortcut).
  • Maybe be more flexible w/ memory selectors. (e.g. support for 1000m, 1000mb, 1000K, 1000Kb)
  • Other uses for enter could be interesting too.
    • Having a "selected layout" with all matching nodes (maybe doesn't work for too many matches though).
    • Focusing another topo that has results if the current one doesn't have any.
  • Containers isn't blue-boxed unless you've previously selected both/system containers for weavescope

@2opremio
Copy link
Contributor

2opremio commented May 9, 2016

@@ -404,7 +401,7 @@ function mapStateToProps(state) {
return {
adjacentNodes: getAdjacentNodes(state),
forceRelayout: state.get('forceRelayout'),
nodes: state.get('nodes'),
nodes: state.get('nodes').filter(node => !node.get('filtered')),

This comment was marked as abuse.

@2opremio
Copy link
Contributor

2opremio commented May 9, 2016

The animations of the nodes are a bit strange. When hovering a node, it jumps and then the other nodes jump. Is that intentional?

(The small framerate makes it difficult to see in the gif above, but I hope it's enough to illustrate it)

Also, when clicking on a "searched" node, the surrounding nodes get grayed out, which I guess is consistent with the view, but maybe a bit unpleasant because you cannot see the edges properly (no strong opinions about this one though).

screen shot 2016-05-09 at 1 57 34 pm

In that case I think there may be no need to gray out since the blue color boxes may be enough to distinguish which ones were filtered.

@2opremio
Copy link
Contributor

2opremio commented May 9, 2016

In contrast mode the search box is not well defined since it has the same color as the background. Maybe it could be surrounded by an edge?

screen shot 2016-05-09 at 2 12 06 pm

screen shot 2016-05-09 at 2 11 06 pm

@2opremio
Copy link
Contributor

2opremio commented May 9, 2016

SVG-generation misses the node tags (which may be intentional as well, I don't know):

normal:

screen shot 2016-05-09 at 2 15 58 pm

svg:

screen shot 2016-05-09 at 2 16 07 pm

return url.replace(new RegExp(SLASH, 'g'), SLASH_REPLACEMENT);
return url
.replace(new RegExp(PERCENT, 'g'), PERCENT_REPLACEMENT)
.replace(new RegExp(SLASH, 'g'), SLASH_REPLACEMENT);
}

This comment was marked as abuse.

This comment was marked as abuse.

@paulbellamy
Copy link
Contributor

paulbellamy commented May 9, 2016

This interacts strangely with filters.

If I select the Containers topology, then set Both for both filters, then select the Containers By Image topology, select System filter, and start doing a search for something which will only match application containers, it highlights the Containers topology link. But when I click back to the Containers view, it applies the filters and there are no nodes shown.

Edit: It's also when you change filters (which affect other topologies) with an active search query, the link highlight won't change.

@paulbellamy
Copy link
Contributor

Also, getting some weird rendering glitches on the labels when hovering over non-matched nodes:
screen shot 2016-05-09 at 16 02 02

try {
return new RegExp(expression, options);
} catch (e) {
return new RegExp(_.escapeRegExp(expression), options);

This comment was marked as abuse.

@foot
Copy link
Contributor

foot commented May 10, 2016

Code looking good. 👍

Some nice comments there.

@davkal davkal force-pushed the search branch 2 times, most recently from 452020d to d0dc202 Compare May 10, 2016 11:05
@2opremio
Copy link
Contributor

2opremio commented May 10, 2016

I have deployed this on the service, and as soon as I start playing with search the app gets overloaded and the probes drop reports. For instance.

<probe> ERRO: 2016/05/10 11:25:56.872675 Dropping report to 10.0.205.170:4040
<probe> ERRO: 2016/05/10 11:26:02.870855 Dropping report to 10.0.205.170:4040
<probe> ERRO: 2016/05/10 11:26:14.835464 Dropping report to 10.0.205.170:4040
<probe> ERRO: 2016/05/10 11:26:23.769282 Dropping report to 10.0.205.170:4040
<probe> ERRO: 2016/05/10 11:26:32.858937 Dropping report to 10.0.205.170:4040

This seems to be an exacerbation of #1457 but probes don't seem to drop reports without search :(

EDIT: I do see reports being dropped but not as often

@davkal davkal mentioned this pull request May 10, 2016
@foot
Copy link
Contributor

foot commented May 11, 2016

More recent commits look fine too.

LGTMTM

davkal and others added 17 commits May 11, 2016 18:08
* adds a search field next to the topologies
* highlight results on canvas as you type
* non-matching nodes are grayed out
* "prefix:" limits search to field label
Examples:

* cpu > 2 // means percent
* memory > 4.5MB
Try regexp, escape if invalid
* Fix node-details-test for search
* Label spacing and matched text truncation
* Delete pinned search on backspace, add hint for metrics, escape % in URL
* Fix text-bg on node highlight
* Added tests for search-utils
* Fix matching of other topologies, added comment re quick clear
* s/cx/classnames/
* Ignore MoC keys when search in focus, blur on Esc
* Fixes search term highlighting on-hover
* Fix SVG exports
* Fine-tuned search item rendering
* Fixed search highlighting in the details panel
* Dont throb node on hover
* Hotkey for search: '/'
* Keep focus on search when tabbing away from the browser
* bring hovered node to top
* background for search results on hover
* fixed height for foreign object to prevent layout glitches
* Dont blur focused nodes on search
* More robust metric matchers
* More meaningful search hints
Having the DOM nodes w/ display:none is still expensive. We only need
them briefly for svg export.
Key was not being generated correctly.
@2opremio
Copy link
Contributor

I find the hovering on not-highlighted nodes/edges surprising (It may be intentional but it feels buggy):

Also, I still see tags from different nodes overlapping

screen shot 2016-05-11 at 5 36 05 pm

Performance-wise it seems to be tolerable

Report: report_search_review.json.gz

Other than that LGTM

@davkal davkal merged commit b5a8b19 into master May 12, 2016
@davkal davkal deleted the search branch May 12, 2016 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/ui Predominantly a front-end issue; most/all of the work can be completed by a f/e developer dogfood Important for the developer's own use of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants