Revisions for search and trace detail embed mode #286

tiffon · 2018-11-30T10:37:17Z

Tweaks to the search page and trace timeline embedded view. 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
Require query parameters to configure embedded to have value=1
Save the embed query params in Redux state instead of passing them
around. Fixes [embed] Enable configuration of any view when in embed mode #279.
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(...)
Rename various component props
Keep redux search query synced with redux search result (and their
processing). Also fixes [Search] Multiple searches can cause the wrong results to show #288.

TODO

Bolster test coverage

Screenshots

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]>

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

codecov · 2018-12-02T06:34:14Z

Codecov Report

Merging #286 into master will increase coverage by 0.42%.
The diff coverage is 67.56%.

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
+ Coverage   81.76%   82.19%   +0.42%     
==========================================
  Files         139      141       +2     
  Lines        3066     3100      +34     
  Branches      633      645      +12     
==========================================
+ Hits         2507     2548      +41     
+ Misses        448      438      -10     
- Partials      111      114       +3

Impacted Files	Coverage Δ
packages/jaeger-ui/src/components/App/index.js	`100% <ø> (ø)`	⬆️
...racePage/TracePageHeader/SpanGraph/ViewingLayer.js	`92.85% <ø> (ø)`
...s/SearchTracePage/SearchResults/ResultItemTitle.js	`80% <ø> (ø)`	⬆️
...ts/TracePage/TracePageHeader/SpanGraph/Scrubber.js	`100% <ø> (ø)`
...ePage/TracePageHeader/SpanGraph/CanvasSpanGraph.js	`100% <ø> (ø)`
...ts/TracePage/TracePageHeader/TracePageSearchBar.js	`75% <ø> (ø)`
.../TracePage/TracePageHeader/SpanGraph/GraphTicks.js	`100% <ø> (ø)`
...omponents/TraceDiff/TraceDiffHeader/TraceHeader.js	`14.28% <ø> (ø)`	⬆️
...nents/TracePage/TracePageHeader/SpanGraph/index.js	`76.92% <ø> (ø)`
...TracePage/TracePageHeader/TracePageHeader.track.js	`80% <ø> (ø)`
... and 31 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 0a36984...1c90199. Read the comment docs.

- Keep redux search query synced with redux search result (and their processing). Also fixes #288. - Bolster unit tests Signed-off-by: Joe Farro <[email protected]>

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

aljesusg · 2018-12-04T09:42:33Z

packages/jaeger-ui/src/utils/embedded-url.js

+// uiEmbed=v0
+// uiSearchHideGraph=1
+// uiTimelineHideMap
+// uiTimelineHideDetails


// uiTimelineShowMap=1 to enable
// uiTimelineShowDetails=1 to enable

packages/jaeger-ui/src/components/App/Page.js

packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.js

everett980 · 2018-12-05T20:48:31Z

packages/jaeger-ui/src/components/TracePage/index.js

-          {embed ? (
-            <TracePageHeaderEmbed {...tracePageProps} {...tracePageEmbedProps} />
+          {embedded ? (
+            <TracePageHeaderEmbed {...headerProps} {...headerEmbeddedProps} />


Should this component be renamed to TracePageHeaderEmbedded to match general change from embed to embedded?

packages/jaeger-ui/src/components/TracePage/index.test.js

packages/jaeger-ui/src/components/App/Page.js

packages/jaeger-ui/src/components/TracePage/index.test.js

aljesusg · 2018-12-10T15:18:52Z

Hi @tiffon , I am working in one of the use of case with this...I wonder if for example in this gif if user navigate from search results to the trave view (inside an iframe) he can't change preferences to show details or minimap. What do you think about to add some buttons in the UI to enable/disable this too?

tiffon · 2018-12-14T01:46:50Z

@aljesusg Thanks for the suggestion! How about we default the minimap and details to collapsed but the user can expand the trace name to show them. This would be the default. We can also have a mode where the embed params can either disable showing the minimap and details, entirely, (each independently) or have them showing, by default?

aljesusg · 2018-12-14T06:45:34Z

Maybe we should show them by default, anyway from searchtraces page you can pass the query Params related with trace pages like uiShowDetails and the UI propagate this param to the view when the user click a trace in the result.

- 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]>

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

tiffon · 2018-12-16T22:58:05Z

@aljesusg Thanks for the suggestions!

@everett980 Thanks for the review!

I've made the following changes to the embedded mode:

The TracePageHeader is collapsible when embedded, and it defaults to open
- uiTimelineCollapseTitle=1 will default it to collapsed
The summary data (number of services, etc.) and minimap are now shown, by default
- uiTimelineHideMinimap=1 will hide the minimap
- uiTimelineHideSummary=1 will hide the trace summary
- If both are hidden, the title is no longer collapsible because there is nothing to show
I've restyled the TracePageHeader; I thought it looked more organized than the current, non-embedded header, so I consolidated the two classes

Here are a few screenshots. The search page hasn't changed.

Normal, header is open

Normal, header is collapsed

Normal with back button to return to search results, header is open

Normal with back button, header is collapsed

Embedded with back button, header is open

Embedded with back button, header is collapsed

Embedded with back button, summary is hidden

Embedded with back button, minimap is hidden

Embedded with back button, both summary and minimap are hidden

Embedded without a back button, collapsed

aljesusg · 2018-12-18T09:51:27Z

Hi @tiffon , I tested it, nice refactor and enhancement. Thanks !!!

everett980

Looks good! There are two it specs that could hardened. Additionally, we should commit to the premise of "versioned embedding" with regards to properties such as showHortcutsHelp. Also, there are some test data that could be a little clearer but aren't terrible as is. Aside from that, it seems ready to merge imo.

packages/jaeger-ui/src/components/TracePage/TracePageHeader/TracePageHeader.test.js

everett980 · 2018-12-18T22:40:01Z

packages/jaeger-ui/src/components/TracePage/index.js

-      onGoFullViewClicked: this.goFullView,
+      resultCount: findMatchesIDs ? findMatchesIDs.size : 0,
+      showArchiveButton: !isEmbedded && archiveEnabled,
+      showShortcutsHelp: !isEmbedded,


to follow the idea of versioned embedding, should these properties be part of the embedded state so that future versions of embedded could affect these separately? i.e.: showShortCutsHelp = embedded.showShortCutsHelp,

Right now, their visibility is implicit in uiEmbed=v0.

Down the road, if we make them configurable, then we would either add new properties to show the (but leave the default the same, which would not be a breaking change) or show them by default and add a property to hide them. The second scenario would require uiEmbed=v1, and uiEmbed=v0 would not change.

Or do you mean the properties would not be configurable but they would be in the embedded state?

everett980 · 2018-12-18T22:51:03Z

packages/jaeger-ui/src/reducers/trace.test.js

+
+  it('handles a failed request', () => {
+    const error = 'error-info';
+    const traces = { preExisting: Math.random() };


here and other places:
why use Math.random() as "filler data"? It seems as though it'd be clearer to simply write { preExistingID: 'preExistingTraceData' }

everett980 · 2018-12-18T22:53:21Z

packages/jaeger-ui/src/components/TracePage/index.test.js

-    const id = 'abc';
-    const trace = {};
-    const state = {
+  const id = 'abc';


clearer variable names and values in such a large file could help. since this is a traceID (I believe), the variable name and value could be traceID. It interferes with object shorthand below but is clearer if there are errors.

everett980 · 2018-12-18T22:54:02Z

packages/jaeger-ui/src/components/TracePage/index.test.js

-    const state = {
+  const id = 'abc';
+  const trace = {};
+  const embedded = Math.random();


I mentioned this below as well, why use Math.random(), an equally valid, but easier to debug test value could be const embedded = 'truthy embedded value';

Signed-off-by: Joe Farro <[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]>

* 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: vvvprabhakar <[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: vvvprabhakar <[email protected]>

ghost assigned tiffon Nov 30, 2018

ghost added the review label Nov 30, 2018

tiffon requested a review from saminzadeh November 30, 2018 10:37

tiffon added 2 commits December 1, 2018 23:19

Merge branch 'master' into embed-tweaks

cf1f653

Fix test break from merging master

ec9e421

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

tiffon added 3 commits December 2, 2018 01:30

Keep redux search query synced with results

5814f6c

- Keep redux search query synced with redux search result (and their processing). Also fixes #288. - Bolster unit tests Signed-off-by: Joe Farro <[email protected]>

Merge branch 'master' into embed-tweaks

c395af5

Fix typo

be681f2

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

tiffon changed the title ~~[WIP] Misc tweaks for search and trace detail embed mode~~ Misc tweaks for search and trace detail embed mode Dec 2, 2018

tiffon changed the title ~~Misc tweaks for search and trace detail embed mode~~ Revisions for search and trace detail embed mode Dec 2, 2018

aljesusg reviewed Dec 4, 2018

View reviewed changes

everett980 reviewed Dec 5, 2018

View reviewed changes

packages/jaeger-ui/src/components/App/Page.js Show resolved Hide resolved

everett980 reviewed Dec 5, 2018

View reviewed changes

packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.js Outdated Show resolved Hide resolved

everett980 requested changes Dec 6, 2018

View reviewed changes

aljesusg mentioned this pull request Dec 11, 2018

Add UI docs jaegertracing/documentation#185

Merged

tiffon added 3 commits December 16, 2018 17:10

Fix typo disableComparisions

e0cb1d7

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

Use public registry to newly installed packages

2569181

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

everett980 approved these changes Dec 18, 2018

View reviewed changes

tiffon added 2 commits December 19, 2018 17:17

Merge branch 'master' into embed-tweaks

c2e9f0e

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

Test improvements

1c90199

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

tiffon merged commit 53937eb into master Dec 20, 2018

ghost removed the review label Dec 20, 2018

tiffon mentioned this pull request Jan 9, 2019

Revive the changelog #300

Merged

yurishkuro deleted the embed-tweaks branch January 29, 2020 15:08

lobaorn mentioned this pull request Dec 16, 2023

Support showing other view modes in Digma embedded Jaeger digma-ai/jaeger-ui#45

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revisions for search and trace detail embed mode #286

Revisions for search and trace detail embed mode #286

tiffon commented Nov 30, 2018 •

edited

Loading

codecov bot commented Dec 2, 2018 •

edited

Loading

aljesusg Dec 4, 2018

everett980 Dec 5, 2018

aljesusg commented Dec 10, 2018

tiffon commented Dec 14, 2018

aljesusg commented Dec 14, 2018

tiffon commented Dec 16, 2018

aljesusg commented Dec 18, 2018

everett980 left a comment

everett980 Dec 18, 2018

tiffon Dec 19, 2018

tiffon Dec 19, 2018

everett980 Dec 18, 2018 •

edited

Loading

everett980 Dec 18, 2018

everett980 Dec 18, 2018

Revisions for search and trace detail embed mode #286

Revisions for search and trace detail embed mode #286

Conversation

tiffon commented Nov 30, 2018 • edited Loading

TODO

Screenshots

codecov bot commented Dec 2, 2018 • edited Loading

Codecov Report

aljesusg Dec 4, 2018

Choose a reason for hiding this comment

everett980 Dec 5, 2018

Choose a reason for hiding this comment

aljesusg commented Dec 10, 2018

tiffon commented Dec 14, 2018

aljesusg commented Dec 14, 2018

tiffon commented Dec 16, 2018

Normal, header is open

Normal, header is collapsed

Normal with back button to return to search results, header is open

Normal with back button, header is collapsed

Embedded with back button, header is open

Embedded with back button, header is collapsed

Embedded with back button, summary is hidden

Embedded with back button, minimap is hidden

Embedded with back button, both summary and minimap are hidden

Embedded without a back button, collapsed

aljesusg commented Dec 18, 2018

everett980 left a comment

Choose a reason for hiding this comment

everett980 Dec 18, 2018

Choose a reason for hiding this comment

tiffon Dec 19, 2018

Choose a reason for hiding this comment

tiffon Dec 19, 2018

Choose a reason for hiding this comment

everett980 Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

everett980 Dec 18, 2018

Choose a reason for hiding this comment

everett980 Dec 18, 2018

Choose a reason for hiding this comment

tiffon commented Nov 30, 2018 •

edited

Loading

codecov bot commented Dec 2, 2018 •

edited

Loading

everett980 Dec 18, 2018 •

edited

Loading