Repository navigation
agents,lib,src: implement OTel metrics API - #634
santigimeno wants to merge 14 commits into
Conversation
WalkthroughRe-review of Every file except that header is byte-identical between
Assessment
CI: at
Verdict: REQUEST_CHANGES — the |
3494718 to
a11a94f
Compare
brycebaril
left a comment
brycebaril
left a comment
There was a problem hiding this comment.
Thanks, Santi. I read the whole change at a11a94f: the JS meter layer, the binding and its store, the collection on each thread's event loop, and the gRPC export. I didn't build it. Lint passes. Both test-linux jobs died during the build, as they have on other PRs to this branch, but coverage-linux ran the JS suites, test/agents included, and none of the new tests are among its failures.
What's right:
- The API matches
@opentelemetry/api's metrics: all seven instrument kinds,addCallbackandremoveCallback, batch callbacks that only reach the instruments they declare, and meters cached by name, version and schema URL. - Attribute sets are canonical, so key order doesn't matter. Each instrument stops at 2,000 sets and puts the rest in
otel.metric.overflow, the spec's default limit. - Observable counters export the observed total, and observable values are cleared before each collection.
- The vendored
max_fix is right: withmin(), a histogram of negative values reported a max of 2.2e-308. The next opentelemetry-cpp update needs to keep it. - There are no new
NSOLID_*settings, so nothing for--helpor the manpage.
Blocking:
- A bad measurement throws. A negative counter increment,
NaNor ±Infinity, anullor object attribute, or a sum past the largest double throws a TypeError fromaddandrecord. The OpenTelemetry spec says API calls must not throw, and the SDK drops such measurements. Inline atmetrics.js:84. register()now takes the global meter provider, and only the gRPC agent sends what it collects. An app that installs its ownMeterProviderafterregister()loses it without an error. Under the OTLP, StatsD or ZMQ agent, the callbacks run on every poll and nothing is sent. Inline atcore.js:40.- A collection can stall. It waits, with no limit, for every async callback to settle, and meanwhile each export repeats the old snapshot, synchronous instruments included. On a thread where nothing else runs JS, it also waits for the next timer or I/O callback: the callbacks run outside a callback scope, so the microtask that finishes the collection doesn't run. Inline at
metrics.js:42andnsolid_api.cc:233. - Points don't say which thread they came from. The same instrument and attributes on two threads export as identical streams, told apart only by the request they arrive in. Inline at
otlp_common.cc:636. nsolid.metrics()now runs every observable callback, synchronously, inside the call. Inline atnsolid.cc:374.
Should fix:
6. Exports are one poll behind, and the last poll is lost. Inline at nsolid.cc:405.
Non-blocking:
7. Each measurement does a JSON round trip and a linear search. Inline at nsolid_otel_metrics_store.cc:139.
|
|
||
| add(value, attributes = {}, context) { | ||
| if (this.#state.disabled) return; | ||
| metrics.add(this.#id, value, JSONStringify(attributes)); |
There was a problem hiding this comment.
The binding throws a TypeError here for measurements the OpenTelemetry SDK accepts or drops with a warning:
- a negative increment on a counter, and
NaNor ±Infinityon any instrument; - an attribute set to
nullor to an object, or an array that mixes types; - an add or record that would take a sum past the largest double.
The validation tests assert most of these. The spec's error-handling rules say API calls must not throw. Since register() puts this provider behind @opentelemetry/api (core.js:40), instrumentation libraries written against that promise would throw inside request handlers.
Suggest dropping the measurement, or just the bad attribute, and warning once per instrument with process.emitWarning. Creating an instrument throws too: for bad histogram advice, and when its name, unit or description, or its meter's name or version, isn't a string. A warning and a fallback (the default boundaries, a working instrument) would match the spec there.
| } | ||
| }); | ||
|
|
||
| enableMeterApi(); |
There was a problem hiding this comment.
register() always installs N|Solid's provider as the global meter provider, before the tracing check and whatever agent is running.
- An app that sets its own
MeterProviderafterwards loses it without an error. The API'ssetGlobalMeterProviderreturns false for a second provider and reports it only throughdiag. - Only the gRPC agent exports what's collected (
grpc_agent.cc:1046). Under the OTLP, StatsD or ZMQ agent, every metrics poll still runs the app's callbacks, and the measurements are never sent.
Suggest making it opt-in, with an option on register() or a call of its own. Then either export through the OTLP agent too, or document that only the gRPC agent sends app metrics.
| // A failed callback must not suppress the remaining observations. | ||
| } | ||
| } | ||
| SafePromisePrototypeFinally( |
There was a problem hiding this comment.
collectionDone() waits for every callback's promise, with no limit. Until it runs, otel_collection_pending_ stays set, no later collection starts, and every export repeats the last snapshot, synchronous instruments included. One callback awaiting a query that never returns stops this thread's app metrics for good.
The OpenTelemetry SDK bounds each callback with the collection timeout. Suggest the same here, with the metrics interval as the limit: complete the collection with what was observed, and drop observations that arrive later.
| Local<Context> context = current_env->context(); | ||
| v8::TryCatch try_catch(isolate_); | ||
| if (otel_callback_.Get(isolate_) | ||
| ->Call(context, v8::Undefined(isolate_), 0, nullptr) |
There was a problem hiding this comment.
This call comes from an event-loop command (run_event_loop_cmds_), with no callback scope around it. collectionDone() runs in a .finally handler, which is a microtask, and Node runs microtasks when a callback scope closes.
So on a thread where nothing else runs JS, such as a server between requests or a worker waiting for messages, the collection finishes only at the next timer, I/O callback or message. Until then the export repeats the old snapshot. The test client's keep-alive setInterval(() => {}, 100) (client.js:25) keeps the tests from seeing this.
Suggest an InternalCallbackScope around the call, as MakeCallback uses, and one test without the keep-alive.
| metrics[{ instrument.meter_name, | ||
| instrument.meter_version, | ||
| instrument.meter_schema_url }]; | ||
| PointAttributes attrs; |
There was a problem hiding this comment.
N|Solid's own thread metrics carry thread.id and thread.name on every point (fill_env_metrics). These points carry only the app's attributes, and the test asserts that.
So the same instrument and attributes on two threads export identical streams. The test's shared-worker-requests is 4 on the main thread and 6 on the worker, both with no attributes, and only the request each arrives in tells them apart. OTLP identifies a stream by resource, scope, name and attributes. Anything that doesn't keep request boundaries sees one cumulative stream with two writers. An app that runs the same instrumentation in every worker would put all its workers on one stream.
The start time is the process's (process_start) for every thread too, so a replacement worker's counter starting over reads as a drop, not a reset.
Suggest adding thread.id and thread.name here unless the app set them, with a start time from the thread or from the instrument's first measurement.
| return UV_EBUSY; | ||
| } | ||
|
|
||
| envinst->CollectOtelMetrics(); |
There was a problem hiding this comment.
nsolid.metrics() reaches this through getEnvMetrics. So every call from app code now runs all the app's observable callbacks, synchronously and inside the call. It also clears their last values and replaces the snapshot the agent sends. Apps call nsolid.metrics() on their own schedule, as often as they like.
Suggest collecting only from the agents' poll, and giving the parallel test an internal way to start a collection.
| } | ||
| }; | ||
| QueueCallback(ret_proxy, tm_sp); | ||
| ei->RequestOtelMetricsCollection(); |
There was a problem hiding this comment.
The agent's callback is queued at :404, before this requests the collection, and the collection has to wait for the event loop. So the agent reads the previous poll's snapshot: each export carries values one interval old, stamped with the export time (otlp_common.cc:598).
Nothing collects when a thread is removed or the process exits. The last interval or two of a worker's or the process's measurements never arrive.
Suggest stamping points with the collection time, and taking a last copy of the store when a thread is removed. The synchronous instruments need no JS for that.
| } | ||
| } | ||
| } | ||
| auto it = std::find_if(instrument.values.begin(), |
There was a problem hiding this comment.
Non-blocking. For each measurement, the attributes are stringified in JS, then parsed, checked and serialized again here, then compared as strings against up to 2,000 sets. A histogram record also copies its whole point with ToPoint() (:188) for the overflow checks. For instruments recorded per request, that adds up.
A hash map from the canonical string, with the sum and count kept beside the aggregation, would make it constant time. The snapshot also copies the six metadata strings into every point, on the app's thread (nsolid_api.cc:324).
Implement the OpenTelemetry metrics API with synchronous and observable instruments, individual and batch callbacks, and external API provider registration. Collect observable measurements on each environment's event loop and publish completed snapshots without delaying native thread metrics. Use the vendored explicit histogram aggregator and bound attribute cardinality. Export each environment's metrics independently through GrpcAgent, preserving scope, instrument metadata and user attributes. Add focused API and export regression coverage.
Contain recording and serialization failures at the JS boundary while preserving native validation before aggregation. Use safe metadata and default histogram boundaries when instrument options are invalid.
Enter a Node callback scope for scheduled observable collection so its promise microtasks finish before the next event-loop command. Cover command ordering and repeated exports from idle main and worker threads without keep-alive timers.
Keep collection timestamps on custom metric snapshots instead of stamping old values at export time. Capture synchronous instruments before orderly environment and process shutdown. Drain final snapshots and flush pending exports before the gRPC exporter closes, without waiting for observable callbacks.
Keep tracing registration enabled by default and require an explicit metrics opt-in. Respect tracing opt-outs when flags change or instrumentations are registered. Preserve application providers on registration conflicts while keeping managed instrumentation cleanup independent of provider ownership.
Send completed custom metric snapshots through the generic HTTP and gRPC exporters, preserving collection timestamps and separate environment batches. Drain final synchronous snapshots on orderly exit. Add coverage for instruments, worker isolation, pending observations, disabling collection and shutdown through both protocols.
Do not invoke observable callbacks or update custom snapshots when nsolid.metrics() reads native process and thread metrics. Keep custom collection on the existing agent path. Use an internal collection binding in the focused API test and verify that runtime reads leave observable callbacks untouched on all threads.
Buffer observations per callback and commit them only after successful completion. Discard failed or timed-out buffers so late observations cannot enter a later collection. Use a 30-second timeout without keeping environments alive, and cancel pending waits when metrics are disabled. Keep promise handling in JS and preserve the existing native collection completion handshake.
Use instrument registration time instead of process start for custom metric points. Preserve it through completed snapshots and cumulative exports so replacement workers report a new accumulation lifetime. Cover cached counters, multiple attribute sets, final snapshots and worker replacement through the generic HTTP and gRPC exporters.
Reuse canonical attributes for previously validated serialized inputs. Bound the cache to 2,000 entries per environment and clear it on disable. Keep value validation and uncached attribute validation unchanged. Cover mutation and invalid updates after cache hits, and add a recording benchmark for counter and histogram attribute cardinalities.
Look up canonical attribute sets by their stored vector position instead of scanning every point. Keep the first-point fast path and index only stored series, including overflow. Clear indexes with observable data.
Track accepted histogram sum and count beside the aggregation for pre-update overflow checks. A bucket cannot exceed the guarded total count. Keep ToPoint() for completed snapshots, without resetting data.
Keep one immutable metadata object per registered instrument and share ownership with completed snapshots. Avoid copying scope and instrument strings into every attribute point while retaining shutdown safety. Keep recorded values and histogram point data owned by each snapshot. Add a benchmark for repeated snapshot collection across cardinalities.
a11a94f to
4ad2203
Compare
Annotate the process/thread metric methods and destructor with override. Mixing annotated and unannotated overrides makes Clang reject the class with -Winconsistent-missing-override enabled.
| std::move(description), | ||
| type, | ||
| std::chrono::system_clock::now() }); | ||
| instruments_.push_back({ std::move(metadata), |
There was a problem hiding this comment.
src/nsolid/nsolid_otel_metrics_store.cc:63
Coverage Windows has failed its Build step on every head of this PR — verified in the logs at a11a94f9a2 (job) and 4ad2203318 (job) — with this error, and this head does not change it:
xmemory(730,9): error : no matching function for call to 'construct_at' [node_base.vcxproj]
... std::_Default_allocator_traits<std::allocator<OtelMetrics::Instrument::Value>>::construct<Value, Value&>
... std::vector<OtelMetrics::Instrument>::_Emplace_reallocate<Instrument>
... nsolid_otel_metrics_store.cc(63,16): 'std::vector<OtelMetrics::Instrument>::push_back' requested here
MSVC STL's vector growth takes the strong-guarantee copy path when !is_nothrow_move_constructible_v<T> && is_copy_constructible_v<T> (if constexpr (is_nothrow_move_constructible_v<_Ty> || !is_copy_constructible_v<_Ty>) move, else copy, in _Emplace_reallocate). Instrument is not nothrow-move-constructible under MSVC STL — its std::unordered_map members' move constructor carries no noexcept — and it still reports copy-constructible, because std::vector<Value>'s copy constructor is unconstrained even though Value's copy is deleted by the unique_ptr member. The reallocation therefore instantiates a copy of Value, and construct_at fails its constraints. libstdc++/libc++ mark unordered_map's move noexcept here, which is why only Windows dies.
Suggested fix: make Instrument explicitly move-only — delete its copy constructor/assignment and default the moves — so the STL must move during reallocation. Since the struct is currently an aggregate under C++20, adding constructors means this push_back call site also needs adjusting (a small metadata/histogram_boundaries constructor, or construct in place); alternatively hold std::unique_ptr<Instrument> in the vector. Then confirm with a Coverage Windows run.
🤖 Ask ns-control-tower to fix this
@ns-control-tower please fix: make OtelMetrics::Instrument explicitly move-only (delete copy, default moves) and adjust the push_back construction at src/nsolid/nsolid_otel_metrics_store.cc:63 so MSVC STL vector growth uses move instead of copy and Coverage Windows builds.
Implement the OpenTelemetry metrics API with synchronous and observable instruments, individual and batch callbacks, and external API provider registration.
Collect observable measurements on each environment's event loop and publish completed snapshots without delaying native thread metrics. Use the vendored explicit histogram aggregator and bound attribute cardinality.
Export each environment's metrics independently through GrpcAgent, preserving scope, instrument metadata and user attributes. Add focused API and export regression coverage.