Add totalMemory in config for trace graph #1262
Conversation
ChenX1993
requested review from
everett980,
rubenvp8510 and
tiffon
as code owners
| @@ -164,4 +164,14 @@ export type Config = { | |||
| menuEnabled?: boolean; | |||
| menuLabel?: string; | |||
| }; | |||
|
|
|||
| // traceGraph controls the trace graph under trace page | |||
| traceGraph?: { | |||
There was a problem hiding this comment.
please move this above L157, I don't think this is "experimental"
There was a problem hiding this comment.
Moved.
I originally put it below L157 because I thought it was "undocumented" in https://www.jaegertracing.io/docs/1.42/frontend-ui/
| @@ -435,6 +438,7 @@ export function mapStateToProps(state: ReduxState, ownProps: TOwnProps): TReduxP | |||
| const archiveEnabled = Boolean(config.archiveEnabled); | |||
| const { state: locationState } = router.location; | |||
| const searchUrl = (locationState && locationState.fromSearch) || null; | |||
| const graphLayoutManagerMemory = config.traceGraph?.layoutManagerMemory; | |||
There was a problem hiding this comment.
Added a test.
It is not possible to test any UI change with this option, so I just followed the existing tests to test the propagation from redux state to component properties.
| @@ -82,6 +82,7 @@ type TReduxProps = { | |||
| searchUrl: null | string; | |||
| trace: FetchedTrace | TNil; | |||
| uiFind: string | TNil; | |||
| graphLayoutManagerMemory: number | undefined; | |||
There was a problem hiding this comment.
I'm not an expect in React, so a Q: why does this setting need to be in the properties (which causes more code changes) instead of only passed from config to <TraceGraph>? Or even looked up from config by TraceGraph directly?
There was a problem hiding this comment.
I haven't worked on React for a while, so I might be wrong.
1>
I observed in this repo:
- The config is passed into React components via Redux.
- Only top/page level components (like TracePage in this example) subscribe to Redux, and most sub-components don't.
So I am trying to follow the same style here - have TracePage get config from redux and pass to TraceGraph via property.
2>
Since we let component TracePage subscribe to Redux, then we need to make sure what we need from Redux is exposed as component property by mapStateToProps function defined in the component.
We can either expose the entire config object, or sub fields/objects, but either requires code change - adding into component property definition.
The reason I exposed graphLayoutManagerMemory (now I changed to traceGraphConfig) instead of entire config object is due component re-render.
If any property value of the component changes, it causes component to re-render. The entire config object contains other fields/sub objects that current component doesn't need at all, which might cause unnecessary re-render.
Trace graph has a problem to renders large traces (with ~15K+ spans), and gives the error "Cannot enlarge memory arrays". Need to give users flexibility to control the totalMemory of LayoutManager used for TraceGraph. Signed-off-by: Chen Xu <chen.x@uber.com>
|
The unit-tests check failed at Not very sure why. |
|
sometimes codecov hiccups, I reran the job and tests were fine. |
Trace graph has a problem to renders large traces (with ~15K+ spans), and gives the error "Cannot enlarge memory arrays". Need to give users flexibility to control the totalMemory of LayoutManager used for TraceGraph. <!-- Please delete this comment before posting. We appreciate your contribution to the Jaeger project! 👋🎉 Before creating a pull request, please make sure: - Your PR is solving one problem - You have read the guide for contributing - See https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING.md - You signed all your commits (otherwise we won't be able to merge the PR) - See https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md#certificate-of-origin---sign-your-work - You added unit tests for the new functionality - You mention in the PR description which issue it is addressing, e.g. "Resolves jaegertracing#123" --> - Resolves jaegertracing#1249 Trace graph has a problem to renders large traces (with ~15K+ spans), and gives the error "Cannot enlarge memory arrays". This can be fixed by providing a larger number of memory when initializing the `LayoutManager` in trace graph, like below: ``` // default 16MB this.layoutManager = new LayoutManager({ totalMemory: 33554432, useDotEdges: true, splines: 'polyline' }); ``` But instead of hardcoding the memory value in code, we want to give users the flexibility to control the totalMemory of LayoutManager for TraceGraph. Signed-off-by: Chen Xu <chen.x@uber.com>
Trace graph has a problem to renders large traces (with ~15K+ spans), and gives the error "Cannot enlarge memory arrays". Need to give users flexibility to control the totalMemory of LayoutManager used for TraceGraph.
Which problem is this PR solving?
Short description of the changes
Trace graph has a problem to renders large traces (with ~15K+ spans), and gives the error "Cannot enlarge memory arrays". This can be fixed by providing a larger number of memory when initializing the
LayoutManagerin trace graph, like below:But instead of hardcoding the memory value in code, we want to give users the flexibility to control the totalMemory of LayoutManager for TraceGraph.