feat: send probe logs to the api #314
Conversation
Walkthrough
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/lib/api-transport.ts (3)
4-8: Consider renamingenabledtosendingEnabledfor API consistency.The
ApiTransportSettingstype usesenabledwhile the internal property issendingEnabled. This inconsistency could confuse API consumers.export type ApiTransportSettings = { - enabled?: boolean; + sendingEnabled?: boolean; bufferSize?: number; sendInterval?: number; };
17-22: Info type should align with Winston's log format structure.The custom
Infotype may not capture all Winston log properties. Consider extending from Winston's native types or making it more flexible.-type Info = { - message: string; - timestamp: string; - level: string; - type: string; -}; +type Info = winston.Logform.TransformableInfo & { + type?: string; +};
42-55: Handle potential race condition in buffer management.The async
setImmediatecall for emitting 'logged' could create timing issues where the buffer is modified before the event is processed.Consider moving the emit after buffer operations:
override log (info: Info, callback?: () => void) { - setImmediate(() => this.emit('logged', info)); - this.logBuffer.push(info); const bufferLength = this.logBuffer.length; const bufferOverflow = bufferLength - this.bufferSize; if (bufferOverflow > 0) { this.logBuffer = this.logBuffer.slice(bufferOverflow); this.droppedLogs += bufferOverflow; } + setImmediate(() => this.emit('logged', info)); callback && callback(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(1 hunks)src/lib/api-transport.ts(1 hunks)src/lib/logger.ts(2 hunks)src/probe.ts(3 hunks)test/unit/lib/api-transport.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit Configuration File
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
test/unit/lib/api-transport.test.tssrc/lib/logger.tssrc/lib/api-transport.tssrc/probe.ts
🔇 Additional comments (4)
package.json (1)
29-30: LGTM!The dependency addition is correctly placed and the version constraint is appropriate for winston-transport.
src/lib/logger.ts (1)
5-7: LGTM!Clean integration of the ApiTransport into the logger configuration.
src/probe.ts (2)
13-14: LGTM!Proper imports for the API transport integration.
107-107: LGTM!Correctly assigns the socket to the transport after the stats agent is initialized.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/lib/api-transport.ts (4)
52-58: Clear timer reference and unref interval so it doesn’t keep the process aliveWhen disabling sending, keep
this.timerin a known state; when enabling,unref()prevents the flush timer from blocking process exit.- _setInterval () { - this.timer && clearInterval(this.timer); - - if (this.sendingEnabled) { - this.timer = setInterval(() => this._sendLogs(), this.sendInterval); - } - } + _setInterval () { + if (this.timer) { + clearInterval(this.timer); + this.timer = undefined; + } + + if (this.sendingEnabled) { + this.timer = setInterval(() => this._sendLogs(), this.sendInterval); + // Avoid keeping the event loop alive solely due to this interval + this.timer.unref?.(); + } + }
12-17: Widen Info typing to preserve metadata and avoid avoidable type friction with WinstonWinston’s info objects can carry arbitrary metadata. Narrow typing risks future incompatibilities or loss of fields.
-type Info = { - message: string; - timestamp: string; - level: string; - scope: string; -}; +type Info = Record<string, unknown> & { + message: string; + level: string; + timestamp?: string; + scope?: string; +};No behavioral change required elsewhere;
logBufferandlogsignatures remain compatible.Also applies to: 24-24, 37-37
75-81: Specify return type for getCurrentSettings for clearer API surfaceHelps callers and keeps the shape aligned with exported settings type.
- getCurrentSettings () { + getCurrentSettings (): ApiTransportSettings { return { sendingEnabled: this.sendingEnabled, bufferSize: this.bufferSize, sendInterval: this.sendInterval, }; }
60-73: Consider exposing a manual flush() and close() for lifecycle controlIf the logger is torn down or the process is exiting, having explicit
flush()/close()lets you push any buffered logs and stop the timer deterministically. This is especially helpful in tests and graceful shutdown paths.Example methods to add (place inside the class):
public flush() { // Only send when enabled and connected; mirrors _sendLogs guardrails this._sendLogs(); } public close() { if (this.timer) { clearInterval(this.timer); this.timer = undefined; } this.flush(); }If you want, I can wire this into the existing logger lifecycle in a follow-up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/lib/api-transport.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit configuration file
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
src/lib/api-transport.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test code / Run e2e tests (20.x)
- GitHub Check: Test docker / Publish Docker image
- GitHub Check: Test code / Run tests (18.x)
- GitHub Check: Test code / Run tests (18.x)
- GitHub Check: Test code / Run tests (20.x)
- GitHub Check: Test code / Run e2e tests (20.x)
- GitHub Check: Test docker / Publish Docker image
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/unit/lib/api-transport.test.ts (2)
69-69: Drop redundant setSocket — the helper already injects the socketcreateTransportAndLogger passes the stubbed socket into the constructor, so calling setSocket again is unnecessary and may confuse future readers.
- transport.setSocket(socket);
12-16: Consider a lighter Socket stub to reduce brittlenesssinon.createStubInstance(Socket) can be fragile if Socket’s runtime shape changes under ESM. A minimal stub with just connected and emit is enough for these tests and avoids prototype coupling.
Example (no diff required):
socket = { connected: true, emit: sandbox.stub() } as unknown as Socket;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/lib/api-transport.ts(1 hunks)src/probe.ts(3 hunks)test/unit/lib/api-transport.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/api-transport.ts
- src/probe.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit configuration file
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
test/unit/lib/api-transport.test.ts
🧬 Code graph analysis (1)
test/unit/lib/api-transport.test.ts (2)
test/utils.ts (1)
useSandboxWithFakeTimers(220-229)src/lib/api-transport.ts (1)
ApiTransportOptions(10-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test code / Run tests (20.x)
- GitHub Check: Test code / Run tests (18.x)
- GitHub Check: Test code / Run tests (22.x)
- GitHub Check: Test docker / Publish Docker image
- GitHub Check: Test code / Run e2e tests (20.x)
- GitHub Check: Test docker / Publish Docker image
- GitHub Check: Test code / Run tests (22.x)
- GitHub Check: Test code / Run e2e tests (20.x)
- GitHub Check: Test code / Run tests (18.x)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/lib/api-transport.ts (2)
52-54: Schedule immediately on socket set to reduce first-send latencyIf
sendingEnabledis already true and a socket is attached later, schedule a send instead of waiting for the next pre-existing timer.setSocket (socket: Socket) { this.socket = socket; + if (this.sendingEnabled) this.setTimeout(); }
71-79: Rename helper to avoid confusion with globalsetTimeoutThe method name shadows the global timer function and can confuse readers. Consider
scheduleNextSendor similar.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/lib/api-transport.ts(1 hunks)test/unit/lib/api-transport.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/lib/api-transport.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit configuration file
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
src/lib/api-transport.ts
🧠 Learnings (1)
📚 Learning: 2025-08-21T15:14:46.182Z
Learnt from: PavelKopecky
PR: jsdelivr/globalping-probe#314
File: src/lib/api-transport.ts:83-88
Timestamp: 2025-08-21T15:14:46.182Z
Learning: In the globalping-probe codebase, input validation for API transport settings is not required because the data always comes from their proprietary API, which is a trusted source.
Applied to files:
src/lib/api-transport.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test docker / Publish Docker image
- GitHub Check: Test code / Run e2e tests (20.x)
- GitHub Check: Test docker / Publish Docker image
- GitHub Check: Test code / Run e2e tests (20.x)
… and sendingEnabled to isActive
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
src/lib/api-logs-transport.ts (4)
80-84: Prevent timers from keeping the process aliveUnref the timer so the transport does not block process exit when active.
Apply:
- this.timer = setTimeout(() => { + this.timer = setTimeout(() => { void this.sendLogs(); }, this.sendInterval); + this.timer.unref?.();
20-29: Expose a proper shutdown to clear timersProvide a
close()to clear the pending timer when the transport is removed or during graceful shutdown.Add (outside the selected lines, near class end):
close() { clearTimeout(this.timer); this.timer = undefined; }
11-18: Avoid over-constraining the info shapeWinston
infocarries arbitrary metadata; requiringtimestampandscoperisks type friction. Loosen the type to the minimal contract you use.Apply:
-type Info = { - message: string; - timestamp: string; - level: string; - scope: string; -}; +type Info = { + message: string; + level: string; +} & Record<string, unknown>;
114-116: Include error context consistentlyLog the error as metadata to preserve stack and context across transports.
Apply:
- this.logger?.error('Failed to send logs to the API.', e); + this.logger?.error('Failed to send logs to the API.', { err: e });test/unit/lib/api-logs-transport.test.ts (2)
198-224: Cover rejection path to assert error loggingSimulate
emitWithAckrejecting (network error) and assert that the transport logs an error and reschedules.Example:
it('should log error and reschedule when emitWithAck rejects', async () => { const { transport, logger } = createTransportAndLogger({ isActive: true, sendInterval: 100 }); const errorSpy = sinon.spy(); // Wire a logger for the transport to capture error() (transport as any).setLogger({ error: errorSpy } as unknown as winston.Logger); socket.emitWithAck.callsFake(() => Promise.reject(new Error('network'))); logger.info('test'); await sandbox.clock.tickAsync(100 + 1); expect(errorSpy.called).to.be.true; // After failure, it should try again on next interval setEmitWithAckResponse('success'); await sandbox.clock.tickAsync(100 + ACK_DELAY); expect(socket.emitWithAck.callCount).to.be.greaterThan(1); });
272-307: Test clamping of invalid settingsAfter clamping logic, add tests to ensure
sendIntervalandmaxBufferSizeare bounded and that disabling works immediately.Example:
it('should clamp sendInterval and maxBufferSize to sane minimums', async () => { const { transport, logger } = createTransportAndLogger({ isActive: true, sendInterval: 1000, maxBufferSize: 10 }); transport.updateSettings({ sendInterval: 0, maxBufferSize: 0 }); const s = transport.getCurrentSettings(); expect(s.sendInterval).to.be.at.least(100); expect(s.maxBufferSize).to.be.at.least(1); logger.info('x'); await sandbox.clock.tickAsync(s.sendInterval); expect(socket.emitWithAck.calledOnce).to.be.true; });src/probe.ts (1)
14-14: Use type-only import to avoid unnecessary runtime dependency and cycles.Mark
ApiTransportSettingsas a type-only import.-import { ApiTransportSettings } from './lib/api-logs-transport.js'; +import type { ApiTransportSettings } from './lib/api-logs-transport.js';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(1 hunks)src/lib/api-logs-transport.ts(1 hunks)src/lib/logger.ts(2 hunks)src/probe.ts(3 hunks)test/unit/lib/api-logs-transport.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit configuration file
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
src/lib/logger.tstest/unit/lib/api-logs-transport.test.tssrc/lib/api-logs-transport.tssrc/probe.ts
🔇 Additional comments (4)
package.json (1)
29-31: No locking required for winston-transport: winston-transport@^4.9.0 has no peerDependency on winston and both packages declare support for Node ≥12.0.0, so using "^3.17.0" and "^4.9.0" is safe across Node 18/20/22/24.src/lib/logger.ts (1)
7-7: Constructor is side-effect free
TheApiLogsTransportconstructor only sets properties and defers timer setup to the send logic (gated byisActive); no timers or I/O run at instantiation.src/probe.ts (2)
107-108: Ensure reconnection semantics are handled.
setSocket(socket)is called once. Socket.IO reuses the same object across reconnects, but if your transport internally caches listeners/state tied to connection lifecycle, verify it correctly handles disconnect/reconnect without needing anothersetSocket.If the transport needs explicit hooks, add:
- on('connect', () => apiLogsTransport.onConnect?.())
- on('disconnect', () => apiLogsTransport.onDisconnect?.())
156-158: Verify API payload keys before normalization
Confirm whether the backend event actually emits legacyenabledor currentsendingEnabled(and the numeric fields) and adjust the normalization logic only if those variants are present.
|
🎉 This PR is included in version 0.40.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Relates to jsdelivr/globalping-dash#26