Skip to content

Prevent trace mutation during critical path computation#3271

Merged
yurishkuro merged 5 commits intomainfrom
copilot/add-unit-tests-for-critical-path
Dec 31, 2025
Merged

Prevent trace mutation during critical path computation#3271
yurishkuro merged 5 commits intomainfrom
copilot/add-unit-tests-for-critical-path

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Dec 31, 2025

The critical path algorithm was mutating the original trace's spans by modifying childSpanIds arrays and references during computation, breaking immutability guarantees.

Changes

New types for isolated computation:

  • Added CPSpan type containing only fields needed for critical path computation (spanID, startTime, duration, references, childSpanIds)
  • Added CPSpanReference type for span references within CPSpan objects

Utility functions for CPSpan creation:

  • Created utils/cpspan.tsx module with createCPSpan() and createCPSpanMap() helper functions
  • Centralized CPSpan object creation to ensure consistent defensive copying across the codebase

Updated critical path functions:

  • criticalPathForTrace() now uses createCPSpanMap() to create defensive copies before processing
  • Updated utility functions (getChildOfSpans, sanitizeOverFlowingChildren, findLastFinishingChildSpan, computeCriticalPath) to operate on CPSpan instead of Span
  • getChildOfSpans now checks childSpanIds.length > 0 instead of using a separate hasChildren field

Example of the fix:

// Before: Modified original trace spans
const spanMap = trace.spans.reduce((map, span) => {
  map.set(span.spanID, span);  // Direct reference
  return map;
}, new Map<string, Span>());

// After: Creates isolated copies using utility function
const spanMap = createCPSpanMap(trace.spans);

Test coverage:

  • Added unit tests verifying original trace remains unmodified after critical path computation
  • Updated existing tests to use centralized createCPSpan() utility
  • Enhanced test descriptions to clearly indicate what each test case validates (e.g., "child starts after parent ends - child dropped")
Original prompt

The funciton criticalPathForTrace() in packages/jaeger-ui/src/components/TracePage/CriticalPath/index.tsx, and specifically the util functions it calls, is modifying the underlying trace structure. Even though it creates a new spanMap, the util functions are manipulating the inside parts of the spans, like references[]

  1. I want to add unit tests to ensure that criticalPathForTrace() does not modify the trace passed to it.
  2. I want to create a helper type CPSpan (for "critical path span") which has childSpanIds array and remove that array from SpanData in packages/jaeger-ui/src/types/trace.tsx
  3. Let criticalPathForTrace() create Map<string, CPSpan> and pass it to util functions, such that the original trace.spans are not modified

Before submitting PR:

  • Run npm run prettier to format the changed code.
  • Make sure npm test and npm run lint are successful

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI self-assigned this Dec 31, 2025
Copilot AI and others added 3 commits December 31, 2025 21:09
Co-authored-by: yurishkuro <3523016+yurishkuro@users.noreply.github.com>
…ation

Co-authored-by: yurishkuro <3523016+yurishkuro@users.noreply.github.com>
Co-authored-by: yurishkuro <3523016+yurishkuro@users.noreply.github.com>
Copilot AI changed the title [WIP] Add unit tests for criticalPathForTrace function Prevent trace mutation during critical path computation Dec 31, 2025
Copilot AI requested a review from yurishkuro December 31, 2025 21:29
duration: number;
references: Array<SpanReference>;
childSpanIds: Array<string>;
hasChildren: boolean;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think hasChildren can be dropped, code can check if(span.childSpanIds) for the same outcome, less data to maintain

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in commit 1d5832f. Removed hasChildren from CPSpan type and updated code to check childSpanIds.length > 0 instead.

import sanitizeOverFlowingChildren from './sanitizeOverFlowingChildren';

// Helper function to create CPSpan objects from Span objects
function createCPSpan(span) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this function seems to be needed in many places, extract it into utility module and use every time we're creating CPSpan

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in commit 1d5832f. Extracted createCPSpan() and createCPSpanMap() functions into a new utility module at utils/cpspan.tsx and updated all test files to use it.

}),
],
])('sanitizeOverFlowingChildren', (testProps, expectedSanitizedData) => {
it('Should sanitize the data(overflowing spans) correctly', () => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add a description string to each test case (illustrating what is specific about it compared to other tests) and include that description in the test name in it()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in commit 1d5832f. Added descriptive strings to each test case (e.g., "child starts after parent ends - child dropped") that appear in both the test suite name and individual test names.

…, add test descriptions

Co-authored-by: yurishkuro <3523016+yurishkuro@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Dec 31, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • iojs.org
    • Triggering command: /usr/bin/curl curl -q --fail --compressed -L -s REDACTED -o - (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.45%. Comparing base (1634eef) to head (1d5832f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3271   +/-   ##
=======================================
  Coverage   97.45%   97.45%           
=======================================
  Files         268      269    +1     
  Lines        8364     8369    +5     
  Branches     2211     2212    +1     
=======================================
+ Hits         8151     8156    +5     
  Misses        210      210           
  Partials        3        3           

☔ 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.

@yurishkuro yurishkuro added the changelog:refactoring Internal, non-functional code improvements label Dec 31, 2025
@yurishkuro yurishkuro marked this pull request as ready for review December 31, 2025 22:21
@yurishkuro yurishkuro requested a review from a team as a code owner December 31, 2025 22:21
@yurishkuro yurishkuro requested review from pavolloffay and removed request for a team December 31, 2025 22:21
@yurishkuro yurishkuro merged commit 0e5f17f into main Dec 31, 2025
11 of 12 checks passed
@yurishkuro yurishkuro deleted the copilot/add-unit-tests-for-critical-path branch December 31, 2025 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog:refactoring Internal, non-functional code improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants