Node.js manual span capture#2661
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2661 +/- ##
==========================================
- Coverage 70.17% 69.64% -0.53%
==========================================
Files 353 354 +1
Lines 48848 49252 +404
==========================================
+ Hits 34277 34304 +27
- Misses 12442 12814 +372
- Partials 2129 2134 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
grcevski
left a comment
grcevski
left a comment
There was a problem hiding this comment.
What a great idea! This is really cool. I left bunch of comments, I think the really interesting issue is the registration of the SDK, I don't know what the OTel SDK does and if they do check not to overwrite the registry. If they overwrite than we are good, if not then we should try to figure out if we add more safety checks.
| } | ||
|
|
||
| ev->type = EVENT_NODE_SPAN; | ||
| ev->_pad[0] = 0; |
There was a problem hiding this comment.
Any reason we are setting these _pad bytes to 0? I don't think anything uses them.
| if (buf[k_delim_offset] == '-') { | ||
| // Manual span: /dev/null/obi-span/<json> | ||
| if (buf[k_variant_offset] == 's') { | ||
| if (!valid_pid(pid_tgid)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yes you are right, moving it up front
| mod, ok := modules[instrumentedIno] | ||
| if ok { | ||
| mod.probes = append(mod.probes, pMap) | ||
| if filtered := dedupModuleProbes(mod.probes, pMap); len(filtered) > 0 { |
There was a problem hiding this comment.
Interesting, nice catch. So same symbol exists in two different modules from the same process? What an edge case.
| // "libuv.so") resolve to the same file — typically through the | ||
| // instrument-the-executable fallback — and would otherwise attach the same | ||
| // program twice at the same offset, duplicating every event it emits. | ||
| func dedupModuleProbes( |
There was a problem hiding this comment.
Do you mind adding a unit test for this?
| const existing = g[API_KEY]; | ||
| // An SDK (or another agent) already registered a provider or context | ||
| // manager — stay inert rather than fight it. | ||
| if (existing && (existing.trace || existing.context)) { |
There was a problem hiding this comment.
Do you know if the OpenTelemetry SDK currently uses similar check for double loading? I wonder the loading of the SDK happens delayed, us setting registry.trace and registry.context prevents the app from actually using OTel SDK? I'm thinking of OTel instrumented services.
If this is a real situation, I think maybe we need to write some code in the nodejs.go that will inspect the depdendencies file and see the application requires the SDK, not the API. If it has the SDK maybe it's unsafe to do this injection?
There was a problem hiding this comment.
it does the exact same thing
if (!allowOverride && api[type]) {
// already registered an API of this type
const err = new Error(
`@opentelemetry/api: Attempted duplicate registration of API: ${type}`
);
diag.error(err.stack || err.message);
return false;
}the requires cache is stronger then the depdendencies file, depdendencies file says "is it declared as a dependency?", requires cache says "is the imported by the application?", but after looking deeper both seem not correct enough.
Applications can depend and import the otel sdk but end up not starting the instrumentations (lets say via OTEL_SDK_DISABLED=true or some other runtime condition), so this will provide a false negetive.
What ill implement instead is a step-aside yield, which will basicly give up our instrumentation in the case another instrumentation tries to register. I think this is the least intrusive solution
There was a problem hiding this comment.
Oh I like this approach. I think I know what you mean. Great idea!
| scope: this._scope, | ||
| }; | ||
| let payload = JSON.stringify(rec); | ||
| if (payload.length > MAX_PAYLOAD) { |
There was a problem hiding this comment.
I think Javascript strings can be UTF-16, therefore this lenght size lies about the true byte size here. Web search tells me the byte size can be found with this if (Buffer.byteLength(payload, 'utf8') > MAX_PAYLOAD). Can we use Buffer.byteLength or that needs a dependency?
Alternatively, maybe we handle partial json in the userspace code?
| ValLength uint16 | ||
| Vtype uint8 | ||
| Reserved uint8 | ||
| Key [32]uint8 |
There was a problem hiding this comment.
Are these supposed to be the ATTR_KEY and ATTR_VALUE from the spanbridge.js? If so, I think your sizes are double in the JS code.
const MAX_ATTR_KEY_LEN = 64;
const MAX_ATTR_VALUE_LEN = 256;
mmat11
left a comment
mmat11
left a comment
There was a problem hiding this comment.
lgtm! left one comment about CI testing
3ab8cdd to
e9c8c65
Compare
|
Markdown linter is failing, can be merged after it's been fixed |
MrAlias
left a comment
MrAlias
left a comment
There was a problem hiding this comment.
The approach is promising, but the SDK takeover and correlation regressions should block merging.
| apiObj[method] = wrapped; | ||
| }; | ||
|
|
||
| for (const key of Object.keys(require.cache ?? {})) { |
There was a problem hiding this comment.
Only API copies already present in require.cache have their setters wrapped. If OBI injects before an app lazily loads @opentelemetry/api and its SDK, that later copy calls an unwrapped setGlobalTracerProvider; it sees the bridge's global trace registration and rejects the app provider as a duplicate. The app's exporter never takes ownership, and spans continue through OBI.
The handoff needs to cover API copies loaded after injection. One viable direction is to extract this wiring into a helper and invoke it from a module-load hook before the SDK receives the API exports; because that hook is process-wide, it also needs to compose safely with existing loader patches. Please add an injection-before-first-API-load regression case on Node 18, 20, and 22.
There was a problem hiding this comment.
Thats a great edge case, will do!
| ev->end_ktime = bpf_ktime_get_ns(); | ||
| task_pid(&ev->pid); | ||
|
|
||
| const obi_ctx_info_t *octx = obi_ctx__get(pid_tgid); |
There was a problem hiding this comment.
obi_ctx__get is not guaranteed to describe the callback ending this span. fdextractor.js emits a -ctx/ sentinel only when its AsyncLocalStorage store has an incoming FD; callbacks created outside a request emit nothing. If a background timer fires while a request is still in flight, this lookup therefore returns the last request context and incorrectly parents the background span into that trace.
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.
MrAlias
left a comment
MrAlias
left a comment
There was a problem hiding this comment.
The CommonJS handoff fixes the prior late-load issue, but native ESM still lets the bridge block a customer's SDK from registering.
| // covers CommonJS require() only; an api pulled in purely through the native | ||
| // ESM loader is not intercepted here. | ||
| try { | ||
| const Module = require('module'); |
There was a problem hiding this comment.
This still only covers CommonJS. Native ESM imports never reach Module._load; if OBI injects before an ESM app imports @opentelemetry/api, that API copy never gets its setter wrapped. Its later setGlobalTracerProvider sees the bridge's registry.trace, rejects the app provider as a duplicate, and the app's exporter never takes over.
The handoff needs an ESM-safe path, or the bridge needs to remain inert where that path cannot be made safe. Please add a native-ESM injection-before-first-API-load regression on Node 18, 20, and 22.
| happens once per process; apps started after OBI are picked up by | ||
| discovery as usual. | ||
| - **Late-registering SDKs** are handled by the step-aside (see Guards): the | ||
| bridge yields and the app's SDK takes over. The residual gap is an |
There was a problem hiding this comment.
This no longer describes the implementation: Module._load now wires API copies loaded by CommonJS require(), while native ESM remains the unhandled case. Please document those two cases explicitly, and remove the later claim that no require hook or module wrapping is used.
4 participants
Part of #2660
Summary
Captures spans that Node.js applications create through
@opentelemetry/api(
tracer.startSpan/startActiveSpan/span.end) when the app has noOpenTelemetry SDK registered. Without an SDK those API calls return
non-recording no-ops and the telemetry is silently lost — this feature makes
them real and exports them through OBI's normal traces pipeline, correlated
with OBI's existing traces.
It's the Node.js analog of OBI's existing Go manual-span support
(
bpf/gotracer/go_sdk.c), which uprobes the no-op global-API tracer and backsoff when a real SDK is present. Since JIT-compiled JavaScript can't be uprobed
(anonymous executable mappings, no inode to attach to), the capture happens
in-process via a tiny injected JS bridge; only the transport back to OBI is
eBPF.
Opt-in:
nodejs.manual_spans: true(OTEL_EBPF_NODEJS_MANUAL_SPANS,default
false), on top of the existingnodejs.enabled.How it works
inspector injector, so it works on already-running processes (tracers
acquired before injection go live — the bridge sets the delegate on every
already-loaded
@opentelemetry/apicopy viarequire.cache, sincepre-acquired
ProxyTracers resolve through that, not the global registry).reuses the sentinel-syscall channel
fdextractor.jsalready uses — no addonto build/ship, no new eBPF attach machinery.
context at span-end, so bridge-root spans parent under OBI's automatic server
span (specifically its "processing" sub-span), and nested manual spans keep
their own parent chain — all on one trace. Orphan manual spans (no
surrounding request) export with a bridge-generated trace ID.
create the API registry) if a provider/context manager is already registered
or any
@opentelemetry/sdk-*is loaded; the app's own registration andexporters are untouched. The existing OTLP-export overlap detection applies
on top.
no stdout/stderr.
OTEL_EBPF_NODEJS_SPAN_BRIDGE_DEBUG=1opts into stderrdiagnostics (why it stayed inert, when it activated, unexpected errors) for
troubleshooting an injection.
Changes
pkg/internal/nodejs/spanbridge.js— the injected bridge: minimalTracerProvider/Tracer/Span + AsyncLocalStorage context manager, span→JSON
serialization with bounded fields, sentinel transport, opt-in diagnostics.
pkg/internal/nodejs/{nodejs.go,injector.go}— embed the bridge andevaluate it after
fdextractor.jswhen the flag is set. Fixes a pre-existinginjector bug: gorilla's default 4 KB websocket write buffer fragmented larger
scripts, which the Node inspector rejects (
close 1006) — enlarged thedialer buffers so each script is a single frame.
bpf/generictracer/nodejs.c— new-span/sentinel branch →node_span_event_ton the ringbuf.bpf/common/{common.h,event_defs.h,common.c}—node_span_event_t,EVENT_NODE_SPAN(21), BTF anchor.pkg/ebpf/common/{common.go,node_otel_transform.go}— ringbuf dispatch +decoder producing the same
EventTypeManualSpanshape as the Go path(attributes encoded identically to
tracesgen.SpanAttr; OTel-JSSpanStatusCodemapped to Gocodes.Code, which are inverted).pkg/ebpf/instrumenter.go— fixes a pre-existing double-attach:"node"and
"libuv.so"both fall back to the executable, souv_fs_accesswasuprobed twice and every sentinel fired twice; added per-module
(symbol, program) dedup.
pkg/obi/config.go+ regenerateddevdocs/config/{config-schema.json,CONFIG.md}—NodeJSConfig.ManualSpans.devdocs/nodejs-manual-spans.md— full architecture, constraints, anddiagnostics doc.
Testing
make docker-generate(BPF compiles amd64+arm64),go vet,golangci-lint,clang-format,
make check-config-schema— all clean.pkg/ebpf/common/node_otel_transform_test.go(re-anchoring, attribute encoding, status mapping, timing, error paths).
internal/test/integration/traces_node_manual_test.go(wired into
TestSuite_NodeJS): a/manualroute on the Node test servercreates nested manual spans via
@opentelemetry/apiwith no SDK; the testasserts via Jaeger that they're captured, carry their attributes/status, nest
correctly, and re-anchor onto OBI's automatic trace. Full suite passes on a
Linux VM including weaver schema validation.
Known limitations
values are stringified (matches the Go path's
otel_attribute_tbudgets); onpayload overflow (~1900 B) attributes/events are dropped so the core span
still ships.
traceStatearen't forwarded.children, of the manual span (they parent via the fd-correlation map, not
traces_ctx_v1). Go-parity nesting is implemented on a follow-up branch.inertness guard.
Validation