Skip to content

Conversation

@DamianMaslanka5
Copy link
Contributor

@DamianMaslanka5 DamianMaslanka5 commented Apr 17, 2025

Which problem is this PR solving?

closes #2179

Description of the changes

Use Set and Map to make lookups faster.

This changes makes collapsing and expanding spans with large (10k) amount of children 200-500ms faster.

Before

image

After

image

How to test

  1. Generate data - go run cmd/tracegen/main.go -service abcd -traces 1 -spans 10000
  2. Collapse root span

firefox_4ekX2LDxUX

@DamianMaslanka5 DamianMaslanka5 requested a review from a team as a code owner April 17, 2025 19:52
@DamianMaslanka5 DamianMaslanka5 requested review from joe-elliott and removed request for a team April 17, 2025 19:52
Use Set and Map to make lookups faster.

This changes makes collapsing and expanding spans with
large (10k) amount of children 200-500ms faster.

Signed-off-by: Damian Maslanka <[email protected]>
@codecov
Copy link

codecov bot commented Apr 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.78%. Comparing base (72a976f) to head (8fe4cef).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2722   +/-   ##
=======================================
  Coverage   96.78%   96.78%           
=======================================
  Files         256      256           
  Lines        7933     7934    +1     
  Branches     2042     1990   -52     
=======================================
+ Hits         7678     7679    +1     
- Misses        254      255    +1     
+ Partials        1        0    -1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@DamianMaslanka5 DamianMaslanka5 force-pushed the faster-collapse-expand branch from 49dfc62 to 6a0df1f Compare April 17, 2025 19:54
});
}

const memoizedSpanByID = memoizeOne((spans: Span[]) => new Map(spans.map(x => [x.spanID, x])));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but I am not sure if there is a good place for this data.

Creating a Map or a Set should not have measurable impact on performance, because it is usually done once per trace.
Also, if any refactoring is needed, then it would probably be best to do this in another PR.

@yurishkuro yurishkuro added the changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements label Apr 19, 2025
@yurishkuro yurishkuro added this pull request to the merge queue Apr 19, 2025
@yurishkuro
Copy link
Member

@DamianMaslanka5 how is this PR different from #1975?

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 19, 2025
@DamianMaslanka5
Copy link
Contributor Author

@DamianMaslanka5 how is this PR different from #1975?

@yurishkuro, #1975 tries to optimize computeCriticalPath, but computeCriticalPath is executed only once (this function is memoized), when the trace details page is loaded

So #1975 will not improve performance issue described in #2179

@yurishkuro
Copy link
Member

Is that PR still needed then? In your experiments with large traces how much initial calculation takes?

@yurishkuro yurishkuro added this pull request to the merge queue Apr 19, 2025
Merged via the queue into jaegertracing:main with commit 9938e2d Apr 19, 2025
9 checks passed
@DamianMaslanka5
Copy link
Contributor Author

Is that PR still needed then? In your experiments with large traces how much initial calculation takes?

@yurishkuro, yes #1975 is still needed, without changes from #1975, computeCriticalPath takes 200ms for 10k spans, with changes from #1975 computeCriticalPath is not visible in the profiler, also #2720 is no longer an issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Critical path is taxing on CPU when expanding span tags

2 participants