metadata: Add runtime labels for NodeJS #2233

kakkoyun · 2023-11-14T19:37:03Z

Signed-off-by: Kemal Akkoyun [email protected]

The version information set in here https://github.com/nodejs/node/blob/36ecf8ca656063060421e5b5440ac056f3a51e20/src/node_version.h so they end up in .data or .rodata sections.

Tested against multiple versions https://pprof.me/75e08cdd91547a303b82065b9281bf1e

Signed-off-by: Kemal Akkoyun <[email protected]>

pkg/metadata/runtime.go

javierhonduco · 2023-11-15T11:41:53Z

pkg/metadata/runtime.go

+			cache.Add(pid, nil)
+			return nil, nil
+		}
+		cache.Add(pid, rt)


rt can be an error, caching errors, is this intended?

rt can be nil but not an error. Why do you think so?

You are right, should have been nil, not an error! Just wanted to make sure that caching the "unhappy path" was something we wanted to do

pkg/runtime/nodejs/nodejs.go

javierhonduco · 2023-11-15T11:53:55Z

pkg/runtime/nodejs/nodejs.go

+
+const semVerRegex string = `v([0-9]+)(\.[0-9]+)(\.[0-9]+)` +
+	`(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?` +
+	`(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?`


nit: Perhaps removing everything after -, +, and trimming a leading v and spaces could be easier to understand and evolve? This is a major nit though, I don't mind the approach and you added awesome tests already!

We can be more precise about matching, but we can't be loose. Removing v gives false matches (I've tested, and we get things like `127.0.0.1). We must be strict since we search the whole space of constant strings. I wish there were a more straightforward way. We could evolve the approach when we know more.

javierhonduco · 2023-11-15T11:55:35Z

pkg/runtime/runtime.go

+type Type string
+
+type Runtime struct {
+	Type    Type


nit: Maybe "name" is clearer

I'm also planning to merge or embed this to the Interpreter type. I'll iterate on it in subsequent PRs.

javierhonduco · 2023-11-15T11:56:08Z

pkg/runtime/nodejs/nodejs.go

+	var lastError error
+	for _, sec := range ef.Sections {
+		if sec.Name == ".data" || sec.Name == ".rodata" {
+			data, err := sec.Data()


perf nit: This might allocate a new buffer, I don't expect it to be very large, but might be worth checking some node js executables

You are spot on I have already an optimization lined up to optimize this part. Stay tuned ;)

@javierhonduco #2254

javierhonduco

LGTM!! Mostly left some nits and questions to understand things a bit better.

Another thing I wanted to ask is where can I see the values retrieved in the pprof profile, I couldn't spot anything 😢 .

One more thing I noticed is that the provided file, https://github.com/nodejs/node/blob/36ecf8ca656063060421e5b5440ac056f3a51e20/src/node_version.h will only run in the preprocessor, so the version inclusion in the binary must happen elsewhere. Could be good to know exactly where it's embedded in case this shows us some opportunity to do things differently.

javierhonduco · 2023-11-15T12:08:46Z

On this last point:

$ /proc/536278/exe --version
v18.15.0

$ objdump -s -j .rodata /proc/536278/exe | grep "v18.15.0"
 223cd30 6173652f 7631382e 31352e30 2f6e6f64  ase/v18.15.0/nod
 223cd40 652d7631 382e3135 2e302e74 61722e67  e-v18.15.0.tar.g
 2d29d90 73652f76 31382e31 352e302f 6e6f6465  se/v18.15.0/node
 2d29da0 2d763138 2e31352e 302d6865 61646572  -v18.15.0-header
[javierhonduco@fedora parca-agent]$

kakkoyun · 2023-11-15T12:09:22Z

Thanks for the review ❤️

Another thing I wanted to ask is where can I see the values retrieved in the pprof profile, I couldn't spot anything 😢 .

This is my bad. For whatever reason, I thought we had the labels in there. I'm gonna add a screenshot of the labels.

kakkoyun · 2023-11-15T12:11:42Z

Thanks for the review ❤️

Another thing I wanted to ask is where can I see the values retrieved in the pprof profile, I couldn't spot anything 😢 .

This is my bad. For whatever reason, I thought we had the labels in there. I'm gonna add a screenshot of the labels.

Shots are the descriptions.

kakkoyun · 2023-11-15T12:13:17Z

On this last point:

$ /proc/536278/exe --version
v18.15.0

$ objdump -s -j .rodata /proc/536278/exe | grep "v18.15.0"
 223cd30 6173652f 7631382e 31352e30 2f6e6f64  ase/v18.15.0/nod
 223cd40 652d7631 382e3135 2e302e74 61722e67  e-v18.15.0.tar.g
 2d29d90 73652f76 31382e31 352e302f 6e6f6465  se/v18.15.0/node
 2d29da0 2d763138 2e31352e 302d6865 61646572  -v18.15.0-header
[javierhonduco@fedora parca-agent]$

This is how I discovered them 😊

I don't know the exact mechanism of how compilers decide to embed these constants.
This is how it's used https://github.com/nodejs/node/blob/36ecf8ca656063060421e5b5440ac056f3a51e20/src/node.cc#L1005-L1011

javierhonduco · 2023-11-15T12:19:04Z

Just if you are curious, seems to be defined here and set here. So it should be stored in the .rodata section of the node_metadata.cc compilation unit

kakkoyun · 2023-11-15T12:22:06Z

Just if you are curious, seems to be defined here and set here. So it should be stored in the .rodata section of the node_metadata.cc compilation unit

Amazing 🤩 Thanks for digging it out

Add runtime metadata labels for NodeJS

0786010

Signed-off-by: Kemal Akkoyun <[email protected]>

kakkoyun requested a review from a team as a code owner November 14, 2023 19:37

kakkoyun changed the title ~~Add runtime metadata labels for NodeJS~~ metadata: Add runtime labels for NodeJS Nov 14, 2023

Merge branch 'main' into add_nodejs_runtime_metadata

6e42566

javierhonduco self-requested a review November 15, 2023 09:50

javierhonduco reviewed Nov 15, 2023

View reviewed changes

pkg/metadata/runtime.go Show resolved Hide resolved

javierhonduco reviewed Nov 15, 2023

View reviewed changes

pkg/runtime/nodejs/nodejs.go Show resolved Hide resolved

javierhonduco reviewed Nov 15, 2023

View reviewed changes

pkg/runtime/nodejs/nodejs.go Show resolved Hide resolved

javierhonduco reviewed Nov 15, 2023

View reviewed changes

javierhonduco approved these changes Nov 15, 2023

View reviewed changes

kakkoyun merged commit bafac2b into main Nov 15, 2023
22 checks passed

kakkoyun deleted the add_nodejs_runtime_metadata branch November 15, 2023 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

metadata: Add runtime labels for NodeJS #2233

metadata: Add runtime labels for NodeJS #2233

kakkoyun commented Nov 14, 2023 •

edited

Loading

javierhonduco Nov 15, 2023

kakkoyun Nov 15, 2023

javierhonduco Nov 15, 2023 •

edited

Loading

javierhonduco Nov 15, 2023 •

edited

Loading

kakkoyun Nov 15, 2023

javierhonduco Nov 15, 2023

kakkoyun Nov 15, 2023

javierhonduco Nov 15, 2023

kakkoyun Nov 15, 2023

kakkoyun Nov 16, 2023

javierhonduco left a comment

javierhonduco commented Nov 15, 2023

kakkoyun commented Nov 15, 2023

kakkoyun commented Nov 15, 2023

kakkoyun commented Nov 15, 2023

javierhonduco commented Nov 15, 2023 •

edited by kakkoyun

Loading

kakkoyun commented Nov 15, 2023

metadata: Add runtime labels for NodeJS #2233

metadata: Add runtime labels for NodeJS #2233

Conversation

kakkoyun commented Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javierhonduco Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

javierhonduco Nov 15, 2023 • edited Loading

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

javierhonduco left a comment

Choose a reason for hiding this comment

javierhonduco commented Nov 15, 2023

kakkoyun commented Nov 15, 2023

kakkoyun commented Nov 15, 2023

kakkoyun commented Nov 15, 2023

javierhonduco commented Nov 15, 2023 • edited by kakkoyun Loading

kakkoyun commented Nov 15, 2023

kakkoyun commented Nov 14, 2023 •

edited

Loading

javierhonduco Nov 15, 2023 •

edited

Loading

javierhonduco Nov 15, 2023 •

edited

Loading

javierhonduco commented Nov 15, 2023 •

edited by kakkoyun

Loading