Update Node custom labels support to use hashmap-based labels.#91
Conversation
The previous approach, which involved resetting the current labelset every time a piece of javascript code ran, was too slow. So we switched to a new approach, where we _only_ run our own code when a new async context is created or destroyed, or when label sets change. Since we are no longer setting the current labelset, we instead store labelsets in a hash map from async ID -> labelset (this is implemented in the custom_labels library). Thus, at profiling time, we need to get the current async ID from the node.js environment, then look it up in the hashmap. This PR accomplishes the profiling side of that change.
|
We should get the CI green and add a Makefile target to run the new container tests (which are awesome)! And then have unit-test-on-pull-request hit that Makefile target. Maybe the test job could be extended to hit the container tests? |
|
@gnurizen my test already runs during the "Interpreter integration" test job |
Oh sweet I was looking at upstream otel fork and didn't see it! Forgot I added a generic integration test invocation to run the luajit tests. |
There was a problem hiding this comment.
Upstream will want to see this in interpreter/customlabels/integrationtests/, upstream the golabels tests now live in interpreter/golabels/integrationtests/.
There was a problem hiding this comment.
I moved the node test to there. I'm gonna leave this one here and address it in a follow-up PR because it already existed before this PR (and needs more changes in order to start working again)
| maxRetries int, waitRetry time.Duration) error | ||
| } | ||
|
|
||
| type MetricsReporter interface { |
There was a problem hiding this comment.
Do we know what happened to the metrics reporter? Might be worth kicking off a discussion on slack if it wasn't obvious what the plan was when this was removed.
There was a problem hiding this comment.
sure, I'll check with them what happened
|
|
||
| major := binary.LittleEndian.Uint32(versBuf[0:4]) | ||
| minor := binary.LittleEndian.Uint32(versBuf[4:8]) | ||
| patch := binary.LittleEndian.Uint32(versBuf[8:12]) |
There was a problem hiding this comment.
What's wrong with the version info Loader already looked up?
There was a problem hiding this comment.
That is the v8 version; this is the Node version.
The Environment class as well as the execution async ID are not v8 concepts, they are higher level Node.js concepts. So two versions of Node could have a different set of offsets even if they use the same underlying version of v8, and so we need the actual Node version here.
| abiVersionElfVA libpf.Address | ||
| currentSetTlsAddr libpf.Address | ||
|
|
||
| hasCurrentHm bool |
There was a problem hiding this comment.
why not just have hasCurrentHm == currentHmTlsAddr != 0?
There was a problem hiding this comment.
0 is a valid TLS symbol, it would just mean the variable happens to be the first thing in the TLS segment.
2 participants
The previous approach, which involved resetting the current labelset every time a piece of javascript code ran, was too slow. So we switched to a new approach, where we only run our own code when a new async context is created or destroyed, or when label sets change.
Since we are no longer setting the current labelset, we instead store labelsets in a hash map from async ID -> labelset (this is implemented in the custom_labels library). Thus, at profiling time, we need to get the current async ID from the node.js environment, then look it up in the hashmap. This PR accomplishes the profiling side of that change.