✨ Add startup ping with version and instanceId to setup webhook #89
Conversation
Send server version and persistent instanceId to n8n webhook on first startup. Skips on subsequent restarts (cached) and in CI environments.
Send server version and instanceId to n8n webhook on first startup or when version changes. Tracks both values in separate cache keys to detect upgrades while maintaining same instanceId.
There was a problem hiding this comment.
Pull request overview
This PR implements a startup telemetry mechanism that sends a one-time ping to an n8n webhook containing the server version and a persistent instanceId. The ping is designed to execute only on the first server startup, with subsequent restarts being skipped by checking for an existing instanceId in Redis cache.
Key Changes:
- Added startup ping function with version detection from package.json
- Integrated cache-based deduplication using 365-day expiration
- Implemented environment-based opt-out (CI environments and DISABLE_PING flag)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| apps/api/src/startup/ping-packmind-setup.ts | New module implementing the startup ping logic with version detection, cache-based deduplication, and webhook communication |
| apps/api/src/main.ts | Integrated pingPackmindSetup call after server startup with fire-and-forget error handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| const cache = Cache.getInstance(); | ||
|
|
||
| // Check if ping is disabled via environment variable | ||
| const disablePing = await Configuration.getConfig('DISABLE_PING'); | ||
| if (disablePing === 'true') { | ||
| logger.info('Ping disabled via DISABLE_PING environment variable'); | ||
| return; |
There was a problem hiding this comment.
There is a potential race condition in this implementation. If multiple server instances start simultaneously (common in container orchestration like Kubernetes), they could all check the cache at line 102 before any has written to it, resulting in multiple pings being sent with different instanceIds. Consider using a distributed lock or atomic cache operation (like Redis SETNX) to ensure only one instance generates and stores the instanceId.
| await cache.set( | ||
| INSTANCE_ID_CACHE_KEY, | ||
| newInstanceId, | ||
| CACHE_EXPIRATION_SECONDS, | ||
| ); | ||
| await cache.set( | ||
| VERSION_CACHE_KEY, | ||
| currentVersion, |
There was a problem hiding this comment.
The instanceId is being stored in cache before verifying that the webhook ping was successful. If the webhook call fails (lines 144), the instanceId remains in cache and prevents retries on subsequent startups. Consider storing the instanceId only after a successful ping response, or implement a separate flag to track successful ping completion.
| } | ||
| } catch { | ||
| // Continue to next path | ||
| continue; |
There was a problem hiding this comment.
The catch block at line 40 uses an empty catch with continue, but the continue statement is unnecessary since it's the last statement in the try-catch block within the loop. The loop would naturally continue to the next iteration. Consider removing the explicit continue for cleaner code.
| continue; |
| pingPackmindSetup().catch((error) => { | ||
| logger.warn('Ping to Packmind setup webhook failed', { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The error handling is redundant. The pingPackmindSetup function already has comprehensive try-catch blocks that prevent any errors from propagating (lines 145-149 in ping-packmind-setup.ts). This outer catch block in main.ts will never execute because pingPackmindSetup never throws. Consider removing this redundant error handler or simplifying the error handling strategy.
| pingPackmindSetup().catch((error) => { | |
| logger.warn('Ping to Packmind setup webhook failed', { | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| }); | |
| pingPackmindSetup(); |
| const INSTANCE_ID_CACHE_KEY = 'packmind:instanceId'; | ||
| const VERSION_CACHE_KEY = 'packmind:version'; | ||
| const CACHE_EXPIRATION_SECONDS = 365 * 24 * 60 * 60; // 365 days | ||
| const WEBHOOK_URL = |
There was a problem hiding this comment.
The webhook URL is hardcoded, which makes it difficult to test in different environments (development, staging, production) and prevents users from pointing to their own telemetry endpoints if desired. Consider making the webhook URL configurable via environment variable (e.g., PING_WEBHOOK_URL) with this URL as the default fallback.
| const WEBHOOK_URL = | |
| const WEBHOOK_URL = | |
| process.env.PING_WEBHOOK_URL || |
|
|
||
| logger.info('Sending ping to Packmind setup webhook', { | ||
| url: WEBHOOK_URL, | ||
| version, | ||
| instanceId, | ||
| }); | ||
|
|
||
| await axios.post(WEBHOOK_URL, payload, { | ||
| timeout: 10000, // 10 second timeout | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); | ||
|
|
||
| logger.info('Successfully sent ping to Packmind setup webhook', { | ||
| version, | ||
| instanceId, |
There was a problem hiding this comment.
The instanceId is being logged at INFO level in multiple places (lines 66-70, 79-82, 132-134), which could expose this identifier in production logs. Since this is meant to be a persistent identifier for tracking, consider logging it at DEBUG level instead to reduce exposure in production log aggregation systems, or ensure that your logging infrastructure properly handles sensitive identifiers.
| const possiblePaths = [ | ||
| path.join(process.cwd(), 'package.json'), // Current working directory (works in most cases) | ||
| path.join(__dirname, '..', '..', '..', '..', 'package.json'), // From apps/api/src/startup to root | ||
| path.join(__dirname, 'package.json'), // In dist/apps/api (production build) |
There was a problem hiding this comment.
The comment on line 22 mentions "dist/apps/api/package.json (copied during build)" but there's no corresponding path in the possiblePaths array that checks for dist/apps/api/package.json. Path 3 checks "__dirname/package.json" which would only work if __dirname is dist/apps/api, but this is not explicitly documented. Consider adding an explicit path like "path.join(__dirname, '..', '..', 'package.json')" to match the comment, or update the comment to accurately reflect the paths being checked.
| path.join(__dirname, 'package.json'), // In dist/apps/api (production build) | |
| path.join(__dirname, '..', '..', 'package.json'), // In dist/apps/api (production build) |
|
Send server version and persistent instanceId to n8n webhook on first startup. Skips on subsequent restarts (cached) and in CI environments.
Explanation
This PR adds a startup ping mechanism that sends server telemetry to an n8n webhook when the Packmind API server starts for the first time. The ping includes:
The ping is sent only once on the first startup. Subsequent server restarts skip the ping by checking if an instanceId already exists in the cache. The ping is also disabled in CI environments (CI=true) and can be manually disabled via DISABLE_PING=true.
Relates to #
Type of Change
Affected Components
Testing
Test Details:
Manual testing scenarios:
Reviewer Notes
Design Decisions: