Annotating CPU profiler samples with embedder values

[Attila Szegedi]

Hi folks,

I recently submitted both a bug[0] and a code patch[1] for annotating CPU profiler samples with embedder values; I wanted to raise awareness of it here for discussion as well.

The use cases for these can be manifold; in practice, at Datadog we're using it to associate tracing span IDs, HTTP endpoint data, and other contextual information our customers want to group and slice samples in their Node.js profiles by. We've been providing this functionality to our customers for the past 3 years through a workaround – we use sample timestamps for correlation with our values. We install our own PROF signal handler function, store the pointer to V8's own signal handler, and then delegate to V8's handler while recording the current context value and time before and after the invocation; then we can associate each sample with a value that's matched with a signal handler invocation time that sample's timestamp falls into.

Our approach works, but it's not ideal – for one thing, it doesn't work on Windows which doesn't use UNIX signals… And frankly, we'd rather _not_ be hijacking the PROF signal and dealing with timestamps as surrogate correlation identifiers if we don't have to.

I opted for a very minimal implementation that adds a single void* to samples in keeping with the convention for other embedder data, External value, etc. It's opaque to V8, sourced by a callback installed into the profiler, and readable through a new void* CPUProfile::GetSampleContext() API. It keeps in spirit of additions of EmbedderStateTag and trace_id_.

One might ask why don't we "just" somehow retrofit trace_id, which is an uint64_t, so the same size (on most platforms) and opaqueness as a void*. The thing with trace_id is that it has particular semantics AFAICT around trace events, and is also serialized to JSON; we need something more generic and don't want to have rendering of what looks like random values in JSON. E.g. in our case, that void* carries data extracted through few dependent reads starting from Isolate's ContinuationPreservedEmbedderData.

Anyhow, I'm curious if other folks on this list find the feature piques their interest. I'm also looking forward to reviews and naturally I'm open to further discussion.

Attila.

(Software Engineer @ Datadog, working on the Node.js profiler.)

---

[0] https://issues.chromium.org/issues/512150509
[1] https://chromium-review.googlesource.com/c/v8/v8/+/7837639

[Attila Szegedi]

Hey y'all,

It's been about a month and a half and I got no signal on this outside of one maintainer I tried assigning to the review removing themselves from it (sad trombone.) I'm sure the V8 team must be inundated with requests, many of them AI generated, and I understand that it's hard to filter out what's worth focusing on.

I'm however an actual human (feels weird to have to spell this out) working for a fairly well-known observability company, trying to make the V8 CPU profiling story a bit better in the long run. I feel like the change is also very small and fairly well isolated. I would be extremely grateful if someone were willing to work with me on getting this accepted and landed.

Thank you!

  Attila.

(Software Engineer @ Datadog, working on the Node.js profiler.)

[Leszek Swirski]

Hi Attila,

Sorry for the lack of response here, I think it's a case of bystander syndrome where no one is quite sure what to reply, but let me try.

Generally, our team has very little mandate to support embedders outside of Chromium. We continue to support them as a gesture of goodwill to the OSS community, using spare or unfunded time, but it means that contributions with any question marks tend to fall prey to this bystander syndrome effect, particularly if they're in an area that no one is actively working on like the profiler. I'm afraid "sad trombone" is an accurate reaction to this.

Specifically for your change, it generally looks fine, and as you say, it is small and well isolated. The reason this wasn't enough for a quick'n'easy stamp is that we tend to be quite conservative with API changes, because they're hard to reverse. This is particularly true for API changes that introduce user hooks into very internal behaviours like stack sampling. Also, from an API design point of view, you can get long-term API inconsistencies via a long sequence of small, well isolated changes, and sometimes we want to take a step back and ask ourselves if we don't want to perhaps change the API and deprecate the old one. All this means that this sort of contribution needs someone to think very hard about whether the small change won't have large effects, and no one has had the spare time to prioritise this hard thinking.

Of course, in replying to your email, I also glanced over your change, so I can leave some concrete comments there, I just wanted to explain why you might get such a silent reaction to a well-structured contribution.

Cheers,

Leszek

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion visit https://groups.google.com/d/msgid/v8-dev/CADatEgZzCPG5LCDXnBnBLgp%2B8Rf%3D_Eu0Px1Zj4B-HzLn1DXRpw%40mail.gmail.com.

[Attila Szegedi]

Hi Leszek,

First of all, thank you for a considered response explaining the forces at work; I appreciate it a lot. I do understand that Chromium priorities come first and everything else is a factor of goodwill. I'm a bit sad to hear that nobody is actively working on the profiler though, as I'm fairly invested in it :-)

I concede your point of small API changes causing mounting inconsistencies over time is valid; I did consider this as much as I was able to, but I agree that having someone more knowledgable in that area of V8 code give it a thought would be beneficial. FWIW, I'm personally finding some weak association parallels between this and – maybe surprisingly – being able to set an ArrayBufferAllocator in Isolate::CreateParams, as that's also an embedder extension point where it can supply custom behavior.

Anyhow, as I said, I truly appreciate the response and will reply to your review comments as well.

Have a great day!

  Attila.

–

(Disclaimer: I use en- and em-dashes in my messages; they've been part of my writing style for decades and I'm not changing it. It's LLMs that should stop using them, not me.)

To view this discussion visit https://groups.google.com/d/msgid/v8-dev/CAGxd1t95YetqT-Ap-09ZWT9XH1aEg9ynPXR0iH7xDnq0a%2Bgkuw%40mail.gmail.com.