Extract TracePage components [Subtask]

[aocenas]

Requirement - what kind of business use case are you trying to solve?

Subtask from #507

Problem - what in Jaeger blocks you from solving the requirement?

Proposal - what do you suggest to solve the problem or improve the existing situation?

Way to extract this depends on what is required to be importable. For Grafana use case we only need part of the Trace page seen in red rectangle:

This means toolbar and archiving components can be either hide-able by props or we could go the route of providing separate more low level components directly like Toolbar, Minimap, TimelineView, GraphView etc to be composed into an application as needed.

(@everett980): I expect this to be the comparatively straightforward part of this effort, however we will need to be mindful of the contract that gets created.
Global dependencies like redux and url state can be worked around by exporting both "connected" and "Impl" (implementation) forms of components, as done/consumed for Deep Dependency Graphs, and then adding a build process to create a standalone file for each desired "Impl" component. Some cleanup will be necessary for TracePage and its subcomponents, and the types should be significantly tightened for these use cases.
Fortunately, tracking is already configurable and off by default.

I think having a mixed connected and unconnected components can be avoided especially if lower level components are exposed. Redux (or other state management) can be left as a concern for the final application. If there is need to sidestep deep props passing, react context api could be used instead to have a localized and separated state management.

Another consideration is Antd component library dependency which may seem as too big dependency for such a package and also contains some global style overrides. This overlaps a bit with theme-ability.

Any open questions to address

• What needs to be exported? One complete page component or a set of lower level components.
• Do we want/need to export connected components in the library?
• Can we remove AntD dependency? If so what to replace it with?

[tiffon]

@everett980, @aocenas you might also consider creating a new package / yarn workspace which holds the components that are to be broken out. A few potential upsides:

• Decisive separation will likely make it much easier to determine what the boundaries between components should be and to understand those boundaries later and for new contributors
• It will likely be much easier to prevent coupling from creeping in
• Bugs that only manifest when the components are imported will be much more likely to be detected since Jaeger UI, itself, would be exercising that scenario (and presumably Uber would dog-food releases, too)
• The UI would serve as documentation for how to import and use the separated components

On a side note, if the components are kept in jaeger-ui, I recommend simply adding an independent build for the separated components. create-react-app is just not a malleable thing.

Also, will these be published as NPM packages or will folks need to checkout the repo in order to make use of the components?

I'm very excited to keep up with this work and see how things shake out. LMK if I can facilitate.

[everett980]

@tiffon, great points about making a separate work space.

In short, we would move packages/jaeger-ui/src/components/TracePage and all of its dependencies to packages/jaeger-ui-components (or some other name), and everything in the new folder would need to be redux/url/other-global-state unaware, right? Using packages/jaeger-ui/src/components/common/UiFindInput as an example: a redux/url unaware version would exist in packages/jaeger-ui-components/src/components/common/UiFind and the existing file would be updated to import that and export a redux & url aware version?

If global state is to be removed from TracePage and its dependencies, I think the VirtualizedTraceView & ScrollManager will be the toughest piece to “de-redux-ify”. This might be a good opportunity to change the scroll/list approach. I remember at some point in time we discussed how it was overcomplicated and scrollIntoView({ behavior: ‘smooth’ }) would have worked instead of ScrollManager. With native smooth scrolling, is there any need to keep the timeline view virtualized? If not, the state management would be much simpler, and the indent guides wouldn’t need any state whatsoever (children spans could be react/DOM children of their parents).

Also, will these be published as NPM packages or will folks need to checkout the repo in order to make use of the components?

If it is a separate workspace, then it probably should be a separate NPM package.
Though we should make sure changes to packages/jaeger-ui-components causes packages/jaeger-ui to rebuild. I've been working on plexus and manually needing to rebuild each is a pain.

[aocenas]

Hey @tiffon and @everett980, thanks for the input and sorry for a bit of delay on my side, was a bit preoccupied lately.

also consider creating a new package / yarn workspace which holds the components that are to be broken out
Also, will these be published as NPM packages or will folks need to checkout the repo in order to make use of the components?

That was what I had in mind so I would say yes to both. Otherwise it would be awkward to use it for Grafana.

In short, we would move packages/jaeger-ui/src/components/TracePage and all of its dependencies to packages/jaeger-ui-components (or some other name), and everything in the new folder would need to be redux/url/other-global-state unaware, right? Using packages/jaeger-ui/src/components/common/UiFindInput as an example: a redux/url unaware version would exist in packages/jaeger-ui-components/src/components/common/UiFind and the existing file would be updated to import that and export a redux & url aware version?

Again almost exactly what I had in mind. Probably only thing would be whether to have TracePage as a component in jaeger-ui-components. To me it feels like the TracePage is mostly layout and Jaeger specific stuff, so for me having its subcomponents available makes more sense so that users can build their own "page" out of them.

Don't have much to say about the particulars of removing redux from VirtualizedTraceView at the moment. My assumption is that at the worst case we can exchange redux for a react context propagation and basically bridge redux -> context on the critical paths to minimise needed rework and refactoring of component internals (at least for initial separation).

[everett980]

Probably only thing would be whether to have TracePage as a component in jaeger-ui-components. To me it feels like the TracePage is mostly layout and Jaeger specific stuff, so for me having its subcomponents available makes more sense so that users can build their own "page" out of them.

Good point @aocenas. It shouldn't be too much harder to build TracePage and its subcomponents. That way users can build their own or have a default option available.

[aocenas]

Started pushing some PRs for this. I want to push a bit smaller PRs to prepare the codebase first as doing both move and change is usually hard to diff and I think this way it will be easier for each PR to be reviewed and get more granular feedback. Let me know if you have some other preference though.

[yurishkuro]

Small PRs are absolutely the preference. Thanks!

[jkowall]

Grafana decided to do their own fork to reskin, so I am closing this out.