[santigimeno]

Include events and recorded exceptions in gRPC and OTLP spans. Pass links from startSpan() through native storage to the shared exporter, including their attributes and trace context.

Reuse attribute conversion for spans, events and links, and preserve event timestamps, including hrtime nanoseconds.

Fixes: #569

[ns-control-tower]

Note

Reviewing the latest changes on this PR — this may take a few minutes.
This comment will be updated in place with the findings; no other top-level comments will be posted.

[ns-control-tower]

Walkthrough

Exports span events, recorded exceptions and startSpan() links through every OTLP export path (fixes #569). The OTel shim (lib/internal/otel/trace.js) serializes links into kSpanLink JSON — context, trace state, attributes — and events into kSpanEvent JSON; both travel as strings on Tracer::SpanStor (src/nsolid.h:969-970) via the new prop type (src/nsolid/nsolid_trace.h:31, nsolid_trace.cc:107). fill_recordable() (agents/otlp/src/otlp_common.cc:734-789) converts them into AddEvent/AddLink calls, reusing the new JsonAttributes KeyValueIterable (:577) for spans, events and links alike. The new test (test/agents/test-tracing-extras.mjs) exercises all three agent exporters, malformed inputs included.

Re-review of the delta since the last review (f1bbb1e → c10450cb): c10450cb re-indents the flagged test row — a one-line whitespace change, and the entire delta. The four commits before it (4c75d10 collection caps, f7dd4e2 relative event times, ccd2895 malformed links, f1bbb1e Date conversion) remain as previously verified; together they address the human review's two "before merge" items and two optional notes.

Assessment

• ✔ Lint — test/agents/test-tracing-extras.mjs:129: the continuation row now carries the expected 34 leading spaces (it had 35), the exact one-space fix the inline suggestion proposed; the delta from f1bbb1e is this single line, so @stylistic/js/indent no longer flags it. Linters passed on this head (run 39944141253, success).
• ✔ Caps — agents/grpc/src/grpc_agent.cc:1126-1131, agents/otlp/src/otlp_agent.cc:591-594,604-607: all three setups set the four collection limits to the SDK defaults (128). Verified against the vendored exporter: MakeRecordable() passes the options into OtlpRecordable, whose AddEvent/AddLink drop past the caps and count dropped_events_count/dropped_links_count/dropped_attributes_count; max_attributes is untouched, so the span-attribute limit is unchanged. The test asserts 131 span attributes and 128 + 1 dropped for each event and link (test-tracing-extras.mjs:118-122).
• ✔ performance.now() — lib/internal/otel/trace.js:111-114: nonnegative numbers below the time origin get getTimeOriginTimestamp() added, and recordException inherits it through addEvent; relative event and exception timestamps are covered (test-tracing-extras.mjs:144-152).
• ✔ Optional notes — lib/internal/otel/trace.js:77-80 skips links with a missing context or a traceState without a callable serialize (these used to throw out of startSpan()); lib/internal/otel/trace.js:116-118 converts only instanceof Date, matching isTimeInput(), so plain objects and new Date(NaN) fall through to the native timestamp validation instead of throwing.
• ✔ Earlier blocker stays fixed — agents/otlp/src/otlp_common.cc:734: the file is unchanged since c2936b0 and still carries the non-throwing parse, type gates, object check for attributes, and link validation; the test asserts every rejection path.
• Validation: source-level re-review of the f1bbb1e → c10450cb delta — a single-line whitespace fix, no other files touched since the last review — on top of the earlier verification of the vendored OtlpRecordable/SpanLimits semantics and all three exporter construction sites; NSOLID_SPAN_PROP_TYPES_STRINGS feeds the JS nsolid_consts from the same enum at runtime, so the mid-list kSpanLink insertion needs no regenerated artifact; the new test is discovered by test/agents/testcfg.py and runs under make test-ci (agents prereqs). C++ lint (lint-cpp) was green at f1bbb1e; on c10450cb the Linters re-run completed green — the check that was red is fixed — and the test/build jobs are still running.
• 🚩 Human review still recommended — the tracing/export pipeline is shared by all agents.
• Assumptions: ZMQ parity stays out of scope (agents/zmq/src/zmq_agent.cc:1924 reads stor.events and has no links field).

Verdict: APPROVE — the only blocker from the previous review (the lint-js indent in the new test) is fixed in code at c10450cb; the delta since f1bbb1e is that one line and nothing else, and no new findings.

[brycebaril]

Thanks, Santi. Checked at c2936b0. One attribute converter for spans, events and links is the right shape. The link checks in fill_recordable hold up: hex length and digits, flags from 0 to 255, and a valid context. The test covers all three exporters, bad inputs included.

Two things before merge:

1. Nothing caps events or links any more. Until now s.events never left the process, so their number didn't matter. Now every event and link goes out, and the exporters' options leave max_events, max_links and the per-event and per-link attribute limits at uint32 max: agents/grpc/src/grpc_agent.cc:1124, and setup_trace_otlp_exporter and setup_trace_grpc_otlp_exporter in agents/otlp/src/otlp_agent.cc. A long-lived span that adds an event per item or per retry can carry thousands. One export over the receiver's message limit (4 MB by default in gRPC) fails with every span in it. The spec's defaults, 128 each, on those three sets of options let OtlpRecordable drop the rest and report dropped_events_count and dropped_links_count.
2. An event time from performance.now() lands in 1970. The OTel API's TimeInput is "hrtime, epoch milliseconds, performance.now() or Date", and the JS SDK reads a number below the time origin as performance.now(). fill_recordable takes every number as epoch milliseconds (otlp_common.cc:744), so span.addEvent('x', {}, performance.now()) is exported a few seconds after 1970, and recordException with such a time too. In addEvent, adding getTimeOriginTimestamp() when startTime is a number below it fixes both; the helper is already imported.

CI's test jobs on c2936b0 ended in a runner shutdown (exit 143) while still building, so the new test hasn't run yet; a re-run settles it. The Windows build fails in libsodium on other PRs on this base too.

Optional notes are inline.

[brycebaril]

Before merge: from here on nothing limits how many events a span sends. The exporters' options leave max_events and max_links, and the per-event and per-link attribute limits, at uint32 max. So a span with thousands of events goes out whole, and an export over the receiver's limit fails with all its spans. The spec's defaults, 128 each, on the options at grpc_agent.cc:1124 and in the two setup_trace_* functions in otlp_agent.cc let OtlpRecordable drop the rest and count them.

[brycebaril]

Before merge: every number is taken as epoch milliseconds here, but TimeInput also allows performance.now(), which the JS SDK tells apart as a number below the time origin. Such an event is exported a few seconds after 1970. Adding getTimeOriginTimestamp() in addEvent when startTime is a number below it covers recordException too, since it goes through addEvent.

[brycebaril]

Optional: the test shows a bad link is meant to be skipped, but two kinds throw out of startSpan instead. isSpanContextValid reads link.context.traceId, so a link with an undefined context throws; trace.getSpanContext() returns undefined for a context with no span. Line 85 calls serialize() on any traceState, so a plain string throws too. Checking link?.context first, and typeof context.traceState?.serialize === 'function' on line 85, skips both like the other bad links.

[brycebaril]

Optional: DatePrototypeGetTime throws for any object that isn't a Date, so span.addEvent('x', {}, {}) throws where other bad times are dropped. startTime instanceof Date, as isTimeInput checks, keeps this to Dates.

[brycebaril]

Thanks, Santi. Checked at c10450c. The rebase left the first commit as I reviewed it, and the four commits after it cover everything from my last review:

1. Caps. All three trace exporters now set max_events, max_links and the per-event and per-link attribute limits to the SDK's 128: grpc_agent.cc:1126-1131, setup_trace_otlp_exporter and setup_trace_grpc_otlp_exporter. Both agents get their recordables from MakeRecordable(), which passes those options to OtlpRecordable, so it keeps 128 of each and counts the rest as dropped. Span attributes stay unlimited, as before.
2. performance.now() times. addEvent adds the time origin to a number between 0 and the origin, as the JS SDK does, and recordException goes through it.
3. Malformed links. A link with no context, or with a trace state that has no serialize, is skipped instead of throwing out of startSpan. One whose serialize returns something other than a string is dropped on the native side.
4. Object times. Only Dates are converted, so {} and an invalid Date are dropped instead of throwing.

The test checks all of this on all three exporters, and its checks fail without the fixes:

• 128 events and links with one dropped, and 128 attributes on each with one dropped.
• performance.now() times that land between the span's start and now.
• The malformed links and times.

It passed on macOS and in the Linux coverage job. The other failures on this head are the ones every PR on this base has: Linux runners shut down mid-run, the Windows build fails in libsodium, and the macOS failures are tests that fail on other PRs too.

Approving. One thing for a separate issue, not this PR: span start and end times still accept only performance.now() values.

• startSpan(name, { startTime }) and span.end(endTime) send the value as given, and Span::add_prop adds the process start time to it (src/nsolid/nsolid_trace.cc:47-70).
• So a start given in epoch milliseconds lands decades ahead.
• A Date or an HrTime array reaches pushSpanDataDouble, which expects a number.

Converting them in JS, as addEvent now does, would match the JS SDK.