test/integration: Add NodeJS Integration tests #2512 #2598

joeyleeeeeee97 · 2024-03-06T05:04:22Z

for #2512

joeyleeeeeee97 · 2024-03-06T05:05:10Z

@kakkoyun Only "17": "17-alpine" image for now, please tell me what to add here

kakkoyun

Awesome work 🎉 Thanks a lot for taking care of this!

It's very close but it's not there yet. I have commented inline what we need probably.

test/integration/nodejs/nodejs_test.go

test/integration/nodejs/testdata/cpu_hog.js

Makefile

test/integration/nodejs/nodejs_test.go

kakkoyun · 2024-03-06T07:10:54Z

test/integration/nodejs/nodejs_test.go

+ FileMode: 0o700,
+ },
+ },
+ Cmd: []string{"node", "/cpu_hug.js"},


And we need special flags until we implement a dedicated unwinder.

This should help you https://www.polarsignals.com/docs/nodejs

The aggregatedStack I got now was [[JS:~ JS:~executeUserEntryPoint JS:~Module._load JS:~Module.load JS:~Module._extensions..js JS:~Module._compile JS:*]]. Not complete.

nodejs_test.go:170: The stack [LazyCompile:*fib] is not contained in any of [[JS:~ JS:~executeUserEntryPoint JS:~Module._load JS:~Module.load JS:~Module._extensions..js JS:~Module._compile JS:*]]

@kakkoyun should I assert on this?

Do you see this stack after you added those flags?

no. this is the output after I added --perf-basic-prof-only-functions --interpreted-frames-native-stack

Let me add this test in our github check so we can see this

@kakkoyun Please check in https://github.com/parca-dev/parca-agent/actions/runs/8201453202/job/22430337815?pr=2598

This is because node js lazyCompiling and make the loop anonymous, I modified the test and tried assert on runLoop instead

Hmm, I suspect nodejs is inlining things. Could we disable inlining?

OK. let me test this without inlining

kakkoyun · 2024-03-12T07:04:36Z

.github/workflows/test-integration-amd64.yml

+
+ - name: Integration Tests (Nodejs)
+ if: ${{ always() }}
+ continue-on-error: true


This shouldn't be the case for NodeJS. We did this for Java because it hasn't been implemented fully yet.

kakkoyun · 2024-03-12T07:06:08Z

test/integration/nodejs/nodejs_test.go

+ "JS:~Module._extensions..js",
+ "JS:~Module._compile",
+ "JS:~",
+ "JS:*runLoop",


Maybe NodeJS is inlining these functions. Could we somehow disable inlining for the tests? Or a specific function?

There is not a single option to disable inlining optimizations, I am using --no-opt to disable all optimizations.
Now I can see the expected whole stack.

@kakkoyun , please take another look

kakkoyun · 2024-03-12T07:08:34Z

@joeyleeeeee97 Thanks again for taking care of this and bearing with me. But we need to make sure that unwinding works as expected, so hang tight for a little longer.

We need to make sure we see the expected stack in the tests.

joeyleeeeeee97 · 2024-03-15T04:20:24Z

@kakkoyun You are right on this.

node --trace-opt cpu_hog.js
[marking 0x1c745a9658a1 <JSFunction cpu (sfi = 0x933e0017551)> for optimization to TURBOFAN, ConcurrencyMode::kConcurrent, reason: hot and stable]
[compiling method 0x1c745a9658a1 <JSFunction cpu (sfi = 0x933e0017551)> (target TURBOFAN), mode: ConcurrencyMode::kConcurrent]
[completed compiling 0x1c745a9658a1 <JSFunction cpu (sfi = 0x933e0017551)> (target TURBOFAN) - took 0.041, 0.375, 0.000 ms]
[completed optimizing 0x1c745a9658a1 <JSFunction cpu (sfi = 0x933e0017551)> (target TURBOFAN)]
[marking 0x1c745a9658e1 <JSFunction c1 (sfi = 0x933e00175a9)> for optimization to TURBOFAN, ConcurrencyMode::kConcurrent, reason: hot and stable]
[marking 0x1c745a965921 <JSFunction b1 (sfi = 0x933e0017601)> for optimization to TURBOFAN, ConcurrencyMode::kConcurrent, reason: hot and stable]
[marking 0x1c745a965961 <JSFunction a1 (sfi = 0x933e0017659)> for optimization to TURBOFAN, ConcurrencyMode::kConcurrent, reason: hot and stable]
[marking 0x1c745a9659a1 <JSFunction runLoop (sfi = 0x933e00176b1)> for optimization to TURBOFAN, ConcurrencyMode::kConcurrent, reason: hot and stable]
[compiling method 0x1c745a965961 <JSFunction a1 (sfi = 0x933e0017659)> (target TURBOFAN), mode: ConcurrencyMode::kConcurrent]
[compiling method 0x1c745a965921 <JSFunction b1 (sfi = 0x933e0017601)> (target TURBOFAN), mode: ConcurrencyMode::kConcurrent]
[compiling method 0x1c745a9658e1 <JSFunction c1 (sfi = 0x933e00175a9)> (target TURBOFAN), mode: ConcurrencyMode::kConcurrent]
[completed compiling 0x1c745a965921 <JSFunction b1 (sfi = 0x933e0017601)> (target TURBOFAN) - took 0.000, 0.250, 0.000 ms]
[completed optimizing 0x1c745a965921 <JSFunction b1 (sfi = 0x933e0017601)> (target TURBOFAN)]
[completed compiling 0x1c745a9658e1 <JSFunction c1 (sfi = 0x933e00175a9)> (target TURBOFAN) - took 0.000, 0.250, 0.000 ms]
[completed optimizing 0x1c745a9658e1 <JSFunction c1 (sfi = 0x933e00175a9)> (target TURBOFAN)]
[completed compiling 0x1c745a965961 <JSFunction a1 (sfi = 0x933e0017659)> (target TURBOFAN) - took 0.000, 0.334, 0.000 ms]
[completed optimizing 0x1c745a965961 <JSFunction a1 (sfi = 0x933e0017659)> (target TURBOFAN)]
[compiling method 0x1c745a9659a1 <JSFunction runLoop (sfi = 0x933e00176b1)> (target TURBOFAN) OSR, mode: ConcurrencyMode::kConcurrent]
[completed compiling 0x1c745a9659a1 <JSFunction runLoop (sfi = 0x933e00176b1)> (target TURBOFAN) OSR - took 0.000, 0.292, 0.041 ms]
[completed optimizing 0x1c745a9659a1 <JSFunction runLoop (sfi = 0x933e00176b1)> (target TURBOFAN) OSR]

Now I added --no-opt on this test.

Also

In the context of V8 engine, JS:*runLoop and JS:^runLoop both represent identifiers for JavaScript functions, but they have different meanings:

JS:*runLoop: This identifier typically denotes a JavaScript function that has been optimized. The * symbol indicates that the function has been optimized and may have been compiled to machine code. When you see an identifier like JS:*functionName, it means that the function has been optimized by the V8 engine.

JS:^runLoop: This identifier typically denotes a JavaScript function that has not been optimized. The ^ symbol indicates that the function has not been optimized. When you see an identifier like JS:^functionName, it means that the function has not been optimized by the V8 engine.

joeyleeeeeee97 · 2024-03-27T03:07:19Z

@brancz Could you help check this?

kakkoyun

LGTM. Happy to merge it when the CI is green.

kakkoyun · 2024-03-28T13:50:06Z

AMD64 tests are passing for 19, 20. 18 has a different stack trace. Maybe we should have a different assertion for that.

ARM64 failures are somewhat known, so I'd be happy to merge if we can fix the issue with 18 on amd64.

joeyleeeeeee97 requested a review from a team as a code owner March 6, 2024 05:04

kakkoyun reviewed Mar 6, 2024

View reviewed changes

test/integration/nodejs/nodejs_test.go Outdated Show resolved Hide resolved

test/integration/nodejs/testdata/cpu_hog.js Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

test/integration/nodejs/nodejs_test.go Outdated Show resolved Hide resolved

kakkoyun reviewed Mar 6, 2024

View reviewed changes

joeyleeeeeee97 force-pushed the nodejs_integration branch 7 times, most recently from 7521280 to a1077f4 Compare March 8, 2024 03:15

joeyleeeeeee97 requested a review from kakkoyun March 11, 2024 16:20

joeyleeeeeee97 force-pushed the nodejs_integration branch 4 times, most recently from e63f431 to 4b48da9 Compare March 11, 2024 20:11

kakkoyun reviewed Mar 12, 2024

View reviewed changes

test/integration: Add NodeJS Integration tests (parca-dev#2512)

0dacd6d

joeyleeeeeee97 force-pushed the nodejs_integration branch from 4b48da9 to 0dacd6d Compare March 15, 2024 04:20

joeyleeeeeee97 requested a review from kakkoyun March 18, 2024 16:15

kakkoyun approved these changes Mar 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test/integration: Add NodeJS Integration tests #2512 #2598

test/integration: Add NodeJS Integration tests #2512 #2598

joeyleeeeeee97 commented Mar 6, 2024

joeyleeeeeee97 commented Mar 6, 2024

kakkoyun left a comment

kakkoyun Mar 6, 2024

joeyleeeeeee97 Mar 7, 2024 •

edited

kakkoyun Mar 7, 2024

joeyleeeeeee97 Mar 8, 2024 •

edited

joeyleeeeeee97 Mar 8, 2024

joeyleeeeeee97 Mar 11, 2024 •

edited

kakkoyun Mar 12, 2024

joeyleeeeeee97 Mar 15, 2024

kakkoyun Mar 12, 2024

joeyleeeeeee97 Mar 15, 2024

kakkoyun Mar 12, 2024

joeyleeeeeee97 Mar 15, 2024

joeyleeeeeee97 Mar 15, 2024

joeyleeeeeee97 Mar 18, 2024

kakkoyun commented Mar 12, 2024

joeyleeeeeee97 commented Mar 15, 2024 •

edited

joeyleeeeeee97 commented Mar 27, 2024

kakkoyun left a comment

kakkoyun commented Mar 28, 2024

test/integration: Add NodeJS Integration tests #2512 #2598

Are you sure you want to change the base?

test/integration: Add NodeJS Integration tests #2512 #2598

Conversation

joeyleeeeeee97 commented Mar 6, 2024

joeyleeeeeee97 commented Mar 6, 2024

kakkoyun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyleeeeeee97 Mar 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyleeeeeee97 Mar 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyleeeeeee97 Mar 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kakkoyun commented Mar 12, 2024

joeyleeeeeee97 commented Mar 15, 2024 • edited

joeyleeeeeee97 commented Mar 27, 2024

kakkoyun left a comment

Choose a reason for hiding this comment

kakkoyun commented Mar 28, 2024

joeyleeeeeee97 Mar 7, 2024 •

edited

joeyleeeeeee97 Mar 8, 2024 •

edited

joeyleeeeeee97 Mar 11, 2024 •

edited

joeyleeeeeee97 commented Mar 15, 2024 •

edited