SED-4789 Implement live reporting in the Node.js agent by jeromecomte · Pull Request #661 · exense/step

jeromecomte · 2026-06-22T20:47:15Z

No description provided.

gemini-code-assist

Code Review

This pull request introduces live reporting capabilities for the Node.js agent, enabling running keywords to stream performance measures, metrics, and file uploads to the controller during execution. Key feedback from the review focuses on improving robustness and error handling: wrapping the initialization of liveReporting in a try-catch block and checking its existence before closing in agent-fork.js, adding an error listener to the HTTP response stream in BatchingRestPoster, using Math.floor instead of the modulo operator to prevent floating-point precision issues during metric bucketing, and addressing potential stack corruption in LiveMeasures during concurrent asynchronous operations.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist · 2026-06-22T20:49:38Z

    const outputBuilder = new OutputBuilder();
+    const liveReporting = createLiveReporting(properties);
    try {


To prevent any unexpected errors during createLiveReporting initialization from crashing the fork process, declare liveReporting using let and initialize it inside the try block. This ensures any initialization errors are caught and reported gracefully via outputBuilder.fail.

const outputBuilder = new OutputBuilder(); let liveReporting; try { liveReporting = createLiveReporting(properties);

Done — liveReporting is now declared with let and initialized as the first statement inside the try, so any initialization error is reported via outputBuilder.fail instead of escaping the message handler.

gemini-code-assist · 2026-06-22T20:49:38Z

+      try {
+        await liveReporting.close();
+      } catch (e) {
+        console.log("[Agent fork] Error while closing live reporting", e);
+      }


Add a safety check to ensure liveReporting is defined before attempting to call close(), preventing a TypeError if initialization failed or was skipped.

try { if (liveReporting) { await liveReporting.close(); } } catch (e) { console.log("[Agent fork] Error while closing live reporting", e); }

References

In the agent forker process (agent-fork.js), use console.log for logging as the main logger utility is not available.

Done — added the if (liveReporting) guard before await liveReporting.close().

gemini-code-assist · 2026-06-22T20:49:38Z

+        (resp) => {
+          const status = resp.statusCode;
+          // Always drain the response so the socket can be reused/released.
+          resp.on('data', () => {});
+          resp.on('end', () => {
+            if (status !== 204) {
+              logger.error(`Error while reporting ${items.length} ${this.label}(s). The live reporting service returned ${status}`);
+            }
+            resolve();
+          });
+        }


In Node.js, the response object (IncomingMessage) can emit an 'error' event (e.g., if the connection is severed mid-response). To prevent potential unhandled exceptions from crashing the process, it is recommended to attach an error listener to the resp stream.

(resp) => { const status = resp.statusCode; resp.on('error', (err) => { logger.error(`Response error while reporting ${items.length} ${this.label}(s):`, err); }); // Always drain the response so the socket can be reused/released. resp.on('data', () => {}); resp.on('end', () => { if (status !== 204) { logger.error(`Error while reporting ${items.length} ${this.label}(s). The live reporting service returned ${status}`); } resolve(); }); }

Done — added a resp.on('error') handler that logs and settles the flush promise. (This code now lives in live-reporting/shared.js after the file split discussed below.)

gemini-code-assist · 2026-06-22T20:49:38Z

+    const bucket = value - (value % this._percentilePrecision);
+    this._distribution.set(bucket, (this._distribution.get(bucket) || 0) + 1);


Using the modulo operator % on floating-point numbers in JavaScript can lead to precision issues (e.g., 0.3 % 0.1 results in 0.09999999999999998), which can cause incorrect or fragmented bucket keys in the distribution map. Consider using Math.floor for more robust bucketing.

Suggested change

const bucket = value - (value % this._percentilePrecision);

this._distribution.set(bucket, (this._distribution.get(bucket) || 0) + 1);

const bucket = Math.floor(value / this._percentilePrecision) * this._percentilePrecision;

this._distribution.set(bucket, (this._distribution.get(bucket) || 0) + 1);

Done — bucketing now uses Math.floor(value / precision) * precision.

gemini-code-assist · 2026-06-22T20:49:38Z

+class LiveMeasures {
+  constructor(destination) {
+    this.destination = destination;
+    this._stack = [];
+  }


The LiveMeasures class uses a shared array stack (this._stack) to track nested measures. In an asynchronous environment like Node.js, if a keyword executes concurrent overlapping asynchronous operations (e.g., using Promise.all), their stack operations will interleave and corrupt the stack. Consider documenting this limitation, or using AsyncLocalStorage to scope the measure stack to the asynchronous execution context.

Documented the limitation on LiveMeasures: startMeasure/stopMeasure share one stack and are intended for sequential or properly nested measures within a single flow; concurrent overlapping async measures should use addMeasure, which records a fully-formed measure atomically. I kept this approach rather than AsyncLocalStorage to stay aligned with the Java LiveMeasures stack semantics.

david-stephan

@jeromecomte all gemeni comments look legit. I added one point of concern about multithreading. I went a big fast on the file streaming code. you may ask Christoph to check it in more details if you'd like.

david-stephan · 2026-06-23T11:06:25Z

+  flush() {
+    if (this.buffer.length === 0) return;
+    const batch = this.buffer;
+    this.buffer = [];


I didn't check the Java implementation, but with multithreading and no synchronization between add and flush we can loose data. (i.e. a 2nd thread run line 151 (this.buffer.push(item);), while the 1st thread is in between L159-160

As discussed, as l158 - l160 contain no async operation, they are guarantied to run atomically on the same Node.js event loop

david-stephan · 2026-06-23T11:31:32Z

    fs.mkdirSync(agentForkerLibPath, { recursive: true });
    fs.copyFileSync(path.resolve(__dirname, 'agent-fork.js'), path.join(agentForkerLibPath, 'agent-fork.js'));
    fs.copyFileSync(path.join(__dirname, 'output.js'), path.join(agentForkerLibPath, 'output.js'));
+    fs.copyFileSync(path.join(__dirname, 'live-reporting.js'), path.join(agentForkerLibPath, 'live-reporting.js'));


I assume this is the reason of the "big" live-reporting.js file, but for code review and maintainability it would have been nicer to split the code.

Agree with this. I would also prefer to split the code in dedicated files. In theory we could already do it and simply add more files here but I would prefer to have a better solution than the copy first

Done — live-reporting.js is now split into a live-reporting/ folder: index.js (container + factory + public API), shared.js (logger, reporting-URL resolution, the batching REST poster), measures.js, metrics.js, and file-uploads.js. To avoid maintaining a per-file copy list in agent.js, the agent now copies the whole folder recursively (fs.cpSync(..., { recursive: true })), so adding new modules no longer requires touching agent.js.

jeromecomte · 2026-06-24T12:16:54Z

Thanks for the review @david-stephan! All points addressed (replied inline as well):

Initialize liveReporting inside the try + guard close() — done
Response error listener on the batching REST poster — done
Math.floor bucketing for the metric distribution — done
Documented the startMeasure/stopMeasure stack concurrency limitation (use addMeasure for concurrent async operations), matching the Java LiveMeasures semantics — done
add/flush data loss: the buffer swap is synchronous, so it runs atomically on the single-threaded event loop (discussed in thread) — no change needed
Maintainability: split live-reporting.js into a live-reporting/ folder (index, shared, measures, metrics, file-uploads); agent.js now copies the folder recursively into the fork libs, so future modules don't need a new copy line

All tests and eslint pass.

SED-4789 Implement live reporting in the Node.js agent

45da24f

jeromecomte requested a review from david-stephan June 22, 2026 20:47

gemini-code-assist Bot reviewed Jun 22, 2026

View reviewed changes

david-stephan requested changes Jun 23, 2026

View reviewed changes

jeromecomte added 2 commits June 24, 2026 12:51

SED-4789 PR feedbacks

3860b47

SED-4789 PR feedback: splitting live-reporting.js

d718806

david-stephan approved these changes Jun 24, 2026

View reviewed changes

		const bucket = value - (value % this._percentilePrecision);
		this._distribution.set(bucket, (this._distribution.get(bucket) \|\| 0) + 1);

Conversation

jeromecomte commented Jun 22, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

jeromecomte Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

jeromecomte Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

jeromecomte Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

jeromecomte Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

jeromecomte Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-stephan left a comment

Choose a reason for hiding this comment

Uh oh!

david-stephan Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

jeromecomte Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

david-stephan Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

jeromecomte Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

jeromecomte Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeromecomte commented Jun 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jeromecomte Jun 24, 2026 •

edited

Loading

jeromecomte Jun 24, 2026 •

edited

Loading

jeromecomte Jun 24, 2026 •

edited

Loading

jeromecomte Jun 24, 2026 •

edited

Loading

jeromecomte Jun 24, 2026 •

edited

Loading

jeromecomte Jun 24, 2026 •

edited

Loading