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

Move filter to query param and highlight filter matches on graphs #310

Conversation

everett980
Copy link
Collaborator

@everett980 everett980 commented Jan 9, 2019

Which problem is this PR solving?

Short description of the changes

  • Moving span filtering from state.filterText to a query param (uiFind) allows filters to be shared
  • On initial load of TimelineView with a truthy uiFind, all rows are collapsed except paths to matches and matches have details expanded
  • Graph nodes are outlined if they match filter
  • Filtering logic returns spans if their spanID === any filter word
  • SpanID in SpanRowDetail is clickable to add to filter to easily link to specific spans
  • Update Ezyme version, re-enable skipped tests, add tests to hit 100% coverage for TracePage/index.js and TraceDiff/

Demo

On initial load with three spanIDs provided:

image

Add another filter word

image

After clicking expand all

Changing filter does not hide / show any children nor expand detail states, only changes which rows are highlighted. On refresh all highlighted rows would have their detail states visible and spans not on any path to a highlighted row would be hidden.
image

Clickable SpanIDs

image

Find in Diff Graph

Diff Graph does not have header, uiFind is present in the bottom right

Finding Gif (old uiFind)

quick uifind diff

Updated uiFind

image

Updated uiFind Close Up

image

Find in Trace Graph

image

Close Up of Find

re-uses find header on Timeline view, but up and down arrows are disabled.
image

@ghost ghost assigned everett980 Jan 9, 2019
@ghost ghost added the review label Jan 9, 2019
@codecov
Copy link

codecov bot commented Jan 9, 2019

Codecov Report

Merging #310 into master will increase coverage by 7.53%.
The diff coverage is 89.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #310      +/-   ##
==========================================
+ Coverage   83.18%   90.72%   +7.53%     
==========================================
  Files         145      152       +7     
  Lines        3224     3288      +64     
  Branches      660      666       +6     
==========================================
+ Hits         2682     2983     +301     
+ Misses        434      250     -184     
+ Partials      108       55      -53
Impacted Files Coverage Δ
...nts/TracePage/TraceTimelineViewer/SpanDetailRow.js 100% <ø> (ø) ⬆️
...nents/TracePage/TracePageHeader/TracePageHeader.js 100% <ø> (+6.66%) ⬆️
...omponents/TraceDiff/TraceDiffHeader/CohortTable.js 100% <ø> (+85%) ⬆️
...aeger-ui/src/components/TracePage/ScrollManager.js 91.48% <0%> (-7.37%) ⬇️
packages/jaeger-ui/src/model/trace-dag/TraceDag.js 78.72% <0%> (+20.02%) ⬆️
...ts/TracePage/TraceTimelineViewer/SpanTreeOffset.js 100% <100%> (ø) ⬆️
...c/components/TracePage/TraceTimelineViewer/duck.js 100% <100%> (ø) ⬆️
packages/jaeger-ui/src/utils/update-ui-find.js 100% <100%> (ø)
...rc/components/TraceDiff/TraceDiffGraph/drawNode.js 100% <100%> (+80%) ⬆️
...ackages/jaeger-ui/src/components/TraceDiff/duck.js 100% <100%> (+86.2%) ⬆️
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31a8a45...d3965dc. Read the comment docs.

@tiffon
Copy link
Member

tiffon commented Jan 14, 2019

Can you add a gif / screenshot of the trace diff search? Also, the gif doesn't show much of a highlight... is it too grainy to see the highlight? If so, can you provide a screenshot of the effect?

And, for the trace graph, is it not possible to use the search already on the page? Seems odd to have a second search input field.

tiffon
tiffon previously requested changes Jan 14, 2019
Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

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

Looks great. A couple of comments. LMK if any of them look awry.

}

export function mapStateToProps(state: ReduxState): { graphSearch?: string } {
const { graphSearch } = queryString.parse(state.router.location.search);
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to converge the graph and trace timeline search.

Also, for the query parameters, we took the approach of prefacing UI state related query params with ui<page><variable>, e.g. uiTraceDetailFind or something along those lines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tiffon I have been working on converging the graph and timeline search.
Things have changed quite a bit while working on the TraceTimeline search, should I close this PR and open a new one once #303 is done?

Also, I changed the name of the queryParameter to be more general, I will make sure it also includes the fact that it is a UI state variable, however I am not sure how to define the <page> part as this will be used for Trace Timeline, Trace Graph, and Trace Diff Graph.

Copy link
Member

Choose a reason for hiding this comment

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

Opening a new PR might make sense. Or, we can try the "discard review" feature and see what happens.

Re the query param, maybe simple uiFind would work?

textFilter
.split(' ')
.map(s => s.trim())
.filter(s => s)
Copy link
Member

Choose a reason for hiding this comment

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

This can be .filter(Boolean).


// split textFilter by whitespace, remove empty strings, and extract includeFilters and excludeKeys
textFilter
.split(' ')
Copy link
Member

Choose a reason for hiding this comment

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

.split(/\s+/) should remove the need for the .trim() map.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated filter-spans with these two changes

if (graphSearchQueryParam) {
queryParams.graphSearch = graphSearchQueryParam;
}
this.props.history.replace(prefixUrl(`?${queryString.stringify(queryParams)}`));
Copy link
Member

Choose a reason for hiding this comment

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

I would think this might always replace the URL with /?...., i.e. with the URL at the root (with the prefix)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not entirely sure how prefixUrl works, but it seems to work as expected on the timeline view and the diff view

@@ -0,0 +1,109 @@
// @flow
Copy link
Member

Choose a reason for hiding this comment

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

Seems like it might be nice to standardize naming where "search" is the search form and jaeger-query related and "find" is local to the UI. What do you think of renaming this to GraphFind?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as discussed above, changed to uiFind

@everett980
Copy link
Collaborator Author

I merged my branch for #303 into this branch. I am going to attempt to Dismiss review (took screenshots in case they are simply deleted) and then will update the PR description to demonstrate the changes in #303 and #307.

@everett980 everett980 dismissed tiffon’s stale review January 16, 2019 21:35

Updated branch accordingly, along with other significant changes relating to #303

copa2 and others added 22 commits January 16, 2019 16:44
Add alternative view in TracePage which allows
to see count, avg. time, total time and self time for
a given trace grouped by service and operation.

Signed-off-by: Patrick Coray <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
* Misc tweaks for search and trace detail embed mode

Mostly from prior comments.

- Rename query parameter for embedding to start with "ui" and use the
  page as the first word, e.g. "uiSearchHideGraph"

- Change query parameters for the minimap and trace details from hiding
  to showing, e.g. "hidemap" -> "uiTimelineShowMap"

- Save the embed query params in Redux state instead of passing them
  around

- Use a Link with an icon instead of text buttons for opening the
  standalone view of the page

- Propagate whether the trace detail page is from the search page or
  not via the Location#state member on the React Router Location

- When returning to the search page use the previous results instead
  of executing a new search. This is done by storing the query with
  the search results.

- Adjusted aesthetic of "Back to Search" button on trace detail page

- Sequester parsing and stripping query parameters for the embed mode
  to a util

- In various places switch to using the component/*/url.js#getUrl
  functions instead of prefixUrl(...)

Signed-off-by: Joe Farro <[email protected]>

* Fix test break from merging master

Signed-off-by: Joe Farro <[email protected]>

* Keep redux search query synced with results

- Keep redux search query synced with redux search result (and their
  processing). Also fixes jaegertracing#288.

- Bolster unit tests

Signed-off-by: Joe Farro <[email protected]>

* Fix typo

Signed-off-by: Joe Farro <[email protected]>

* Make TracePageHeader collapsible when embedded

- Reconfigured embed query parameters for timeline:

  - uiTimelineCollapseTitle=1 - TracePageHeader starts out collapsed

  - uiTimelineHideMinimap=1 - TracePageHeader does not show the minimap

  - uiTimelineHideSummary=1 - TracePageHeader does not show the trace
    summary

- Consolidate TracePageHeader and TracePageHeaderEmbed

- Style changes to TracePageHeader

- Embedded TracePageHeader can now be expanded and collapsed

- Misc cleanup in TracePageHeader

- Better comparisons for search page query to prevent re-fetching
  when returning to the search page

Signed-off-by: Joe Farro <[email protected]>

* Fix typo disableComparisions

Signed-off-by: Joe Farro <[email protected]>

* Use public registry to newly installed packages

Signed-off-by: Joe Farro <[email protected]>

* Test improvements

Signed-off-by: Joe Farro <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Joe Farro <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
…racing#290)

* Add button to reset viewing layer zoom (jaegertracing#215)

Signed-off-by: Everett Ross <[email protected]>

* Adhere to className pattern, sort imports, remove event handling

Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
…gertracing#292)

* Add a copy icon to entries in KeyValuesTable (jaegertracing#204)

Signed-off-by: Everett Ross <[email protected]>

* Add a tooltip to copy icon in KeyValuesTable

Signed-off-by: Everett Ross <[email protected]>

* Fix copied test name, add test for KeyValuesTable state change on tooltip hide

Signed-off-by: Everett Ross <[email protected]>

* Add eslint rule to prevent unnecessary braces in jsx

Signed-off-by: Everett Ross <[email protected]>

* Add classname to tr to remove element selector, fix yarn.lock

Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
…tracing#244) (jaegertracing#291)

* Add validation for duration fields in SearchForm (jaegertracing#244)

Signed-off-by: Everett Ross <[email protected]>

* Add tests for redux-form-field-adapter

Signed-off-by: Everett Ross <[email protected]>

* Fix boolean prop type

Signed-off-by: Everett Ross <[email protected]>

* Add boolean for input validation, change popover to show when inactive

Signed-off-by: Everett Ross <[email protected]>

* Add tests for onChangeAdapter

Signed-off-by: Everett Ross <[email protected]>

* Create separate ValidatedAdaptedInput for duration fields

Signed-off-by: Everett Ross <[email protected]>

* Remove unnecessary curly braces

Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
…racing#297)

* Add indent guides to trace timeline view (jaegertracing#172)

Signed-off-by: Everett Ross <[email protected]>

* Add tests for connect functions, add more flow types

Signed-off-by: Everett Ross <[email protected]>

* Consolidate ducks, remove redudant PropTypes, add event type

Signed-off-by: Everett Ross <[email protected]>

* Rename hoverSpanId to hoverIndentGuideId

Signed-off-by: Everett Ross <[email protected]>

* Derive props from span, use dataset over getAttribute

Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
* WIP upgrade to create-react-app v2.1.2

Requires changes from PR jaegertracing#346 which are published as
[email protected]

timarney/react-app-rewired#346

Signed-off-by: Joe Farro <[email protected]>

* Use node 8, less liberal browser support, fix test

Signed-off-by: Joe Farro <[email protected]>

* Use eslintrc, fix CI build, fix pre-commit hook

Make sure the ./packages/jaeger-ui uses the ./.eslintrc.

Make sure all tests are run in pre-commit hook.

CRA is now failing builds in CI if there are any webpack build warnings:

https://github.com/facebook/create-react-app/blob/73e3d0ebf1f2834e1c8c41d3a25ae5e0e99e6f14/packages/react-scripts/scripts/build.js#L171-L184
Signed-off-by: Joe Farro <[email protected]>

* Skip react-vis.css format check, fail-fast in CI

Signed-off-by: Joe Farro <[email protected]>

* Don't collect coverage from dev proxy setup

Signed-off-by: Joe Farro <[email protected]>

* Upgrade react-app-rewired to 2.0.1

Avoid issue yarnpkg/yarn#6300

Signed-off-by: Joe Farro <[email protected]>

* Cleanup npm packages in packages/jaeger-ui

Signed-off-by: Joe Farro <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
…cing#275) (jaegertracing#278)

* Ability to open additional menu links in same tab (jaegertracing#275)

Signed-off-by: Vitaliy Zabolotskyy <[email protected]>

* Add negative test case

Signed-off-by: Vitaliy Zabolotskyy <[email protected]>

* Add helper function to create item links

Signed-off-by: Vitaliy Zabolotskyy <[email protected]>

* Fix no-use-before-define lint error

Signed-off-by: Vitaliy Zabolotskyy <[email protected]>

* Use anchorTarget in custom menu configuration

Signed-off-by: Joe Farro <[email protected]>

* Fix typo in test case

Signed-off-by: Joe Farro <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
* Revive the changelog

Signed-off-by: Joe Farro <[email protected]>

* Add helper script to generate CHANGELOG entries

Signed-off-by: Joe Farro <[email protected]>

* One more changelog item, fix date typo

Signed-off-by: Joe Farro <[email protected]>

* Fix phrasing in changelog

Signed-off-by: Joe Farro <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
@everett980
Copy link
Collaborator Author

everett980 commented Mar 5, 2019

  • Is there a reason to use the blur on the box-shadow? Seems like it might be a perf hit.
  • From what I can tell, only the yellow box-shadow is being used, not the grey outline. Seems like the outline might have better performance because it's a much simpler effect. Maybe we should use outline exclusively?

Seems as though mixing inheritance and definition when using both long-hand and short-hand css rules for outlines works a bit differently than I originally thought. I changed the isUiFindMatch styling to only add an outline, no box-shadow. Performance gains are always nice to have, and this has the benefit of no longer relying on the implicit color behavior of box-shadows without a color.

@everett980 everett980 dismissed tiffon’s stale review March 5, 2019 23:48

Feedback has been addressed, will make issues for the outstanding work outside of the scope of this PR

…aph' into issue-239-fix-TracePageHeader-tests-once-new-version-of-enzyme-is-released
@everett980
Copy link
Collaborator Author

This PR should get the test coverage up to snuff. It was once 100% for all files in this PR. Now it might be slightly dated so I'll take a look on Thursday.

tiffon
tiffon previously requested changes Mar 7, 2019
Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

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

Looks great. Search seems faster.

A handful of small comments.

stroke-width: 1.2;
}

.TraceDiffGraph--dag.uiFind {
Copy link
Member

Choose a reason for hiding this comment

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

Because this CSS selector reflects the state of a component, uiFind should be prefixed with is-, e.g. something like .is-uiFind or .is-find-mode.

props: Props;
export function setOnNodesContainer(state: Object) {
const { zoomTransform } = state;
const matchSize = 8 + 4 / _get(zoomTransform, 'k', 1);
Copy link
Member

Choose a reason for hiding this comment

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

Seems like these magic numbers could stand to be consts.

const ancestors: Span[] = [];
let ref = getFirstAncestor(span);
while (ref) {
ancestors.push(ref);
Copy link
Member

Choose a reason for hiding this comment

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

Seems like you can push the ID in and skip _map(...., 'spanID')

ancestors.push(ref.spanID);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good call, I changed it to a set of IDs (just in case there are repeated IDs) and then converted to an array in the return.
although now I am wondering if it is possible for there to be repeated spanIDs in a trace.

_searchBar: { current: Input | null };
_scrollManager: ScrollManager;
traceDagEV: Object;
Copy link
Member

Choose a reason for hiding this comment

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

Why make this an object instead of something with { edges: ..., vertices: ... } or direct _edges and _vertices?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if convPlexus's return value was typed, then this and other places could simply import that return value. I started trying to type it but wasn't quite satisfying flow and the errors it gave were not simple to debug. Since we're about to convert to typescript, I'd rather add a todo comment.
// TODO: typescript conversion - use convPlexus return type

@@ -346,10 +306,14 @@ export class TracePageImpl extends React.PureComponent<TracePageProps, TracePage
return <ErrorMessage className="ub-m3" error={trace.error || 'Unknown error'} />;
}

// $FlowIgnore because flow believes Set<string> cannot be assigned to Set<string | number>
const findMatches: Set<string | number> = traceGraphView
Copy link
Member

Choose a reason for hiding this comment

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

If uiFind is empty, then filtering the vertices or spans is unnecessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both of the filter functions that can be called have a default return for falsy uiFind arguments.

packages/jaeger-ui/src/utils/update-ui-find.js Outdated Show resolved Hide resolved
rubenvp8510 and others added 3 commits March 7, 2019 18:54
* Process FOLLOWS_FROM spans in TraceView

Signed-off-by: Ruben Vargas <[email protected]>

* Add test for FOLLOWS_FROM span relation for SpanTreeOffset

Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
@everett980 everett980 force-pushed the issue-307-add-node-search-to-tracegraph-and-tracediffgraph branch from 41ebaad to c3d4d14 Compare March 8, 2019 00:10
@everett980
Copy link
Collaborator Author

everett980 commented Mar 8, 2019

DCO seems to not work whenever master is merged into a branch. Which ever commits exists on master and not my branch fail even though they made it to master somehow.
I manually set DCO to pass because that's an option and it's not my commit that's failing, but I don't understand how this PR has had this issue for numerous commits given that I'm not doing anything abnormal.

@everett980 everett980 requested a review from tiffon March 8, 2019 14:49
everett980 and others added 3 commits March 8, 2019 09:58
…aph' into issue-239-fix-TracePageHeader-tests-once-new-version-of-enzyme-is-released
Signed-off-by: Everett Ross <[email protected]>
…ests-once-new-version-of-enzyme-is-released

Update enzyme, Re-enable and add TraceDiff and TracePage tests
Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

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

🎉 The tests look great! 🎉

Some minor comments.

).toEqual([defaultA, defaultB, ...defaultCohortIds, nonDefaultCohortId]);
});

// This test may false negative if previous tests are failing
Copy link
Member

Choose a reason for hiding this comment

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

Why is this test dependent on previous tests? Seems like setting defaultProps in beforeEach(...) would avoid this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the test passes in isolation, but if this test and other tests are failing then the other tests should be investigated first as this one encompasses the functionality of simpler tests.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, not sure I'd call it a false negative. If this test and the simpler tests are failing, this test will still fail if run in isolation.

packages/jaeger-ui/src/components/TraceDiff/url.test.js Outdated Show resolved Hide resolved
@everett980 everett980 dismissed tiffon’s stale review March 11, 2019 19:43

Feedback implemented except for one TODO re types that'll be easier in tyescript

@ghost ghost assigned tiffon Mar 12, 2019
Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

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

Awesome work! 🎉

@tiffon tiffon merged commit 2d5edfd into jaegertracing:master Mar 12, 2019
@ghost ghost removed the review label Mar 12, 2019
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
…de-search-to-tracegraph-and-tracediffgraph

Move filter to query param and highlight filter matches on graphs
Signed-off-by: vvvprabhakar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants