-
Notifications
You must be signed in to change notification settings - Fork 150
Node.js manual span capture #2661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
1c75f27
7345657
d31962b
e9c8c65
65a9bc2
19f2ccf
f8f84eb
8eaf928
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,8 @@ | |
| #include <bpfcore/bpf_helpers.h> | ||
| #include <bpfcore/bpf_tracing.h> | ||
|
|
||
| #include <common/common.h> | ||
| #include <common/ringbuf.h> | ||
| #include <common/strings.h> | ||
| #include <common/tracing.h> | ||
|
|
||
|
|
@@ -15,14 +17,19 @@ | |
| #include <maps/fd_to_connection.h> | ||
| #include <maps/nodejs_fd_map.h> | ||
|
|
||
| #include <pid/pid.h> | ||
|
|
||
| #include <shared/obi_ctx.h> | ||
|
|
||
| enum { | ||
| k_delim_offset = 13, | ||
| k_variant_offset = 14, | ||
| k_fd1_offset = 14, | ||
| k_fd2_offset = 18, | ||
| k_ctx_fd_offset = 18, | ||
| k_max_fd_digits = 4 | ||
| k_max_fd_digits = 4, | ||
| // strlen("/dev/null/obi-span/") — the JSON span payload starts here | ||
| k_span_payload_offset = 19, | ||
| }; | ||
|
|
||
| static __always_inline int handle_async_switch(char *buf, const u64 pid_tgid) { | ||
|
|
@@ -51,6 +58,54 @@ static __always_inline int handle_async_switch(char *buf, const u64 pid_tgid) { | |
| return 0; | ||
| } | ||
|
|
||
| // Manual span emitted by the injected span bridge (spanbridge.js): | ||
| // /dev/null/obi-span/<json> | ||
| // The JSON document (name, ids, duration, attributes...) is copied verbatim | ||
| // into a node_span_event_t; user space parses it (ReadNodeSpanEventIntoSpan). | ||
| // We stamp the event with bpf_ktime_get_ns() (the sentinel fires inside | ||
| // span.end(), so this is the span end time in the same monotonic domain the | ||
| // rest of the pipeline uses) and with the current request trace context from | ||
| // traces_ctx_v1 — maintained by the async-context sentinels above — so the | ||
| // span can be parented under OBI's automatic server span. | ||
| static __always_inline int handle_node_span(const char *path, const u64 pid_tgid) { | ||
| node_span_event_t *ev = bpf_ringbuf_reserve(&events, sizeof(node_span_event_t), 0); | ||
| if (!ev) { | ||
| return 0; | ||
| } | ||
|
|
||
| ev->type = EVENT_NODE_SPAN; | ||
| ev->_pad[0] = 0; | ||
| ev->_pad[1] = 0; | ||
| ev->end_ktime = bpf_ktime_get_ns(); | ||
| task_pid(&ev->pid); | ||
|
|
||
| const obi_ctx_info_t *octx = obi_ctx__get(pid_tgid); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can we have the no-store async-hook path send an explicit context-clear signal and delete this map entry before node-span events rely on it? A regression test with a background span overlapping a slow request would cover this. |
||
| if (octx) { | ||
| ev->has_parent_ctx = 1; | ||
| bpf_memcpy(ev->parent_trace_id, (void *)octx->trace_id, TRACE_ID_SIZE_BYTES); | ||
| bpf_memcpy(ev->parent_span_id, (void *)octx->span_id, SPAN_ID_SIZE_BYTES); | ||
| } else { | ||
| ev->has_parent_ctx = 0; | ||
| bpf_memset(ev->parent_trace_id, 0, TRACE_ID_SIZE_BYTES); | ||
| bpf_memset(ev->parent_span_id, 0, SPAN_ID_SIZE_BYTES); | ||
| } | ||
|
|
||
| const long len = bpf_probe_read_user_str( | ||
| ev->payload, NODE_SPAN_PAYLOAD_MAX_LEN, path + k_span_payload_offset); | ||
| if (len <= 1) { // empty or unreadable payload | ||
| bpf_ringbuf_discard(ev, 0); | ||
| return 0; | ||
| } | ||
|
|
||
| ev->payload_len = (u32)(len - 1); // exclude the NUL terminator | ||
|
|
||
| bpf_dbg_printk("nodejs_manual_span: len=%d", ev->payload_len); | ||
|
|
||
| bpf_ringbuf_submit(ev, get_flags()); | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| static __always_inline int handle_fd_correlation(char *buf, const u64 pid_tgid) { | ||
| u32 fd1 = 0; | ||
| u32 fd2 = 0; | ||
|
|
@@ -77,24 +132,30 @@ int BPF_KPROBE(obi_uv_fs_access, void *loop, void *req, const char *path) { | |
| (void)loop; | ||
| (void)req; | ||
|
|
||
| // the obi nodejs agent (fdextractor.js) passes signals to the ebpf layer | ||
| // by invoking uv_fs_access() with a fake path. Two formats are used: | ||
| // the obi nodejs agents (fdextractor.js, spanbridge.js) pass signals to | ||
| // the ebpf layer by invoking uv_fs_access() with a fake path. Three | ||
| // formats are used: | ||
| // | ||
| // 1. fd pair correlation (outgoing -> incoming): | ||
| // /dev/null/obi/<fd1><fd2> — each fd is a left-zero-padded 4-digit number | ||
| // | ||
| // 2. async context switch (before-hook fires before each JS callback): | ||
| // /dev/null/obi-ctx/<fd> — 4-digit incoming fd for the current async context | ||
| // | ||
| // Both paths share the prefix "/dev/null/obi" (13 chars). The character at | ||
| // position 13 distinguishes the two formats: | ||
| // '/' -> format 1 (fd pair) | ||
| // '-' -> format 2 (context switch, "-ctx/" follows) | ||
| // 3. manual span end (spanbridge.js): | ||
| // /dev/null/obi-span/<json> — serialized manual span, variable length | ||
| // | ||
| // All paths share the prefix "/dev/null/obi" (13 chars). The characters at | ||
| // positions 13-14 distinguish the formats: | ||
| // '/' -> format 1 (fd pair) | ||
| // '-', 'c' -> format 2 (context switch, "-ctx/" follows) | ||
| // '-', 's' -> format 3 (manual span, "-span/" follows) | ||
| static const char prefix[] = "/dev/null/obi"; | ||
| static const u8 prefix_size = sizeof(prefix) - 1; | ||
|
|
||
| // Buffer sized to hold the longest path + null terminator. | ||
| // Both formats are exactly 22 characters long. | ||
| // Buffer sized to hold the longest fixed-size path + null terminator. | ||
| // Formats 1 and 2 are exactly 22 characters long; format 3 is read | ||
| // directly from user memory in handle_node_span. | ||
| char buf[] = "/dev/null/obi/00000000"; | ||
|
|
||
| if (bpf_probe_read_user(buf, sizeof(buf), path) != 0) { | ||
|
|
@@ -107,10 +168,17 @@ int BPF_KPROBE(obi_uv_fs_access, void *loop, void *req, const char *path) { | |
|
|
||
| const u64 pid_tgid = bpf_get_current_pid_tgid(); | ||
|
|
||
| // Async context switch: /dev/null/obi-ctx/XXXX | ||
| // Fires from the async_hooks 'before' callback in fdextractor.js to refresh | ||
| // traces_ctx_v1 before each JS callback. | ||
| if (buf[k_delim_offset] == '-') { | ||
| // Manual span: /dev/null/obi-span/<json> | ||
| if (buf[k_variant_offset] == 's') { | ||
| if (!valid_pid(pid_tgid)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually looking at this code I wonder why we don't actually check for valid_pid right at the start of the function, like elsewhere. I suppose we wouldn't be injecting the agent into a process we are not instrumenting, but it's a maybe similar security concern as we had for Java. Someone can add something to a node app and mimic what our agent does and we'd happily read the data. I think so far it didn't matter it's only adding internal thread correlation, so it's good you added the check, but I'd say let's pull it up at the front.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes you are right, moving it up front |
||
| return 0; | ||
| } | ||
| return handle_node_span(path, pid_tgid); | ||
| } | ||
| // Async context switch: /dev/null/obi-ctx/XXXX | ||
| // Fires from the async_hooks 'before' callback in fdextractor.js to | ||
| // refresh traces_ctx_v1 before each JS callback. | ||
| return handle_async_switch(buf, pid_tgid); | ||
| } | ||
| // fd pair correlation: /dev/null/obi/<fd1><fd2> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason we are setting these _pad bytes to 0? I don't think anything uses them.