Skip to content

[Node.js] disable source maps in node heap#107

Merged
IlyasShabi merged 7 commits intomainfrom
ishabi/node-profile-heap
Apr 2, 2026
Merged

[Node.js] disable source maps in node heap#107
IlyasShabi merged 7 commits intomainfrom
ishabi/node-profile-heap

Conversation

@IlyasShabi
Copy link
Copy Markdown
Contributor

@IlyasShabi IlyasShabi commented Mar 31, 2026

The node_heap test started failing after dd-trace-js changed profiler startup to happen earlier, during tracer initialization.

References:

  1. The failing runs show this error: TypeError: mapper.loadDirectory is not a function and the issue turned out to be a version mismatch. The package dd-trace is installed from master branch, but @datadog/pprof was overridden to "dev" which is an npm dist-tag, not a git branch, and it was resolving to an older pre-release build 6.0.0-pre-fdd7f20 rather than the current main branch.
    The pprof pipeline to publish dev was fixed use trusted publishing for dev releases pprof-nodejs#310 and use release publisher file to release dev tag too pprof-nodejs#311
  2. Update measurements

@IlyasShabi IlyasShabi force-pushed the ishabi/node-profile-heap branch from 35a893a to 097ac19 Compare March 31, 2026 15:51
@IlyasShabi IlyasShabi force-pushed the ishabi/node-profile-heap branch from 097ac19 to 1410323 Compare March 31, 2026 16:05
@IlyasShabi IlyasShabi marked this pull request as ready for review April 1, 2026 07:56
@IlyasShabi IlyasShabi requested a review from a team as a code owner April 1, 2026 07:56
@IlyasShabi IlyasShabi requested a review from r1viollet April 1, 2026 07:56
{
"regular_expression": "processTimers;listOnTimeout;work;b;slice",
"percent": 66,
"percent": 50,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why is this changing to 50 50 ?

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.

These are the observed values and reflect exactly what the code is doing. When back to 60 I had from CI

Assertion failed: stack 'processTimers;listOnTimeout;work;b;slice' (labels=[]) should have been 66% +/- 5% of the profile but was 50% with 16% error

Assertion failed: stack 'processTimers;listOnTimeout;work;a;slice' (labels=[]) should have been 33% +/- 5% of the profile but was 49% with 16% error

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I understand, but are these values expected ?
Is the scenario meant to have 1/3 2/3 split or a 50/50 split ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  function work() {
    count += 1
    a(allocSize, refs);
    b(allocSize * 2, refs);

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.

IMO the 50/50 is correct because the objects profile counts allocations, not bytes. In work() a and b are called once on every iteration.

The old 66/33 value for objects was not the expectation of the scenario. It was an observed bias from earlier runs https://github.com/DataDog/prof-correctness/blob/main/scenarios/node_heap/README.md?plain=1#L23

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ah OK, this is number of objects, apologies

@IlyasShabi IlyasShabi requested a review from r1viollet April 2, 2026 07:47
ENV DD_REMOTE_CONFIGURATION_ENABLED=0
ENV DD_PROFILING_PROFILERS=space
ENV DD_TRACE_DEBUG=1
ENV DD_PROFILING_DEBUG_SOURCE_MAPS=0
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

NIT: could requires a comment to explain that we can remove this at a specific release

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.

I just checked that the default value is false not needed anymore

Copy link
Copy Markdown
Collaborator

@r1viollet r1viollet left a comment

Choose a reason for hiding this comment

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

LGTM

@IlyasShabi IlyasShabi merged commit e4667cf into main Apr 2, 2026
9 checks passed
@IlyasShabi IlyasShabi deleted the ishabi/node-profile-heap branch April 2, 2026 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants