Reorganize span detail components #64

tiffon · 2017-08-22T08:16:52Z

For now, target branch is refactor-trace-detail-selectors, PR #53 is a WIP.

Split sub-components of SpanDetail out into separate files.
Move styles into CSS (addresses this PR comment)
Misc style and DOM tweaks (see attached screenshot)
- Expanded table styling
- Tag summary styling
- Background on hover of expandable / collapsable elements
- Debug info

TODO - Test are currently in a bad state - Finish shifting to transformed trace, exclusively, instead of raw trace - Misc cleanup (unused / ordering of vars, imports, etc) is outstanding while changes are WIP - Continued refinement of trace-detail components, selectors and utils - Many styles will be moved to CSS - Will likely remove `model/trace-viewer.js` SpanGraph - Fix #49 - Span position in graph doesn not match its position in the detail - Ticks in span graph made to match trace detail (in number and formatting) - Span graph refactored to trim down files and DOM elements TracePageHeader - `trace` prop removed - Added props for various title values instead of deriving them from `trace` Trace Detail - Several components split out into separate files - `transformTrace` to use alread span tree to determine span depth Span Detail - Fix uber/jaeger #326: extraneous scrollbars in trace views - Fix: Some tags were not being rendered due to clashing keys (observed in a log message) - Tall content scrolls via entire table instead of single table cell - Horizontal scrolling for wide content (e.g. long log values) - Full width of the header is clickable for tags, process, and logs headers (instead of header text, only) - Service and endpoint are shown on mouseover anywhere in span bar row Misc - Several TraceTimelineViewer / utils removed - `TreeNode` `.walk()` method can now be used to calculate the depth, avoiding use of less efficient `.getPath()` - Removed several `console.error` warnings caused by React key issues

TODO - Test are currently in a bad state - Finish shifting to transformed trace, exclusively, instead of raw trace - Misc cleanup (unused / ordering of vars, imports, etc) is outstanding while changes are WIP - Continued refinement of trace-detail components, selectors and utils - Many styles will be moved to CSS - Will likely remove `model/trace-viewer.js` Span Bar / Detail - Label on span bars no longer off-screen - Clip or hide span bars when zoomed in (insted of flush left) - Add shadow to left / right boundary when span bar view is clipped - Darkened span name column to differentiate from span bar section - Span detail left column color coded by service - Clicking span detail left column collapses detail - Clicking anywhere left of parent span name toggles children visibility

Sub-page scrolling used for trace detail (TODO: revert this). This lays the ground work for using react-virtualized, but unfortunately has major perf issues, hence the TODO: revert. yarn upgrade --latest. Notable changes: - Removed react-sticky - react-router v4 - react-vis CSS needed to be included via a sym-link Misc tweaks: - Styling adjusted on trace mini-map - Scatterplot dots are sized based on number of spans - Scatterplot dots mouseover shows trace name

Add back the styling that adds an ellipsis to truncated span name text.

https://github.com/uber/jaeger-ui/pull/53#discussion_r134313013

https://github.com/uber/jaeger-ui/pull/53#discussion_r134314020

https://github.com/uber/jaeger-ui/pull/53#discussion_r134316418

https://github.com/uber/jaeger-ui/pull/53#discussion_r134318188

coveralls · 2017-08-22T08:20:49Z

Changes Unknown when pulling 83b28d5 on span-detail-reorg into ** on refactor-trace-detail-selectors**.

jcorbin · 2017-08-22T16:25:33Z

Awesome, love the screenshot btw, makes reviewing this at a high/user level much easier.

My only feedback is: what if instead of hiding (how even do we show?) the span id, why not float it in the upper right (across from the Big Bold operation name)? I'd even be fine if you wanted to soften the grey contrast a bit so it doesn't draw as much attention.

https://github.com/uber/jaeger-ui/pull/53#discussion_r134321990

yurishkuro · 2017-08-22T16:29:19Z

I suggest keeping "SpanID:" label before the hex value in bottom right.

Otherwise looks great.

tiffon · 2017-08-22T18:33:27Z

@jcorbin

My only feedback is: what if instead of hiding (how even do we show?) the span id, why not float it in the upper right (across from the Big Bold operation name)?

Currently, the span ID is always showing in the lower right corner of the detail panel (the down arrow points to it). Do you mean moving it to the upper right instead of the lower right, or were you thinking it wasn't being shown at all?

tiffon · 2017-08-22T18:44:34Z

Screenshot of "SpanID:" label added.

https://github.com/uber/jaeger-ui/pull/64#issuecomment-324080675

coveralls · 2017-08-22T18:53:23Z

Coverage decreased (-0.03%) to 55.955% when pulling 9bdf8b6 on span-detail-reorg into bdd6d7e on refactor-trace-detail-selectors.

coveralls · 2017-08-22T18:53:42Z

Coverage increased (+0.03%) to 56.017% when pulling 9bdf8b6 on span-detail-reorg into bdd6d7e on refactor-trace-detail-selectors.

jcorbin · 2017-08-22T21:27:07Z

Ahh okay then, I was only suggesting that upper right seems easier to get to, rather than scroll past one or more expanded-or-collapsed sections. It's also a top-level property, similar to tags or operation name in my mind. Maybe needing span ids isn't a common enough thing to worry about tho... not sure.

yurishkuro · 2017-08-22T22:08:25Z

Maybe needing span ids isn't a common enough thing to worry about tho..

it isn't, imo, only used by "experts", e.g. when you want to deep dive into trace JSON

saminzadeh · 2017-08-23T15:50:00Z

src/components/App/App.css

+  float: none;
+}
+
+.u-cursor-pointer {


what does the u prefix mean?

@saminzadeh util

Got it, I recommend bringing this in for CSS utils:
http://basscss.com/

saminzadeh · 2017-08-23T15:50:55Z

src/components/TracePage/TraceTimelineViewer/SpanDetail/AccordianKeyValues.js

+
+import './AccordianKeyValues.css';
+
+const dataPropType = PropTypes.arrayOf(


Adding flow typing, which is handy for props, definitely reminded me why I'm skeptical — bugs.

Changes that gave me trouble

Underlying issue

Simplified example

saminzadeh · 2017-08-23T15:55:50Z

src/components/TracePage/TraceTimelineViewer/SpanDetail/AccordianLogs.js

+  timestamp: PropTypes.number.isRequired,
+};
+
+export default toggleEnhancer(AccordianLogs);


Good use of HOC here

@saminzadeh Thanks

saminzadeh · 2017-08-23T22:14:00Z

src/components/App/App.css

+  float: none;
+}
+
+.u-cursor-pointer {


Got it, I recommend bringing this in for CSS utils:
http://basscss.com/

https://github.com/uber/jaeger-ui/pull/64#discussion_r134792924

coveralls · 2017-08-24T05:30:07Z

Coverage decreased (-0.2%) to 55.776% when pulling 5c7f896 on span-detail-reorg into bdd6d7e on refactor-trace-detail-selectors.

* Update prettier and wrap at 110 instead of 80 * WIP commit for refactoring trace detail view TODO - Test are currently in a bad state - Finish shifting to transformed trace, exclusively, instead of raw trace - Misc cleanup (unused / ordering of vars, imports, etc) is outstanding while changes are WIP - Continued refinement of trace-detail components, selectors and utils - Many styles will be moved to CSS - Will likely remove `model/trace-viewer.js` SpanGraph - Fix #49 - Span position in graph doesn not match its position in the detail - Ticks in span graph made to match trace detail (in number and formatting) - Span graph refactored to trim down files and DOM elements TracePageHeader - `trace` prop removed - Added props for various title values instead of deriving them from `trace` Trace Detail - Several components split out into separate files - `transformTrace` to use alread span tree to determine span depth Span Detail - Fix uber/jaeger #326: extraneous scrollbars in trace views - Fix: Some tags were not being rendered due to clashing keys (observed in a log message) - Tall content scrolls via entire table instead of single table cell - Horizontal scrolling for wide content (e.g. long log values) - Full width of the header is clickable for tags, process, and logs headers (instead of header text, only) - Service and endpoint are shown on mouseover anywhere in span bar row Misc - Several TraceTimelineViewer / utils removed - `TreeNode` `.walk()` method can now be used to calculate the depth, avoiding use of less efficient `.getPath()` - Removed several `console.error` warnings caused by React key issues * WIP commit for refactoring trace detail view TODO - Test are currently in a bad state - Finish shifting to transformed trace, exclusively, instead of raw trace - Misc cleanup (unused / ordering of vars, imports, etc) is outstanding while changes are WIP - Continued refinement of trace-detail components, selectors and utils - Many styles will be moved to CSS - Will likely remove `model/trace-viewer.js` Span Bar / Detail - Label on span bars no longer off-screen - Clip or hide span bars when zoomed in (insted of flush left) - Add shadow to left / right boundary when span bar view is clipped - Darkened span name column to differentiate from span bar section - Span detail left column color coded by service - Clicking span detail left column collapses detail - Clicking anywhere left of parent span name toggles children visibility * WIP refactor trace detail, split out css, start fixing tests * WIP refactor trace detail, tests working again * WIP refactor trace, yarn upgrade Sub-page scrolling used for trace detail (TODO: revert this). This lays the ground work for using react-virtualized, but unfortunately has major perf issues, hence the TODO: revert. yarn upgrade --latest. Notable changes: - Removed react-sticky - react-router v4 - react-vis CSS needed to be included via a sym-link Misc tweaks: - Styling adjusted on trace mini-map - Scatterplot dots are sized based on number of spans - Scatterplot dots mouseover shows trace name * WIP refactor trace, test maintenance * remove unused import * add license to css files * Revert sub-page scrolling for trace detail * Support URL prefix via homepage in package.json * Prevent collision of logs in log entries table * Add comments to new utils * WIP Use react-virtualized in trace detail view * Fix #59 - "Span Name" to "Service & Operation" * Fix unreleased regression - ellipsis on span name Add back the styling that adds an ellipsis to truncated span name text. * Address PR comment on search results scatter plot https://github.com/uber/jaeger-ui/pull/53#discussion_r134313013 * Misc cleanup from PR comment https://github.com/uber/jaeger-ui/pull/53#discussion_r134314020 * PR comment - Remove ms and use nano seconds https://github.com/uber/jaeger-ui/pull/53#discussion_r134316418 * PR comment - Adjust export, relates to recompose https://github.com/uber/jaeger-ui/pull/53#discussion_r134318188 * Reorganize span detail components * PR comment - Comment getTraceSpanIdsAsTree() https://github.com/uber/jaeger-ui/pull/53#discussion_r134321990 * PR comment - Add "SpanID:" label https://github.com/uber/jaeger-ui/pull/64#issuecomment-324080675 * WIP react-virtualized layout progress Outstanding: - Window scroller used https://bvaughn.github.io/react-virtualized/#/components/WindowScroller - Expanding span details - Collapsing / expanding children * WIP react-virtualized layout progress Switching to use a props oriented ui state management for span details. This is necessary because the virtualized scroller removes and adds span details as it scrolls. The span detail components cannot retain their own state (e.g. tags table is expanded / collapsed, etc.) because they are not long-lived. * PR comment - Flow typing for props https://github.com/uber/jaeger-ui/pull/64#discussion_r134792924 * WIP Switching react-virtualized to custom solution * fix file-header licenses * WIP Switching react-virtualized to custom solution * Add flow to ListView, comment ListView, Positions * Positions tests, fix edge-case bug in Positions * Fix comment issue from prettier * Unit tests for ListView, remove unused propType/* * Minor changes to a comment and a test case description * Unit tests for TraceTimelineViewer/duck * remove react-virtualized from package.json * PR feedback - JSDoc, move trace transform - JSDoc tweaks - documentation.js can infer types automatically - Move trace transform - Move trace tranform out of src/components/TracePage/TraceTimelineViewer/transforms and into src/model/transform-trace-data.js - Apply trace transform to data from HTTP response so the trace data stored in the state is always the enriched form * Use top-level redux store for traceTimeline state

* PR comment - Add "SpanID:" label https://github.com/uber/jaeger-ui/pull/64#issuecomment-324080675 * PR comment - Flow typing for props https://github.com/uber/jaeger-ui/pull/64#discussion_r134792924 Signed-off-by: vvvprabhakar <[email protected]>

* Update prettier and wrap at 110 instead of 80 * WIP commit for refactoring trace detail view TODO - Test are currently in a bad state - Finish shifting to transformed trace, exclusively, instead of raw trace - Misc cleanup (unused / ordering of vars, imports, etc) is outstanding while changes are WIP - Continued refinement of trace-detail components, selectors and utils - Many styles will be moved to CSS - Will likely remove `model/trace-viewer.js` SpanGraph - Fix #49 - Span position in graph doesn not match its position in the detail - Ticks in span graph made to match trace detail (in number and formatting) - Span graph refactored to trim down files and DOM elements TracePageHeader - `trace` prop removed - Added props for various title values instead of deriving them from `trace` Trace Detail - Several components split out into separate files - `transformTrace` to use alread span tree to determine span depth Span Detail - Fix uber/jaeger jaegertracing#326: extraneous scrollbars in trace views - Fix: Some tags were not being rendered due to clashing keys (observed in a log message) - Tall content scrolls via entire table instead of single table cell - Horizontal scrolling for wide content (e.g. long log values) - Full width of the header is clickable for tags, process, and logs headers (instead of header text, only) - Service and endpoint are shown on mouseover anywhere in span bar row Misc - Several TraceTimelineViewer / utils removed - `TreeNode` `.walk()` method can now be used to calculate the depth, avoiding use of less efficient `.getPath()` - Removed several `console.error` warnings caused by React key issues * WIP commit for refactoring trace detail view TODO - Test are currently in a bad state - Finish shifting to transformed trace, exclusively, instead of raw trace - Misc cleanup (unused / ordering of vars, imports, etc) is outstanding while changes are WIP - Continued refinement of trace-detail components, selectors and utils - Many styles will be moved to CSS - Will likely remove `model/trace-viewer.js` Span Bar / Detail - Label on span bars no longer off-screen - Clip or hide span bars when zoomed in (insted of flush left) - Add shadow to left / right boundary when span bar view is clipped - Darkened span name column to differentiate from span bar section - Span detail left column color coded by service - Clicking span detail left column collapses detail - Clicking anywhere left of parent span name toggles children visibility * WIP refactor trace detail, split out css, start fixing tests * WIP refactor trace detail, tests working again * WIP refactor trace, yarn upgrade Sub-page scrolling used for trace detail (TODO: revert this). This lays the ground work for using react-virtualized, but unfortunately has major perf issues, hence the TODO: revert. yarn upgrade --latest. Notable changes: - Removed react-sticky - react-router v4 - react-vis CSS needed to be included via a sym-link Misc tweaks: - Styling adjusted on trace mini-map - Scatterplot dots are sized based on number of spans - Scatterplot dots mouseover shows trace name * WIP refactor trace, test maintenance * remove unused import * add license to css files * Revert sub-page scrolling for trace detail * Support URL prefix via homepage in package.json * Prevent collision of logs in log entries table * Add comments to new utils * WIP Use react-virtualized in trace detail view * Fix jaegertracing#59 - "Span Name" to "Service & Operation" * Fix unreleased regression - ellipsis on span name Add back the styling that adds an ellipsis to truncated span name text. * Address PR comment on search results scatter plot https://github.com/uber/jaeger-ui/pull/53#discussion_r134313013 * Misc cleanup from PR comment https://github.com/uber/jaeger-ui/pull/53#discussion_r134314020 * PR comment - Remove ms and use nano seconds https://github.com/uber/jaeger-ui/pull/53#discussion_r134316418 * PR comment - Adjust export, relates to recompose https://github.com/uber/jaeger-ui/pull/53#discussion_r134318188 * Reorganize span detail components * PR comment - Comment getTraceSpanIdsAsTree() https://github.com/uber/jaeger-ui/pull/53#discussion_r134321990 * PR comment - Add "SpanID:" label https://github.com/uber/jaeger-ui/pull/64#issuecomment-324080675 * WIP react-virtualized layout progress Outstanding: - Window scroller used https://bvaughn.github.io/react-virtualized/#/components/WindowScroller - Expanding span details - Collapsing / expanding children * WIP react-virtualized layout progress Switching to use a props oriented ui state management for span details. This is necessary because the virtualized scroller removes and adds span details as it scrolls. The span detail components cannot retain their own state (e.g. tags table is expanded / collapsed, etc.) because they are not long-lived. * PR comment - Flow typing for props https://github.com/uber/jaeger-ui/pull/64#discussion_r134792924 * WIP Switching react-virtualized to custom solution * fix file-header licenses * WIP Switching react-virtualized to custom solution * Add flow to ListView, comment ListView, Positions * Positions tests, fix edge-case bug in Positions * Fix comment issue from prettier * Unit tests for ListView, remove unused propType/* * Minor changes to a comment and a test case description * Unit tests for TraceTimelineViewer/duck * remove react-virtualized from package.json * PR feedback - JSDoc, move trace transform - JSDoc tweaks - documentation.js can infer types automatically - Move trace transform - Move trace tranform out of src/components/TracePage/TraceTimelineViewer/transforms and into src/model/transform-trace-data.js - Apply trace transform to data from HTTP response so the trace data stored in the state is always the enriched form * Use top-level redux store for traceTimeline state Signed-off-by: vvvprabhakar <[email protected]>

tiffon added 20 commits August 11, 2017 01:21

Update prettier and wrap at 110 instead of 80

fe35bbd

WIP refactor trace detail, split out css, start fixing tests

93a6fb7

WIP refactor trace detail, tests working again

b8945ab

WIP refactor trace, test maintenance

c34bbd3

remove unused import

c041353

add license to css files

9f54454

Revert sub-page scrolling for trace detail

c657cf1

Support URL prefix via homepage in package.json

2226744

Prevent collision of logs in log entries table

f555e8b

Add comments to new utils

bef7f5b

Fix #59 - "Span Name" to "Service & Operation"

fa6f016

Fix unreleased regression - ellipsis on span name

3797f5b

Add back the styling that adds an ellipsis to truncated span name text.

Address PR comment on search results scatter plot

0fd43f5

https://github.com/uber/jaeger-ui/pull/53#discussion_r134313013

Misc cleanup from PR comment

542c1ba

https://github.com/uber/jaeger-ui/pull/53#discussion_r134314020

PR comment - Remove ms and use nano seconds

42736e0

https://github.com/uber/jaeger-ui/pull/53#discussion_r134316418

PR comment - Adjust export, relates to recompose

d16097b

https://github.com/uber/jaeger-ui/pull/53#discussion_r134318188

Reorganize span detail components

83b28d5

tiffon requested review from yurishkuro, saminzadeh and a team August 22, 2017 08:16

tiffon mentioned this pull request Aug 22, 2017

Refactor trace detail #53

Merged

9 tasks

PR comment - Comment getTraceSpanIdsAsTree()

bdd6d7e

https://github.com/uber/jaeger-ui/pull/53#discussion_r134321990

tiffon added 2 commits August 22, 2017 14:48

PR comment - Add "SpanID:" label

7204684

https://github.com/uber/jaeger-ui/pull/64#issuecomment-324080675

Merge branch 'refactor-trace-detail-selectors' into span-detail-reorg

9bdf8b6

saminzadeh reviewed Aug 23, 2017

View reviewed changes

saminzadeh approved these changes Aug 23, 2017

View reviewed changes

tiffon added 2 commits August 24, 2017 01:12

Merge branch 'master' into span-detail-reorg

0113985

PR comment - Flow typing for props

5c7f896

https://github.com/uber/jaeger-ui/pull/64#discussion_r134792924

tiffon changed the base branch from refactor-trace-detail-selectors to master August 25, 2017 18:59

tiffon merged commit f3ea98b into master Aug 25, 2017

yurishkuro deleted the span-detail-reorg branch January 29, 2020 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganize span detail components #64

Reorganize span detail components #64

tiffon commented Aug 22, 2017 •

edited

Loading

coveralls commented Aug 22, 2017

jcorbin commented Aug 22, 2017

yurishkuro commented Aug 22, 2017

tiffon commented Aug 22, 2017

tiffon commented Aug 22, 2017

coveralls commented Aug 22, 2017

coveralls commented Aug 22, 2017

jcorbin commented Aug 22, 2017

yurishkuro commented Aug 22, 2017

saminzadeh Aug 23, 2017

tiffon Aug 23, 2017

saminzadeh Aug 23, 2017

saminzadeh Aug 23, 2017

tiffon Aug 24, 2017

saminzadeh Aug 23, 2017

tiffon Aug 23, 2017

saminzadeh Aug 23, 2017

coveralls commented Aug 24, 2017


		import './AccordianKeyValues.css';

		const dataPropType = PropTypes.arrayOf(

Reorganize span detail components #64

Reorganize span detail components #64

Conversation

tiffon commented Aug 22, 2017 • edited Loading

coveralls commented Aug 22, 2017

jcorbin commented Aug 22, 2017

yurishkuro commented Aug 22, 2017

tiffon commented Aug 22, 2017

tiffon commented Aug 22, 2017

coveralls commented Aug 22, 2017

coveralls commented Aug 22, 2017

jcorbin commented Aug 22, 2017

yurishkuro commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 24, 2017

tiffon commented Aug 22, 2017 •

edited

Loading