Move redux and focusSpan up from VirtualizedTraceView #517

aocenas · 2020-02-06T13:53:06Z

Signed-off-by: Andrej Ocenas [email protected]

Which problem is this PR solving?

Part of: Extract TracePage components [Subtask] #508

Short description of the changes

Pushing redux and url creation (focusSpan) dependencies up from VirtualizedTraceView and TraceTimelineViewer to TracePage so it will be easier to move them to separate package.

Signed-off-by: Andrej Ocenas <[email protected]>

aocenas · 2020-02-06T13:55:34Z

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

 });

 describe('mapDispatchToProps()', () => {
  it('creates the actions correctly', () => {
-    expect(mapDispatchToProps(() => {})).toEqual({
+    expect(mapDispatchToProps(() => {})).toMatchObject({


There are few more functions because of the usage of:

...((bindActionCreators(timelineActions, dispatch) as unknown) as TDispatchPropsTimelineViewer),

In the map dispatch see a comment there.

aocenas · 2020-02-06T13:56:52Z

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

+    acknowledgeArchive,
+    archiveTrace,
+    fetchTrace,
+    ...((bindActionCreators(timelineActions, dispatch) as unknown) as TDispatchPropsTimelineViewer),


Not sure if the preferred way to do this. Seen it at few places and is definitely shorter than destructuring all the functions and then assign them again, but there is more stuff passed to the component than necessary needed.

I admittedly have a bias towards verbosity, but I find it clearer and preferable to destructure everything that is necessary and pass it along, rather than (bindActionCreators(actions, dispatch) as unknown) as TType.

It's a little more verbose in this case, but I'd still prefer it over the cast.

aocenas · 2020-02-06T14:05:05Z

Moving the redux up and passing an all the handlers makes for a pretty big props list but also I realised that all of the redux actions are simple UI toggles like expanding span. I wonder if this is so that navigating to search and back to the same trace will preserve the UI state or if that is just unneeded side-effect. Because I think lots of those could be hidden in a local component state making the props list shorter and probably making the components a bit easier to use if do not need to preserve the UI state on navigation.

Another question is whether to keep the /TraceTimelineViewer/duck.tsx where it is. I think if we keep the redux state it makes sense to still have it split like this ie having TracePage/traceTimelineViewer.duck.tsx and TracePage/archiveNotifier.duck.tsx as separate files but can also be moved and merged into TracePage/duck.tsx later on.

codecov · 2020-02-06T14:12:00Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (8cb13c4) 92.92% compared to head (3539f92) 96.57%.
Report is 751 commits behind head on main.

❗ Current head 3539f92 differs from pull request most recent head e884e7c. Consider uploading reports for the commit e884e7c to get more accurate results

Files	Patch %	Lines
.../jaeger-ui/src/components/QualityMetrics/index.tsx	93.87%	3 Missing ⚠️
...r-ui/src/components/Monitor/ServicesView/index.tsx	98.37%	2 Missing ⚠️
...components/SearchTracePage/SearchResults/index.tsx	90.00%	2 Missing ⚠️
...aeger-ui/src/actions/path-agnostic-decorations.tsx	97.91%	1 Missing ⚠️
packages/jaeger-ui/src/components/App/TopNav.tsx	94.44%	1 Missing ⚠️
...rc/components/DeepDependencies/SidePanel/index.tsx	96.77%	1 Missing ⚠️
...jaeger-ui/src/components/DependencyGraph/index.jsx	95.83%	1 Missing ⚠️
...ger-ui/src/components/Monitor/EmptyState/index.tsx	92.85%	1 Missing ⚠️
...c/components/Monitor/ServicesView/serviceGraph.tsx	98.18%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #517      +/-   ##
==========================================
+ Coverage   92.92%   96.57%   +3.64%     
==========================================
  Files         197      254      +57     
  Lines        4808     7620    +2812     
  Branches     1160     1986     +826     
==========================================
+ Hits         4468     7359    +2891     
+ Misses        299      261      -38     
+ Partials       41        0      -41

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

everett980

Hi @aocenas, thanks again for your patience with this, sorry it took so long to circle back to.

Big picture this looks good, I left some feedback to clean it up a bit and make it a bit DRYer. I don't believe it will take long to address my feedback, but of course let me know if you have any questions or disagree.

everett980 · 2020-02-25T19:39:16Z

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

@@ -80,13 +80,14 @@ describe('makeShortcutCallbacks()', () => {
 });

 describe('<TracePage>', () => {
-  TraceTimelineViewer.prototype.shouldComponentUpdate.mockReturnValue(false);
+  TraceTimelineViewer.mockReturnValue('');


what does this do? I've never mocked the return value of a React component before.

everett980 · 2020-02-25T19:41:49Z

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

+  describe('focusSpan', () => {
+    it('calls updateUiFind and focusUiFindMatches', () => {
+      const spanName = 'span1';
+      wrapper.instance().focusSpan(spanName);


I believe to follow the pattern (generally) present in the code base, this should be: wrapper.find(TraceTimelineView).prop('focusSpan')(spanName);

reason being: testing wrapper.instance().fnName(arg) can lean towards testing implementation, while testing someChildComponent.prop('fnName')(arg) tests functionality (in this case child component is able to set and focus uiFind)

everett980 · 2020-02-25T19:44:17Z

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

 import { TraceArchive } from '../../types/archive';
 import { EmbeddedState } from '../../types/embedded';
 import filterSpans from '../../utils/filter-spans';
 import updateUiFind from '../../utils/update-ui-find';

 import './index.css';
+import TTraceTimeline from '../../types/TTraceTimeline';
+
+type TDispatchPropsTimelineViewer = {


this type should exist somewhere that VirtualizedTraceView can import it, to be DRYer.

perhaps TTraceTimelineDispatchFns in ../../types/TTraceTimeline

everett980 · 2020-02-25T19:46:17Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.tsx


-type TDispatchProps = {
+  // was from redux


rather than commenting this, a type should be imported and TVirtualizedTraceViewOwnProps can be called TProps (no separation of own vs dispatch props) which would look like:

TProps = { currentViewRangeTime: [number, number]; findMatchesIDs: Set<string> | TNil; focusSpan: (uiFind: string) => void; scrollToFirstVisibleSpan: () => void; registerAccessors: (accesors: Accessors) => void; trace: Trace; } & TImportedType;

everett980 · 2020-02-25T19:47:10Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.tsx

@@ -332,6 +310,7 @@ export class VirtualizedTraceViewImpl extends React.Component<VirtualizedTraceVi
      findMatchesIDs,
      spanNameColumnWidth,
      trace,
+      focusSpan,


admittedly nit, but please destructure and pass focusSpan in alphabetical order.

everett980 · 2020-02-25T19:51:05Z

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

@@ -71,17 +90,17 @@ type TOwnProps = {
  match: Match<{ id: string }>;
 };

-type TReduxProps = {
+type TReduxProps = TExtractUiFindFromStateReturn & {


everett980 · 2020-02-25T19:53:03Z

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

+    acknowledgeArchive,
+    archiveTrace,
+    fetchTrace,
+    ...((bindActionCreators(timelineActions, dispatch) as unknown) as TDispatchPropsTimelineViewer),


I admittedly have a bias towards verbosity, but I find it clearer and preferable to destructure everything that is necessary and pass it along, rather than (bindActionCreators(actions, dispatch) as unknown) as TType.

It's a little more verbose in this case, but I'd still prefer it over the cast.

everett980 · 2020-02-25T20:18:51Z

I wonder if this is so that navigating to search and back to the same trace will preserve the UI state or if that is just unneeded side-effect.

While the case of going from a trace back to search back to that trace in a single session is probably pretty rare, I believe this implementation also ensures that going from the Trace Gantt Chart to the Trace Graph and back to the Gantt Chart preserves the visible spans / span details / KV tables within span details. For this reason I believe it visibility state should not be lost on unmount.

However, if we wanted to simplify the props list and keep this functionality, TraceTimelineViewer could tuck its visibility state away into session storage on componentWillUnmount then restored in the constructor (not, not componentWillMount as its deprecated).

Using session storage instead of a highly verbose props list is actually probably the best approach as it will lessen the burden to embed this component.

everett980 · 2020-02-25T20:21:03Z

I think if we keep the redux state it makes sense to still have it split like this ie having TracePage/traceTimelineViewer.duck.tsx and TracePage/archiveNotifier.duck.tsx as separate files but can also be moved and merged into TracePage/duck.tsx later on.

Using session storage makes this a moot point, but in case a similar pattern comes up later:
I also like having the divide between these two ducks. I think the best approach would be to have a TracePage/duck.tsx file that simply imports the two ducks and exports the full redux functionality for TracePage

Move redux up from VirtualizedTraceView

e884e7c

Signed-off-by: Andrej Ocenas <[email protected]>

aocenas changed the title ~~Move redux up from VirtualizedTraceView~~ Move redux and focusSpan up from VirtualizedTraceView Feb 6, 2020

aocenas commented Feb 6, 2020

View reviewed changes

aocenas requested review from everett980 and tiffon February 6, 2020 14:05

aocenas requested a review from yurishkuro February 17, 2020 15:35

aocenas mentioned this pull request Feb 21, 2020

Move up redux from SpanTreeOffset #529

Draft

everett980 requested changes Feb 25, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move redux and focusSpan up from VirtualizedTraceView #517

Move redux and focusSpan up from VirtualizedTraceView #517

aocenas commented Feb 6, 2020

aocenas Feb 6, 2020

aocenas Feb 6, 2020 •

edited

Loading

everett980 Feb 25, 2020

aocenas commented Feb 6, 2020

codecov bot commented Feb 6, 2020 •

edited

Loading

everett980 left a comment

everett980 Feb 25, 2020

everett980 Feb 25, 2020

everett980 Feb 25, 2020

everett980 Feb 25, 2020

everett980 Feb 25, 2020

everett980 Feb 25, 2020

everett980 Feb 25, 2020

everett980 Feb 25, 2020

everett980 commented Feb 25, 2020

everett980 commented Feb 25, 2020

Move redux and focusSpan up from VirtualizedTraceView #517

Are you sure you want to change the base?

Move redux and focusSpan up from VirtualizedTraceView #517

Conversation

aocenas commented Feb 6, 2020

Which problem is this PR solving?

Short description of the changes

Choose a reason for hiding this comment

aocenas Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aocenas commented Feb 6, 2020

codecov bot commented Feb 6, 2020 • edited Loading

Codecov Report

everett980 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

everett980 commented Feb 25, 2020

everett980 commented Feb 25, 2020

aocenas Feb 6, 2020 •

edited

Loading

codecov bot commented Feb 6, 2020 •

edited

Loading