Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature]: Adding Config for TotalMemory in LayoutManager for TraceGraph #1249

Closed
ChenX1993 opened this issue Mar 10, 2023 · 1 comment · Fixed by #1262
Closed

[Feature]: Adding Config for TotalMemory in LayoutManager for TraceGraph #1249

ChenX1993 opened this issue Mar 10, 2023 · 1 comment · Fixed by #1262

Comments

@ChenX1993
Copy link
Contributor

ChenX1993 commented Mar 10, 2023

Requirement

As a Jaeger operator,
I want to have TotalMemory option in config so that we have the flexibility to tune it for trace graph with large trace (15K+ spans).

Problem

Today, we face the issue that the trace graph doesn't show up for large traces (with ~15K+ spans).

Screenshot 2023-03-10 at 12 14 25 AM

Error message:

Coordinator.js:115 Viz worker onerror 
Cannot enlarge memory arrays. Either (1) compile with  -s TOTAL_MEMORY=X  with X higher than the current value 16777216, (2) compile with  -s ALLOW_MEMORY_GROWTH=1  which allows increasing the size at runtime but prevents some optimizations, (3) set Module.TOTAL_MEMORY to a higher value before the program runs, or (4) if you want malloc to return NULL (0) instead of this abort, compile with  -s ABORTING_MALLOC=0  

I think the reason is the allocated memory for viz is not enough.
So when I provided a larger number for memory like below in code here, then the trace graph can show up successfully.

// default 16MB
// 32MB: 33554432
this.layoutManager = new LayoutManager({ totalMemory: 33554432, useDotEdges: true, splines: 'polyline' });

Proposal

I can fix by provide a large totalMemory when initializing LayoutManager for trace graph, but I don't think hardcode the value in the code (TraceGraph.tsx) is a good way, and default value is good enough for most use cases.

So I would like to have totalMemory exposed in the config, so that anyone has the flexibility to tune the totalMemory themselves if needed.

Like:

export type Config = {
  ....
  traceGraph?: {
     layoutManager?: {
        totalMemory?: number; 
     };
  };
  ...
};

If this proposal is ok with you, I can implement it and submit a PR.

Open questions

No response

@yurishkuro
Copy link
Member

+1

yurishkuro pushed a commit that referenced this issue Mar 14, 2023
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 #123"
-->

## Which problem is this PR solving?
- Resolves #1249

## 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
`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>
Binrix pushed a commit to Binrix/jaeger-ui that referenced this issue Apr 18, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants