Telemetry proxy - agent detection#39
Conversation
- Added `getInvocationContext` function to determine if the CLI is invoked by an AI agent or a human based on environment variables. - Updated `trackEvent` to include `invocation_context` and `agent_name` in the payload sent to the telemetry service. - Expanded test coverage for `trackEvent` and `getInvocationContext` to validate new functionality.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR updates the telemetry plugin to (1) send metrics via a proxy using a detached “fire-and-forget” flush worker and a persistent on-disk retry queue, and (2) add AI agent vs human invocation detection and include that context in every event.
Changes:
- Replace direct HTTP posting from the main process with a detached subprocess flush worker (
src/flush-worker.js) and add a persistent retry queue (src/queue-store.js). - Add agent detection via environment variables (plus Copilot PATH markers) and attach
invocation_context/agent_nameattributes to emitted telemetry. - Update tests and README to reflect the new flush architecture and additional telemetry attributes; remove unused runtime deps.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/telemetry-lib.js |
Builds New Relic-style metric payloads, adds agent detection, and spawns detached flush worker instead of POSTing directly. |
src/flush-worker.js |
New detached worker that POSTs merged queued+current metrics and persists failures for retry. |
src/queue-store.js |
New persistent JSON-backed queue store under the aio config directory (XDG-aware). |
test/telemetry-lib.test.js |
Updates tests to assert spawned worker payloads, agent context attributes, env overrides, and postrun duration behavior. |
test/hooks.test.js |
Updates hook interface tests to validate flush-worker spawning instead of direct fetch usage. |
test/queue-store.test.js |
Adds unit tests for queue path resolution and queue read/write/clear behavior. |
test/flush-worker.test.js |
Adds unit tests for flush-worker merging, posting, and queue persistence on failure. |
README.md |
Documents environment opt-out, flush architecture, agent detection, and new payload shape. |
package.json |
Removes unused dependencies (os-name, splunk-logging). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const parsed = JSON.parse(process.argv[2]) | ||
| const { body, postUrl: url, headers } = parsed | ||
| if (!url || typeof url !== 'string') { | ||
| return | ||
| } | ||
| postUrl = url | ||
| if (headers && typeof headers === 'object') { | ||
| const safe = { ...headers } | ||
| delete safe['Api-Key'] | ||
| delete safe['api-key'] | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | ||
| } | ||
| currentMetrics = JSON.parse(body)[0].metrics | ||
| } catch { | ||
| // Malformed argument — nothing useful to do. | ||
| return | ||
| } | ||
|
|
||
| // Merge previously-queued metrics (if any) with the current event so they | ||
| // are all retried in a single POST. | ||
| const queuedMetrics = readQueue() | ||
| const allMetrics = [...queuedMetrics, ...currentMetrics] | ||
|
|
| try { | ||
| debug('POST %s requestHeaders=%o', postUrl, requestHeaders) | ||
| await fetch(postUrl, { | ||
| method: 'POST', | ||
| headers: requestHeaders, | ||
| body: JSON.stringify({ batches: [{ metrics: allMetrics }] }) | ||
| }) | ||
| // Successful delivery — the queue is no longer needed. | ||
| clearQueue() | ||
| } catch { | ||
| // Network failure — persist all metrics so the next invocation can retry. | ||
| writeQueue(allMetrics) | ||
| } |
| // Merge previously-queued metrics (if any) with the current event so they | ||
| // are all retried in a single POST. | ||
| const queuedMetrics = readQueue() | ||
| const allMetrics = [...queuedMetrics, ...currentMetrics] | ||
|
|
||
| try { | ||
| debug('POST %s requestHeaders=%o', postUrl, requestHeaders) | ||
| await fetch(postUrl, { | ||
| method: 'POST', | ||
| headers: requestHeaders, | ||
| body: JSON.stringify({ batches: [{ metrics: allMetrics }] }) | ||
| }) | ||
| // Successful delivery — the queue is no longer needed. | ||
| clearQueue() | ||
| } catch { | ||
| // Network failure — persist all metrics so the next invocation can retry. | ||
| writeQueue(allMetrics) | ||
| } |
| init: (versionString, binName, remoteConf = {}) => { | ||
| global.commandHookStartTime = Date.now() | ||
| rootCliVersion = versionString | ||
| if (remoteConf.fetchHeaders) { | ||
| fetchHeaders = remoteConf.fetchHeaders | ||
| } | ||
| if (remoteConf.postUrl) { | ||
| postUrl = remoteConf.postUrl | ||
| postUrl = remoteConf.postUrl || process.env.AIO_TELEMETRY_POST_URL || DEFAULT_TELEMETRY_POST_URL | ||
| fetchHeaders = { | ||
| 'Content-Type': 'application/json' | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 PR Reviewer
This is a well-structured refactor that replaces a synchronous Splunk/AEP telemetry POST with a fire-and-forget detached subprocess pattern, adds persistent retry queuing, and introduces agent detection. The code is clear, well-tested, and follows good practices. A few minor issues worth addressing: the header filtering in flush-worker.js has a case-sensitivity bug that could allow Api-Key through, the queue can grow unboundedly on repeated failures, and the flushPayload passes fetchHeaders to the worker which then redundantly re-merges DEFAULT_HEADERS.
📝 4 suggestion(s) - Please review inline comments below.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| postUrl = url | ||
| if (headers && typeof headers === 'object') { | ||
| const safe = Object.fromEntries( | ||
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') |
There was a problem hiding this comment.
The filter strips headers whose key lowercased equals 'api-key', but the DEFAULT_HEADERS are merged after, meaning any header named exactly 'Content-Type' passed in fetchHeaders will correctly override. However the intent seems to be blocking sensitive credential headers. The filter only blocks 'api-key' but not common credential headers like 'authorization', 'x-api-key', 'x-ingest-key', etc. At minimum add 'authorization' and 'x-api-key' to the blocklist, or invert to an allowlist.
| postUrl = url | |
| if (headers && typeof headers === 'object') { | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } |
| * @param {Array<object>} items Flat array of New Relic metric objects. | ||
| */ | ||
| function writeQueue (items) { | ||
| const file = getQueuePath() | ||
| try { | ||
| fs.mkdirSync(path.dirname(file), { recursive: true }) | ||
| fs.writeFileSync(file, JSON.stringify(items), 'utf8') | ||
| } catch { | ||
| // silently ignore — failing to persist the queue must not affect the CLI |
There was a problem hiding this comment.
There is no cap on the queue size. If the telemetry endpoint is down for an extended period, the queue will grow indefinitely, consuming disk space. Add a max-items guard before writing.
| * @param {Array<object>} items Flat array of New Relic metric objects. | |
| */ | |
| function writeQueue (items) { | |
| const file = getQueuePath() | |
| try { | |
| fs.mkdirSync(path.dirname(file), { recursive: true }) | |
| fs.writeFileSync(file, JSON.stringify(items), 'utf8') | |
| } catch { | |
| // silently ignore — failing to persist the queue must not affect the CLI | |
| function writeQueue (items) { | |
| const MAX_QUEUE_SIZE = 100 | |
| const file = getQueuePath() | |
| try { | |
| fs.mkdirSync(path.dirname(file), { recursive: true }) | |
| const capped = items.slice(-MAX_QUEUE_SIZE) | |
| fs.writeFileSync(file, JSON.stringify(capped), 'utf8') | |
| } catch { | |
| // silently ignore — failing to persist the queue must not affect the CLI | |
| } | |
| } |
| nodeVersion: process.version, | ||
| osNameVersion, | ||
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | ||
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | ||
| } |
There was a problem hiding this comment.
The flushPayload includes fetchHeaders, but flush-worker.js also defines its own DEFAULT_HEADERS and merges them again. This means Content-Type is always set twice (harmlessly), but the round-trip serialization of headers is redundant if the worker already applies defaults. More importantly, if fetchHeaders is mutated between spawning and the worker parsing (unlikely but possible in tests), behavior could be surprising. Consider only passing extra/override headers and letting the worker own DEFAULT_HEADERS entirely.
| nodeVersion: process.version, | |
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| const flushPayload = JSON.stringify({ | |
| body: fetchConfig.body, | |
| postUrl, | |
| headers: Object.fromEntries( | |
| Object.entries(fetchHeaders).filter(([k]) => k.toLowerCase() !== 'content-type') | |
| ) | |
| }) |
| async function trackEvent (eventType, rawEventData = {}) { | ||
| // prerunEvent will be null when telemetry-prompt event fires, this happens before | ||
| // any command is actually run, so we want to ignore the command+flags in this case |
There was a problem hiding this comment.
The AIO_TELEMETRY_DISABLED check uses a truthy JS check on the env string, meaning AIO_TELEMETRY_DISABLED=0 or AIO_TELEMETRY_DISABLED=false would still disable telemetry, which contradicts the README's claim of 'any truthy value'. This is inconsistent — env vars are always strings, so '0' and 'false' are truthy in JS. Either document this clearly or normalize the check.
| async function trackEvent (eventType, rawEventData = {}) { | |
| // prerunEvent will be null when telemetry-prompt event fires, this happens before | |
| // any command is actually run, so we want to ignore the command+flags in this case | |
| const isEnvDisabled = process.env.AIO_TELEMETRY_DISABLED !== undefined && | |
| process.env.AIO_TELEMETRY_DISABLED !== '' && | |
| process.env.AIO_TELEMETRY_DISABLED !== '0' && | |
| process.env.AIO_TELEMETRY_DISABLED.toLowerCase() !== 'false' | |
| const optedOut = isDisabledForCommand || isEnvDisabled || config.get(`${configKey}.optOut`, 'global') === true |
There was a problem hiding this comment.
🤖 PR Reviewer
This is a substantial, well-structured refactor: replacing a direct HTTP call with a fire-and-forget detached subprocess, adding a persistent retry queue, and introducing agent-detection telemetry. Code quality is good overall, tests are comprehensive, and the architecture is sound. Three previously raised issues remain unaddressed (header blocklist, queue size cap, and the AIO_TELEMETRY_DISABLED truthy-string semantics), and there is one new issue with an argument injection risk when passing the flush payload as a CLI argument.
📝 4 suggestion(s) (1 new, 3 re-raised)
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| const safe = Object.fromEntries( | ||
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | ||
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } |
There was a problem hiding this comment.
[Re-raised] The filter strips only 'api-key'. Common credential headers like 'authorization', 'x-api-key', and 'x-ingest-key' are not blocked. Expand the blocklist.
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } |
| /** | ||
| * Persists the given metrics array to the queue file, creating directories as needed. | ||
| * Silently ignores write errors — telemetry must never crash the CLI. | ||
| * @param {Array<object>} items Flat array of New Relic metric objects. |
There was a problem hiding this comment.
[Re-raised] There is no cap on the queue size. If the endpoint is unreachable for an extended period the file will grow unboundedly. Cap at a reasonable maximum.
| * @param {Array<object>} items Flat array of New Relic metric objects. | |
| function writeQueue (items) { | |
| const MAX_QUEUE_SIZE = 100 | |
| const file = getQueuePath() | |
| try { | |
| fs.mkdirSync(path.dirname(file), { recursive: true }) | |
| const capped = items.slice(-MAX_QUEUE_SIZE) | |
| fs.writeFileSync(file, JSON.stringify(capped), 'utf8') | |
| } catch { | |
| // silently ignore — failing to persist the queue must not affect the CLI | |
| } | |
| } |
| osNameVersion | ||
| } | ||
| }) | ||
| body: JSON.stringify([{ |
There was a problem hiding this comment.
[Re-raised] AIO_TELEMETRY_DISABLED uses a JS truthy check on a string. The strings '0' and 'false' are truthy in JS, so they would incorrectly disable telemetry. Normalize the check to match the documented behaviour.
| body: JSON.stringify([{ | |
| const envDisabled = process.env.AIO_TELEMETRY_DISABLED !== undefined && | |
| process.env.AIO_TELEMETRY_DISABLED !== '' && | |
| process.env.AIO_TELEMETRY_DISABLED !== '0' && | |
| process.env.AIO_TELEMETRY_DISABLED.toLowerCase() !== 'false' | |
| const optedOut = isDisabledForCommand || envDisabled || config.get(`${configKey}.optOut`, 'global') === true |
| osNameVersion, | ||
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | ||
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | ||
| } | ||
| }] | ||
| }]) | ||
| } | ||
| const flushPayload = JSON.stringify({ | ||
| body: fetchConfig.body, |
There was a problem hiding this comment.
The entire JSON-serialized flush payload is passed as a single CLI argument (process.argv[2]). On Windows, argument length is capped around 32 767 characters and can silently truncate or fail. For large queues or long eventData strings this will cause data loss. Consider writing the payload to a temp file and passing the file path instead, then deleting it in the worker after parsing.
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| }] | |
| }]) | |
| } | |
| const flushPayload = JSON.stringify({ | |
| body: fetchConfig.body, | |
| const tmpFile = path.join(os.tmpdir(), `aio-telemetry-${Date.now()}-${Math.random().toString(36).slice(2)}.json`) | |
| try { | |
| require('fs').writeFileSync(tmpFile, flushPayload, 'utf8') | |
| } catch (err) { | |
| debug('Failed to write telemetry payload to temp file: %O', err) | |
| return | |
| } | |
| try { | |
| const child = spawn(process.execPath, [path.join(__dirname, 'flush-worker.js'), tmpFile], { | |
| env: { ...process.env, AIO_TELEMETRY_DISABLED: '1' }, | |
| detached: true, | |
| stdio: 'ignore' | |
| }) | |
| child.unref() | |
| } catch (err) { | |
| debug('Failed to launch telemetry flush worker: %O', err) | |
| } |
…ut only 1 server call
There was a problem hiding this comment.
🤖 PR Reviewer
The refactor is well-structured and the test coverage is thorough. Three issues from the previous review remain unaddressed (header blocklist too narrow, unbounded queue, AIO_TELEMETRY_DISABLED truthy-string bug), and the Windows argument-length concern is still present. One new minor issue exists: fetchHeaders is re-initialized to a bare Content-Type object in init, so any remoteConf.fetchHeaders passed by the host CLI is silently ignored despite being documented in the README.
📝 5 suggestion(s) (1 new, 4 re-raised)
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
⚠️ Inline comments could not be attached (lines not in diff). See summary above.
| init: (versionString, binName, remoteConf = {}) => { | ||
| global.commandHookStartTime = Date.now() | ||
| rootCliVersion = versionString | ||
| if (remoteConf.fetchHeaders) { | ||
| fetchHeaders = remoteConf.fetchHeaders | ||
| } | ||
| if (remoteConf.postUrl) { | ||
| postUrl = remoteConf.postUrl | ||
| postUrl = remoteConf.postUrl || process.env.AIO_TELEMETRY_POST_URL || DEFAULT_TELEMETRY_POST_URL | ||
| fetchHeaders = { | ||
| 'Content-Type': 'application/json' | ||
| } |
| // TODO: detect VSCODE run as an agent | ||
| /** | ||
| * Environment variables checked for agent detection (proposed standard first, then tool-specific). | ||
| * Used for metrics only. See aio-cli README "Agent detection" for full list. | ||
| */ | ||
| const AGENT_ENV_VARS = [ | ||
| { env: 'AGENT', name: (v) => (v && v !== '1' && v !== 'true' ? String(v).toLowerCase() : 'generic') }, | ||
| { env: 'AI_AGENT', name: (v) => (v && v !== '1' && v !== 'true' ? String(v).toLowerCase() : 'generic') }, |
| if (headers && typeof headers === 'object') { | ||
| const safe = Object.fromEntries( | ||
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | ||
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | ||
| } | ||
| currentMetrics = JSON.parse(body)[0].metrics | ||
| } catch { | ||
| // Malformed argument — nothing useful to do. | ||
| return | ||
| } | ||
|
|
||
| // Merge previously-queued metrics (if any) with the current event so they | ||
| // are all retried in a single POST. | ||
| const queuedMetrics = readQueue() | ||
| const allMetrics = [...queuedMetrics, ...currentMetrics] | ||
|
|
| /** | ||
| * Appends metric objects to the end of the queue (read + merge + write). | ||
| * Silently ignores invalid input or write errors. | ||
| * @param {Array<object>} newItems Flat array of New Relic metric objects. | ||
| */ | ||
| function appendToQueue (newItems) { | ||
| if (!Array.isArray(newItems) || newItems.length === 0) { | ||
| return | ||
| } | ||
| const existing = readQueue() | ||
| writeQueue([...existing, ...newItems]) | ||
| } |
| - `fetchHeaders`: Optional extra headers merged into telemetry requests (`Content-Type` is always set) | ||
| - `productPrivacyPolicyLink`: A link to display to users when prompting to opt in | ||
| - `productName`: How to refer to the CLI when the user is prompted to enable telemetry (from `displayName` or `name` in `package.json`) | ||
| - `productBin`: Shown in help text (from `bin` in `package.json`; if several bins exist, the first is used). Example: run `${productBin} telemetry on` | ||
|
|
There was a problem hiding this comment.
🤖 PR Reviewer
This is a well-structured refactor that replaces a direct Splunk/AEP HTTP POST with a fire-and-forget detached flush worker, adds a persistent retry queue, and introduces agent detection. The code is clean, well-documented, and has comprehensive test coverage. A few minor issues around security, correctness, and robustness are noted below.
📝 6 suggestion(s) - Please review inline comments below.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| const { body, postUrl: url, headers } = parsed | ||
| if (!url || typeof url !== 'string') { | ||
| return | ||
| } | ||
| postUrl = url | ||
| if (headers && typeof headers === 'object') { |
There was a problem hiding this comment.
The header filter only blocks 'api-key' (case-insensitively), but callers could inadvertently pass 'authorization', 'x-api-key', or other sensitive headers. Since the parent already controls fetchHeaders and strips secrets before spawning, consider either allowlisting known safe headers or at minimum blocking a broader set of sensitive header names.
| const { body, postUrl: url, headers } = parsed | |
| if (!url || typeof url !== 'string') { | |
| return | |
| } | |
| postUrl = url | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } |
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | ||
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } |
There was a problem hiding this comment.
If body is valid JSON but does not contain an array at index 0, or if metrics is missing, this will throw a TypeError that is silently swallowed. Add a guard to avoid misleading silent failures.
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| const parsed = JSON.parse(process.argv[2]) | |
| const { body, postUrl: url, headers } = parsed | |
| if (!url || typeof url !== 'string') { | |
| return | |
| } | |
| postUrl = url | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody) || !parsedBody[0] || !Array.isArray(parsedBody[0].metrics)) { | |
| return | |
| } | |
| currentMetrics = parsedBody[0].metrics |
| fs.mkdirSync(path.dirname(file), { recursive: true }) | ||
| fs.writeFileSync(file, JSON.stringify(items), 'utf8') | ||
| } catch { | ||
| // silently ignore — failing to persist the queue must not affect the CLI |
There was a problem hiding this comment.
The queue file can grow unboundedly if the telemetry endpoint is unreachable for a long time (e.g. offline laptop). Consider capping the number of retained metrics to prevent disk/memory issues on the next flush.
| fs.mkdirSync(path.dirname(file), { recursive: true }) | |
| fs.writeFileSync(file, JSON.stringify(items), 'utf8') | |
| } catch { | |
| // silently ignore — failing to persist the queue must not affect the CLI | |
| function readQueue () { | |
| const MAX_QUEUE_SIZE = 500 | |
| try { | |
| const data = fs.readFileSync(getQueuePath(), 'utf8') | |
| const parsed = JSON.parse(data) | |
| if (!Array.isArray(parsed)) return [] | |
| return parsed.slice(-MAX_QUEUE_SIZE) | |
| } catch { | |
| return [] | |
| } | |
| } |
| debug('trackEvent %s eventData=%j postUrl=%s willSend=%s', eventType, eventData, postUrl, willSend) | ||
|
|
||
| if (optedOut) { |
There was a problem hiding this comment.
process.env.AIO_TELEMETRY_DISABLED is truthy for any non-empty string (including '0' or 'false'). This is documented intentionally, but it may surprise operators. At minimum, add a comment clarifying this is intentional and consistent with standard CLI env-var conventions.
| debug('trackEvent %s eventData=%j postUrl=%s willSend=%s', eventType, eventData, postUrl, willSend) | |
| if (optedOut) { | |
| // AIO_TELEMETRY_DISABLED: any non-empty string disables telemetry (including '0'/'false'). | |
| // This is intentional — operators should set it to '1' to disable, and unset/empty to enable. | |
| const optedOut = isDisabledForCommand || process.env.AIO_TELEMETRY_DISABLED || config.get(`${configKey}.optOut`, 'global') === true |
| }] | ||
| }]) | ||
| } | ||
| const metrics = JSON.parse(fetchConfig.body)[0].metrics | ||
|
|
||
| if (eventType !== 'postrun') { | ||
| // Queue until postrun so only one detached flush worker runs per command (avoids concurrent merges). | ||
| queueStore.appendToQueue(metrics) | ||
| return | ||
| } | ||
|
|
||
| const flushPayload = JSON.stringify({ |
There was a problem hiding this comment.
The flush payload passes fetchHeaders (which includes user-supplied headers) as a CLI argument string. If fetchHeaders contains values with characters that are significant in shell contexts (quotes, newlines), and the spawned process is ever launched via shell:true, this could be a command injection vector. The current spawn uses shell:false (default) so it is safe today, but it is worth adding an explicit comment and ensuring shell is never set to true.
| }] | |
| }]) | |
| } | |
| const metrics = JSON.parse(fetchConfig.body)[0].metrics | |
| if (eventType !== 'postrun') { | |
| // Queue until postrun so only one detached flush worker runs per command (avoids concurrent merges). | |
| queueStore.appendToQueue(metrics) | |
| return | |
| } | |
| const flushPayload = JSON.stringify({ | |
| const child = spawn(process.execPath, [path.join(__dirname, 'flush-worker.js'), flushPayload], { | |
| // shell MUST remain false (the default) — flushPayload is passed as a raw argument | |
| // to avoid any shell interpolation of header values or URLs. | |
| env: { ...process.env, AIO_TELEMETRY_DISABLED: '1' }, | |
| detached: true, | |
| stdio: 'ignore' | |
| }) |
There was a problem hiding this comment.
🤖 PR Reviewer
The diff is a substantial refactor introducing a flush-worker subprocess, persistent queue store, agent detection, and improved telemetry payload format. Most of the code is well-structured and tested. However, several previously-raised issues remain unaddressed (syntax error in init, flush worker inheriting full env, inconsistent wire format, redundant JSON round-trip, narrow header blocklist in flush-worker, missing guard in flush-worker body parse, missing typeof guard in detectCopilotAgent, redundant mkdirSync in readQueue shrink path), plus the tests assert the old batches wrapper format which will fail against the suggested fix.
📝 10 suggestion(s) (2 new, 8 re-raised)
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| Object.entries(rawExtra).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | ||
| extraFetchHeaders = Object.fromEntries( | ||
| Object.entries(rawExtra).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | ||
| ) | ||
| configKey = binName + '-cli-telemetry' | ||
| }, | ||
| getClientId, |
There was a problem hiding this comment.
[Re-raised] Syntax error: duplicate/orphaned Object.fromEntries assignment — the closing parenthesis of the first call is missing, then the entire expression is repeated. This is a parse error that will prevent the module from loading.
| Object.entries(rawExtra).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| extraFetchHeaders = Object.fromEntries( | |
| Object.entries(rawExtra).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| configKey = binName + '-cli-telemetry' | |
| }, | |
| getClientId, | |
| extraFetchHeaders = Object.fromEntries( | |
| Object.entries(rawExtra).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| fetchHeaders = { ...DEFAULT_FETCH_HEADERS, ...extraFetchHeaders } | |
| configKey = binName + '-cli-telemetry' |
| headers: fetchHeaders, | ||
| body: JSON.stringify({ | ||
| id: Math.floor(timestamp * Math.random()), | ||
| timestamp, | ||
| _adobeio: { | ||
| eventType, | ||
| eventData, | ||
| cliVersion: rootCliVersion, | ||
| clientId, | ||
| command: prerunEvent.command, | ||
| commandDuration: timestamp - prerunEvent.start, | ||
| commandFlags: prerunEvent.flags.toString(), | ||
| commandSuccess: eventType !== 'command-error', | ||
| nodeVersion: process.version, | ||
| osNameVersion | ||
| } | ||
| }) | ||
| body: JSON.stringify([{ | ||
| metrics: [{ |
There was a problem hiding this comment.
[Re-raised] The line const metrics = JSON.parse(fetchConfig.body)[0].metrics re-parses JSON that was just serialized. Build the metrics array first and reference it directly.
| headers: fetchHeaders, | |
| body: JSON.stringify({ | |
| id: Math.floor(timestamp * Math.random()), | |
| timestamp, | |
| _adobeio: { | |
| eventType, | |
| eventData, | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion | |
| } | |
| }) | |
| body: JSON.stringify([{ | |
| metrics: [{ | |
| const metricsArray = [{ | |
| name: 'aio.cli.telemetry', | |
| type: 'gauge', | |
| value: 1, | |
| timestamp, | |
| attributes: { | |
| eventType, | |
| eventData: formatEventDataAttribute(eventData), | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| }] | |
| const fetchConfig = { | |
| method: 'POST', | |
| headers: fetchHeaders, | |
| body: JSON.stringify([{ metrics: metricsArray }]) | |
| } | |
| const metrics = metricsArray |
| nodeVersion: process.version, | ||
| osNameVersion, | ||
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | ||
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | ||
| } | ||
| }] | ||
| }]) | ||
| } | ||
| const metrics = JSON.parse(fetchConfig.body)[0].metrics |
There was a problem hiding this comment.
[Re-raised] The flush worker subprocess inherits the full process.env including potentially sensitive variables. Pass only a minimal environment to the worker.
| nodeVersion: process.version, | |
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| }] | |
| }]) | |
| } | |
| const metrics = JSON.parse(fetchConfig.body)[0].metrics | |
| const child = spawn(process.execPath, [path.join(__dirname, 'flush-worker.js'), flushPayload], { | |
| env: { | |
| PATH: process.env.PATH, | |
| HOME: process.env.HOME, | |
| XDG_CONFIG_HOME: process.env.XDG_CONFIG_HOME, | |
| NODE_PATH: process.env.NODE_PATH, | |
| AIO_TELEMETRY_DISABLED: '1' | |
| }, | |
| detached: true, | |
| stdio: 'ignore' | |
| }) |
| function detectCopilotAgent (pathValue) { | ||
| if (!pathValue) return null | ||
| // Extension id appears in globalStorage paths on all platforms; do not tie to '/' (Windows uses '\'). | ||
| if (pathValue.includes('github.copilot-chat')) { | ||
| return 'github-copilot' |
There was a problem hiding this comment.
[Re-raised] detectCopilotAgent does not guard against non-string values. If PATH is set to a non-string (e.g. null, a number injected via a test), pathValue.includes will throw a TypeError.
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue) return null | |
| // Extension id appears in globalStorage paths on all platforms; do not tie to '/' (Windows uses '\'). | |
| if (pathValue.includes('github.copilot-chat')) { | |
| return 'github-copilot' | |
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue || typeof pathValue !== 'string') return null | |
| if (pathValue.includes('github.copilot-chat')) { | |
| return 'github-copilot' | |
| } | |
| return null | |
| } |
| } | ||
| postUrl = url | ||
| if (headers && typeof headers === 'object') { | ||
| const safe = Object.fromEntries( | ||
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') |
There was a problem hiding this comment.
[Re-raised] The header filter only blocks 'api-key'. Broaden to cover 'authorization', 'x-api-key', and 'x-ingest-key' to be consistent with telemetry-lib.js.
| } | |
| postUrl = url | |
| if (headers && typeof headers === 'object') { | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } |
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } |
There was a problem hiding this comment.
[Re-raised] If the body parses but does not have an array at index 0, or metrics is absent/non-array, this throws a TypeError silently swallowed by the outer catch. Add an explicit guard.
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody) || !parsedBody[0] || !Array.isArray(parsedBody[0].metrics)) { | |
| return | |
| } | |
| currentMetrics = parsedBody[0].metrics |
| try { | ||
| debug('POST %s requestHeaders=%o', postUrl, requestHeaders) | ||
| const res = await fetch(postUrl, { | ||
| method: 'POST', | ||
| headers: requestHeaders, | ||
| body: JSON.stringify({ batches: [{ metrics: allMetrics }] }) |
There was a problem hiding this comment.
[Re-raised] The flush-worker POSTs with body shape { batches: [{ metrics: allMetrics }] } but telemetry-lib.js serializes the body as [{ metrics: [...] }] (a top-level array). Both sides must agree on the wire format. Change the worker to use the same top-level array format to ensure the proxy receives consistent payloads.
| try { | |
| debug('POST %s requestHeaders=%o', postUrl, requestHeaders) | |
| const res = await fetch(postUrl, { | |
| method: 'POST', | |
| headers: requestHeaders, | |
| body: JSON.stringify({ batches: [{ metrics: allMetrics }] }) | |
| const res = await fetch(postUrl, { | |
| method: 'POST', | |
| headers: requestHeaders, | |
| body: JSON.stringify([{ metrics: allMetrics }]) | |
| }) |
| * Persists the given metrics array to the queue file, creating directories as needed. | ||
| * Silently ignores write errors — telemetry must never crash the CLI. | ||
| * @param {Array<object>} items Flat array of New Relic metric objects. | ||
| */ | ||
| function writeQueue (items) { | ||
| const file = getQueuePath() | ||
| const toWrite = normalizeQueueItems(items) | ||
| try { |
There was a problem hiding this comment.
[Re-raised] The mkdirSync call inside the shrink-rewrite path in readQueue is unnecessary — the file already exists at this point (readFileSync succeeded). Remove it to avoid the redundant directory creation.
| * Persists the given metrics array to the queue file, creating directories as needed. | |
| * Silently ignores write errors — telemetry must never crash the CLI. | |
| * @param {Array<object>} items Flat array of New Relic metric objects. | |
| */ | |
| function writeQueue (items) { | |
| const file = getQueuePath() | |
| const toWrite = normalizeQueueItems(items) | |
| try { | |
| if (parsed.length > normalized.length) { | |
| try { | |
| fs.writeFileSync(file, JSON.stringify(normalized), 'utf8') | |
| } catch { | |
| // same as writeQueue — never throw from telemetry persistence | |
| } | |
| } | |
| return normalized |
| expect(writeQueue).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| test('writes merged metrics to queue on fetch failure', async () => { |
There was a problem hiding this comment.
Once the wire format inconsistency in flush-worker.js is fixed (top-level array instead of batches wrapper), these assertions need to be updated to match the corrected format.
| expect(writeQueue).not.toHaveBeenCalled() | |
| }) | |
| test('writes merged metrics to queue on fetch failure', async () => { | |
| const posted = JSON.parse(opts.body) | |
| expect(posted[0].metrics).toHaveLength(2) | |
| expect(posted[0].metrics[0]).toEqual(queued[0]) | |
| expect(posted[0].metrics[1]).toEqual(METRIC) |
| await main() | ||
|
|
||
| expect(writeQueue).toHaveBeenCalledTimes(1) | ||
| expect(writeQueue).toHaveBeenCalledWith([METRIC]) |
There was a problem hiding this comment.
Update assertion to match corrected top-level array wire format after fixing the batches wrapper inconsistency.
| await main() | |
| expect(writeQueue).toHaveBeenCalledTimes(1) | |
| expect(writeQueue).toHaveBeenCalledWith([METRIC]) | |
| const posted = JSON.parse(fetch.mock.calls[0][1].body) | |
| expect(posted[0].metrics).toHaveLength(1) | |
| expect(posted[0].metrics[0]).toEqual(METRIC) | |
| expect(clearQueue).toHaveBeenCalledTimes(1) |
There was a problem hiding this comment.
🤖 PR Reviewer
The diff addresses the architectural refactor from direct HTTP POST to a detached flush-worker subprocess, adds agent detection, and improves documentation. Most previous re-raised issues remain unaddressed: flush-worker.js still uses the { batches: [...] } wrapper instead of the top-level array format that telemetry-lib.js produces, tests still assert the batches shape, the blocked-header set in flush-worker.js is still too narrow, and the detectCopilotAgent guard still doesn't check typeof, and the body is still re-parsed from JSON right after serialization. Several new minor issues are also present.
📝 8 suggestion(s) (2 new, 6 re-raised)
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| return | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
[Re-raised] The flush-worker POSTs with body shape { batches: [...] } but telemetry-lib.js serializes as [{ metrics: [...] }] (a top-level array). The wire format is inconsistent and will cause the proxy to receive malformed payloads. Change to pass the parsed array directly as the body, matching the format telemetry-lib produces.
| return | |
| } | |
| try { | |
| const res = await fetch(postUrl, { | |
| method: 'POST', | |
| headers: requestHeaders, | |
| body: JSON.stringify(batches) | |
| }) |
| const { body, postUrl: url, headers } = parsed | ||
| if (!url || typeof url !== 'string') { | ||
| return | ||
| } | ||
| postUrl = url |
There was a problem hiding this comment.
[Re-raised] The header filter only blocks 'api-key'. Broaden to cover 'authorization', 'x-api-key', and 'x-ingest-key' to be consistent with telemetry-lib.js.
| const { body, postUrl: url, headers } = parsed | |
| if (!url || typeof url !== 'string') { | |
| return | |
| } | |
| postUrl = url | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } |
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | ||
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | ||
| } | ||
| const parsedBody = JSON.parse(body) | ||
| if (!Array.isArray(parsedBody)) { | ||
| return |
There was a problem hiding this comment.
[Re-raised] If the body parses but does not have an array at index 0, or metrics is absent/non-array, the batches variable will hold an unexpected shape, causing silent failures or incorrect POSTs. Add an explicit guard.
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody)) { | |
| return | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody) || !parsedBody[0] || !Array.isArray(parsedBody[0].metrics)) { | |
| return | |
| } | |
| batches = parsedBody |
| * | ||
| * @param {string|null|undefined} [pathValue] - PATH environment variable value. | ||
| * @returns {string|null} Agent name when detected, otherwise null. | ||
| */ | ||
| function detectCopilotAgent (pathValue) { | ||
| if (!pathValue) return null |
There was a problem hiding this comment.
[Re-raised] detectCopilotAgent does not guard against non-string values. If PATH is set to null or a non-string (e.g. injected via a test), pathValue.includes will throw a TypeError. Add a typeof guard.
| * | |
| * @param {string|null|undefined} [pathValue] - PATH environment variable value. | |
| * @returns {string|null} Agent name when detected, otherwise null. | |
| */ | |
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue) return null | |
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue || typeof pathValue !== 'string') return null | |
| if (pathValue.includes('github.copilot-chat')) { | |
| return 'github-copilot' | |
| } | |
| return null | |
| } |
| commandSuccess: eventType !== 'command-error', | ||
| nodeVersion: process.version, | ||
| osNameVersion, | ||
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | ||
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' |
There was a problem hiding this comment.
[Re-raised] The line const batch = JSON.parse(fetchConfig.body) re-parses JSON that was just serialized two lines above. Build the metrics array first and reference it directly, avoiding the redundant serialization/deserialization round-trip.
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| const metricsThisEvent = [{ | |
| name: 'aio.cli.telemetry', | |
| type: 'gauge', | |
| value: 1, | |
| timestamp, | |
| attributes: { | |
| eventType, | |
| eventData: formatEventDataAttribute(eventData), | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| }] |
| if (optedOut) { | ||
| pendingCommandMetrics.length = 0 | ||
| debug('Telemetry is off. Not logging telemetry event', eventType) | ||
| } else { | ||
| const clientId = getClientId() | ||
| const timestamp = Date.now() | ||
| const invocationContext = getInvocationContext() | ||
| const fetchConfig = { | ||
| method: 'POST', | ||
| headers: fetchHeaders, | ||
| body: JSON.stringify({ | ||
| id: Math.floor(timestamp * Math.random()), | ||
| timestamp, | ||
| _adobeio: { | ||
| eventType, | ||
| eventData, | ||
| cliVersion: rootCliVersion, | ||
| clientId, | ||
| command: prerunEvent.command, | ||
| commandDuration: timestamp - prerunEvent.start, | ||
| commandFlags: prerunEvent.flags.toString(), | ||
| commandSuccess: eventType !== 'command-error', | ||
| nodeVersion: process.version, | ||
| osNameVersion | ||
| } | ||
| }) | ||
| body: JSON.stringify([{ | ||
| metrics: [{ | ||
| name: 'aio.cli.telemetry', | ||
| type: 'gauge', | ||
| value: 1, | ||
| // id: Math.floor(timestamp * Math.random()), | ||
| timestamp, | ||
| attributes: { | ||
| eventType, | ||
| eventData: formatEventDataAttribute(eventData), | ||
| cliVersion: rootCliVersion, | ||
| clientId, | ||
| command: prerunEvent.command, | ||
| commandDuration: timestamp - prerunEvent.start, | ||
| commandFlags: prerunEvent.flags.toString(), |
There was a problem hiding this comment.
The fetchConfig object is only used to serialize the body, then immediately re-parsed. After applying the suggestion to build metricsThisEvent directly, this entire fetchConfig block can be removed.
| if (optedOut) { | |
| pendingCommandMetrics.length = 0 | |
| debug('Telemetry is off. Not logging telemetry event', eventType) | |
| } else { | |
| const clientId = getClientId() | |
| const timestamp = Date.now() | |
| const invocationContext = getInvocationContext() | |
| const fetchConfig = { | |
| method: 'POST', | |
| headers: fetchHeaders, | |
| body: JSON.stringify({ | |
| id: Math.floor(timestamp * Math.random()), | |
| timestamp, | |
| _adobeio: { | |
| eventType, | |
| eventData, | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion | |
| } | |
| }) | |
| body: JSON.stringify([{ | |
| metrics: [{ | |
| name: 'aio.cli.telemetry', | |
| type: 'gauge', | |
| value: 1, | |
| // id: Math.floor(timestamp * Math.random()), | |
| timestamp, | |
| attributes: { | |
| eventType, | |
| eventData: formatEventDataAttribute(eventData), | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| const timestamp = Date.now() | |
| const invocationContext = getInvocationContext() |
| }) | ||
|
|
||
| test('does not throw when fetch rejects', async () => { | ||
| fetch.mockRejectedValue(new Error('network error')) | ||
|
|
There was a problem hiding this comment.
[Re-raised] Once the wire format inconsistency in flush-worker.js is fixed (top-level array instead of batches wrapper), these assertions need to be updated to match the corrected format.
| }) | |
| test('does not throw when fetch rejects', async () => { | |
| fetch.mockRejectedValue(new Error('network error')) | |
| const posted = JSON.parse(opts.body) | |
| expect(Array.isArray(posted)).toBe(true) | |
| expect(posted[0].metrics).toHaveLength(1) | |
| expect(posted[0].metrics[0]).toEqual(METRIC) |
There was a problem hiding this comment.
🤖 PR Reviewer
Most previously raised issues remain unresolved. The wire-format mismatch ({ batches } wrapper vs top-level array) still exists in both flush-worker.js and the test assertions. The header blocklist in flush-worker.js still only blocks 'api-key'. The parsedBody[0].metrics array guard is absent. The redundant JSON serialize/parse round-trip in telemetry-lib.js is still present. The spawn env still inherits all of process.env including secrets. The detectCopilotAgent non-string guard issue is partially addressed (falsy check added) but a non-string truthy value (e.g. a number or object) would still reach .includes and throw.
🔄 8 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| const parsedBody = JSON.parse(body) | ||
| if (!Array.isArray(parsedBody)) { | ||
| return | ||
| } |
There was a problem hiding this comment.
[Re-raised] Wire format mismatch: the body is a top-level array (as produced by telemetry-lib.js) but is re-wrapped in { batches } here. The proxy will receive a different shape than what telemetry-lib serializes.
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody)) { | |
| return | |
| } | |
| const res = await fetch(postUrl, { | |
| method: 'POST', | |
| headers: requestHeaders, | |
| body: JSON.stringify(batches) | |
| }) |
| if (!url || typeof url !== 'string') { | ||
| return | ||
| } | ||
| postUrl = url | ||
| if (headers && typeof headers === 'object') { | ||
| const safe = Object.fromEntries( |
There was a problem hiding this comment.
[Re-raised] Header filter only blocks 'api-key'. Broaden to cover 'authorization', 'x-api-key', and 'x-ingest-key' for consistency with telemetry-lib.js.
| if (!url || typeof url !== 'string') { | |
| return | |
| } | |
| postUrl = url | |
| if (headers && typeof headers === 'object') { | |
| const safe = Object.fromEntries( | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } |
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | ||
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | ||
| } | ||
| const parsedBody = JSON.parse(body) |
There was a problem hiding this comment.
[Re-raised] Missing guard: if parsedBody[0] exists but lacks a metrics array the worker silently sends a malformed payload. Add an explicit check.
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } | |
| const parsedBody = JSON.parse(body) | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody) || !parsedBody[0] || !Array.isArray(parsedBody[0].metrics)) { | |
| return | |
| } | |
| batches = parsedBody |
| debug('trackEvent %s eventData=%j postUrl=%s willSend=%s', eventType, eventData, postUrl, willSend) | ||
|
|
||
| if (isDisabledForCommand || config.get(`${configKey}.optOut`, 'global') === true) { | ||
| if (optedOut) { | ||
| pendingCommandMetrics.length = 0 | ||
| debug('Telemetry is off. Not logging telemetry event', eventType) | ||
| } else { | ||
| const clientId = getClientId() | ||
| const timestamp = Date.now() | ||
| const invocationContext = getInvocationContext() | ||
| const fetchConfig = { | ||
| method: 'POST', | ||
| headers: fetchHeaders, | ||
| body: JSON.stringify({ | ||
| id: Math.floor(timestamp * Math.random()), | ||
| timestamp, | ||
| _adobeio: { | ||
| eventType, | ||
| eventData, | ||
| cliVersion: rootCliVersion, | ||
| clientId, | ||
| command: prerunEvent.command, | ||
| commandDuration: timestamp - prerunEvent.start, | ||
| commandFlags: prerunEvent.flags.toString(), | ||
| commandSuccess: eventType !== 'command-error', | ||
| nodeVersion: process.version, | ||
| osNameVersion | ||
| } | ||
| }) | ||
| body: JSON.stringify([{ | ||
| metrics: [{ | ||
| name: 'aio.cli.telemetry', | ||
| type: 'gauge', | ||
| value: 1, | ||
| // id: Math.floor(timestamp * Math.random()), |
There was a problem hiding this comment.
[Re-raised] The fetchConfig body is serialized to JSON and then immediately re-parsed two lines later. Build the metrics array directly to avoid the redundant round-trip.
| debug('trackEvent %s eventData=%j postUrl=%s willSend=%s', eventType, eventData, postUrl, willSend) | |
| if (isDisabledForCommand || config.get(`${configKey}.optOut`, 'global') === true) { | |
| if (optedOut) { | |
| pendingCommandMetrics.length = 0 | |
| debug('Telemetry is off. Not logging telemetry event', eventType) | |
| } else { | |
| const clientId = getClientId() | |
| const timestamp = Date.now() | |
| const invocationContext = getInvocationContext() | |
| const fetchConfig = { | |
| method: 'POST', | |
| headers: fetchHeaders, | |
| body: JSON.stringify({ | |
| id: Math.floor(timestamp * Math.random()), | |
| timestamp, | |
| _adobeio: { | |
| eventType, | |
| eventData, | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion | |
| } | |
| }) | |
| body: JSON.stringify([{ | |
| metrics: [{ | |
| name: 'aio.cli.telemetry', | |
| type: 'gauge', | |
| value: 1, | |
| // id: Math.floor(timestamp * Math.random()), | |
| const timestamp = Date.now() | |
| const invocationContext = getInvocationContext() | |
| const metricsThisEvent = [{ | |
| name: 'aio.cli.telemetry', | |
| type: 'gauge', | |
| value: 1, | |
| timestamp, | |
| attributes: { | |
| eventType, | |
| eventData: formatEventDataAttribute(eventData), | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| }] |
| timestamp, | ||
| attributes: { |
There was a problem hiding this comment.
[Re-raised] These two lines exist only because of the redundant serialize/parse. Remove them after applying the suggestion above.
|
|
||
| process.argv = ['node', 'flush-worker.js', flushArg()] | ||
| await expect(main()).resolves.toBeUndefined() | ||
| expect(fetch).toHaveBeenCalledTimes(1) | ||
| }) |
There was a problem hiding this comment.
[Re-raised] Test assertions still expect the { batches } wrapper shape. Once the wire format is fixed to use a top-level array, update these assertions accordingly.
| process.argv = ['node', 'flush-worker.js', flushArg()] | |
| await expect(main()).resolves.toBeUndefined() | |
| expect(fetch).toHaveBeenCalledTimes(1) | |
| }) | |
| const posted = JSON.parse(opts.body) | |
| expect(Array.isArray(posted)).toBe(true) | |
| expect(posted[0].metrics).toHaveLength(1) | |
| expect(posted[0].metrics[0]).toEqual(METRIC) |
…but the parent never calls fetch
There was a problem hiding this comment.
🤖 PR Reviewer
The re-raised issues from the previous review remain unresolved: the flush worker still wraps the payload in { batches } instead of posting the top-level array, the header filter still only blocks 'api-key', the malformed-payload guard is missing the metrics-array check, detectCopilotAgent lacks a typeof string guard, and the spawned worker still inherits all of process.env. The test assertions also still expect the { batches } wrapper shape. All other new code (agent detection, batching, env-var disable, formatEventDataAttribute, resolveEventData) is well-structured and thoroughly tested.
🔄 6 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| return | ||
| } | ||
| batches = parsedBody | ||
| } catch { |
There was a problem hiding this comment.
[Re-raised] Wire format mismatch: body is a top-level array but is re-wrapped in { batches }. Post the parsed array directly.
| } catch { | |
| body: JSON.stringify(batches) |
| try { | ||
| const parsed = JSON.parse(process.argv[2]) | ||
| const { body, postUrl: url, headers } = parsed | ||
| if (!url || typeof url !== 'string') { |
There was a problem hiding this comment.
[Re-raised] Header filter only blocks 'api-key'. Broaden to cover authorization, x-api-key, and x-ingest-key.
| try { | |
| const parsed = JSON.parse(process.argv[2]) | |
| const { body, postUrl: url, headers } = parsed | |
| if (!url || typeof url !== 'string') { | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } |
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | ||
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } |
There was a problem hiding this comment.
[Re-raised] Missing guard: if parsedBody[0] exists but lacks a metrics array the worker silently sends a malformed payload. Add an explicit check.
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody) || !parsedBody[0] || !Array.isArray(parsedBody[0].metrics)) { | |
| return | |
| } | |
| batches = parsedBody |
| return 'github-copilot' | ||
| } |
There was a problem hiding this comment.
[Re-raised] detectCopilotAgent does not guard against non-string truthy values. A non-string (e.g. number, object) would reach .includes and throw TypeError.
| return 'github-copilot' | |
| } | |
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue || typeof pathValue !== 'string') return null | |
| if (pathValue.includes('github.copilot-chat')) { | |
| return 'github-copilot' | |
| } | |
| return null | |
| } |
| osNameVersion, | ||
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | ||
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | ||
| } | ||
| } | ||
|
|
||
| if (eventType !== 'postrun') { | ||
| pendingCommandMetrics.push(metric) | ||
| return |
There was a problem hiding this comment.
[Re-raised] The spawned worker inherits all of process.env including secrets and tokens. Pass only the minimal env vars needed.
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| } | |
| if (eventType !== 'postrun') { | |
| pendingCommandMetrics.push(metric) | |
| return | |
| const child = spawn(process.execPath, [path.join(__dirname, 'flush-worker.js'), flushPayload], { | |
| env: { | |
| PATH: process.env.PATH, | |
| HOME: process.env.HOME, | |
| XDG_CONFIG_HOME: process.env.XDG_CONFIG_HOME, | |
| NODE_PATH: process.env.NODE_PATH, | |
| AIO_TELEMETRY_DISABLED: '1' | |
| }, | |
| detached: true, | |
| stdio: 'ignore' | |
| }) |
| process.argv = ['node', 'flush-worker.js', flushArg()] | ||
| await expect(main()).resolves.toBeUndefined() | ||
| expect(fetch).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
There was a problem hiding this comment.
[Re-raised] Test assertions expect { batches } wrapper. Once wire format is fixed to top-level array, update accordingly.
| process.argv = ['node', 'flush-worker.js', flushArg()] | |
| await expect(main()).resolves.toBeUndefined() | |
| expect(fetch).toHaveBeenCalledTimes(1) | |
| }) | |
| const posted = JSON.parse(opts.body) | |
| expect(Array.isArray(posted)).toBe(true) | |
| expect(posted[0].metrics).toHaveLength(1) | |
| expect(posted[0].metrics[0]).toEqual(METRIC) |
…A-4597) Repoint DEFAULT_TELEMETRY_POST_URL from the stage namespace (53444-aioclitelemetryproxy-stage) to production (53444-aioclitelemetryproxy), now that the prod proxy is deployed and verified. Override via aioTelemetry.postUrl or AIO_TELEMETRY_POST_URL is unchanged.
Drop the explicit 'To opt out, run ...' line from the telemetry notice (per @purplecabbage on PR #40) so it does not over-nudge opt-out; the privacy link already covers how to opt out. Removes the now-unused binName argument from getNoticeMessage/notice.
feat: make telemetry opt-out with a one-time notice
There was a problem hiding this comment.
🤖 PR Reviewer
The refactor is substantial and well-structured: removing the Splunk/inquirer dependencies, introducing a detached flush worker, buffering non-postrun metrics, and adding agent detection. Tests are comprehensive. Three previously-raised issues remain unresolved: the wire format wraps the parsed array in { batches } instead of posting it directly, the header blocklist only filters api-key, and the flush-worker guard for a missing metrics array is absent. The detectCopilotAgent function also still lacks a typeof string guard, and the spawned worker inherits all of process.env.
🔄 6 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| return | ||
| } | ||
| batches = parsedBody | ||
| } catch { |
There was a problem hiding this comment.
[Re-raised] Wire format mismatch: the parsed array is re-wrapped in { batches }. The proxy expects a bare array (same shape as mergedBody in telemetry-lib). Post it directly.
| } catch { | |
| body: JSON.stringify(batches) |
| try { | ||
| const parsed = JSON.parse(process.argv[2]) | ||
| const { body, postUrl: url, headers } = parsed | ||
| if (!url || typeof url !== 'string') { | ||
| return |
There was a problem hiding this comment.
[Re-raised] Header filter only blocks 'api-key'. Broaden the blocklist to cover common secret header names.
| try { | |
| const parsed = JSON.parse(process.argv[2]) | |
| const { body, postUrl: url, headers } = parsed | |
| if (!url || typeof url !== 'string') { | |
| return | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } |
| if (headers && typeof headers === 'object') { | ||
| const safe = Object.fromEntries( | ||
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | ||
| ) |
There was a problem hiding this comment.
[Re-raised] Missing guard: if the parsed array exists but its first element lacks a metrics array the worker silently sends a malformed payload.
| if (headers && typeof headers === 'object') { | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| ) | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody) || !parsedBody[0] || !Array.isArray(parsedBody[0].metrics)) { | |
| return | |
| } | |
| batches = parsedBody |
| return null | ||
| } | ||
|
|
||
| // TODO: detect VSCODE run as an agent |
There was a problem hiding this comment.
[Re-raised] detectCopilotAgent does not guard against non-string truthy values. A non-string value (e.g. a number injected via a test env object) would reach .includes and throw TypeError.
| return null | |
| } | |
| // TODO: detect VSCODE run as an agent | |
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue || typeof pathValue !== 'string') return null | |
| if (pathValue.includes('github.copilot-chat')) { | |
| return 'github-copilot' | |
| } | |
| return null | |
| } |
| eventType, | ||
| eventData: formatEventDataAttribute(eventData), | ||
| cliVersion: rootCliVersion, | ||
| clientId, | ||
| command: prerunEvent.command, | ||
| commandDuration: timestamp - prerunEvent.start, | ||
| commandFlags: prerunEvent.flags.toString(), |
There was a problem hiding this comment.
[Re-raised] The spawned worker inherits all of process.env, including secrets and tokens present in the CLI environment. Pass only the minimal set of vars the worker actually needs.
| eventType, | |
| eventData: formatEventDataAttribute(eventData), | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| const child = spawn(process.execPath, [path.join(__dirname, 'flush-worker.js'), flushPayload], { | |
| env: { | |
| PATH: process.env.PATH, | |
| HOME: process.env.HOME, | |
| XDG_CONFIG_HOME: process.env.XDG_CONFIG_HOME, | |
| NODE_PATH: process.env.NODE_PATH, | |
| AIO_TELEMETRY_DISABLED: '1' | |
| }, | |
| detached: true, | |
| stdio: 'ignore' | |
| }) |
| expect(posted.batches[0].metrics[0]).toEqual(METRIC) | ||
| }) | ||
|
|
||
| test('does not throw when fetch rejects', async () => { |
There was a problem hiding this comment.
[Re-raised] Test assertions expect the { batches } wrapper. Once the wire format is fixed to post the bare array, update these assertions accordingly.
| expect(posted.batches[0].metrics[0]).toEqual(METRIC) | |
| }) | |
| test('does not throw when fetch rejects', async () => { | |
| const posted = JSON.parse(opts.body) | |
| expect(Array.isArray(posted)).toBe(true) | |
| expect(posted[0].metrics).toHaveLength(1) | |
| expect(posted[0].metrics[0]).toEqual(METRIC) |
| if (eventType !== 'postrun') { | ||
| pendingCommandMetrics.push(metric) |
There was a problem hiding this comment.
Flagged by claude:
in oclif, when a command throws, the command_error hook fires and the process exits — the postrun hook does not run. Same for command_not_found. So under the new design, error/not-found telemetry is buffered and then silently dropped on exit. That's a functional regression: the previous code POSTed each event immediately, so failures were actually captured. With this change, error telemetry (arguably the most valuable signal) effectively disappears.
The tests mask this because they manually call trackEvent('postrun') right after the error hook to force a flush — e.g. test/hooks.test.js "command-error" — which doesn't reflect the real oclif lifecycle where no postrun follows an error.
Suggestion: flush terminal events (command-error, command-not-found) immediately rather than buffering them, or have the command_error hook trigger a flush.
There was a problem hiding this comment.
@pru55e11 I'm working on a fix in a new PR. I will post the link here shortly
The batched-flush design only spawned the flush worker on the postrun hook. But postrun does not run after a command fails or is not found, so those metrics were buffered in memory and silently dropped on exit — error/not-found telemetry was never delivered. Lifecycle (verified against @oclif/core 4.11.4 and @adobe/aio-cli 11.1.0): oclif runs postrun only after command.run() returns; on a throw the error propagates first, and on a not-found oclif throws before postrun. command_error is not an oclif hook — the host CLI fires it from its top-level catch. So no postrun follows an error or not-found. Flush on any terminal event (postrun, command-error, command-not-found), merging buffered non-terminal metrics into the batch. Update the hook tests to exercise the real lifecycle (the error/not-found hook flushes on its own, no manufactured postrun) and the README "Flush architecture" section.
…telemetry A mistyped command fires command-not-found (from oclif) and then command-error (the host CLI rethrowing the same typo, exit 127), recording a typo twice. Track a per-process commandNotFoundFired flag (reset in init) and drop only the single command-error that follows a command-not-found, then clear the flag so a later, unrelated command-error still flushes. Scoped to the typo on purpose: init fires once per process while nested config.runCommand calls re-fire prerun/postrun (e.g. app:undeploy -> app:clean), so a blunt "first terminal event wins" flag would mask real command-errors and misattribute commands. Also clear the flag in trackPrerun so plugin-not-found's "Did you mean ...? Yes" (which runs the suggestion) doesn't mask its errors. Mark command-not-found as commandSuccess: false (a missing command did not succeed). Add tests for the single-typo entry, flag reset, both "Did you mean?" answers, and nested commands.
`eventType` is a New Relic reserved word and is dropped on Metric API ingest, so the attribute was unqueryable in NRQL (every value came back null). Map it to `eventName` at the serialization boundary; the internal `eventType` variable is unchanged. Verified end-to-end: the attribute now lands and is queryable.
…-events fix(telemetry): flush terminal events immediately so error telemetry is delivered
There was a problem hiding this comment.
🤖 PR Reviewer
The re-raised issues are largely still present: the wire format mismatch ({ batches } wrapper vs bare array) persists in both flush-worker.js and the test assertions, the header blocklist is still narrow (only 'api-key'), and the detectCopilotAgent function still lacks a typeof string guard. The environment inheritance concern for spawned workers also remains. The new code is otherwise well-structured with good test coverage and clean architecture.
🔄 6 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| } catch { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
[Re-raised] Wire format mismatch: the parsed array is re-wrapped in { batches }. The proxy expects a bare array (same shape as mergedBody in telemetry-lib). Post it directly.
| body: JSON.stringify(batches) |
| if (!url || typeof url !== 'string') { | ||
| return | ||
| } | ||
| postUrl = url | ||
| if (headers && typeof headers === 'object') { | ||
| const safe = Object.fromEntries( | ||
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') |
There was a problem hiding this comment.
[Re-raised] Header filter only blocks 'api-key'. Broaden the blocklist to cover common secret header names.
| if (!url || typeof url !== 'string') { | |
| return | |
| } | |
| postUrl = url | |
| if (headers && typeof headers === 'object') { | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } |
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | ||
| } | ||
| const parsedBody = JSON.parse(body) | ||
| if (!Array.isArray(parsedBody)) { |
There was a problem hiding this comment.
[Re-raised] Missing guard: if the parsed array exists but its first element lacks a metrics array the worker silently sends a malformed payload.
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody)) { | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody) || !parsedBody[0] || !Array.isArray(parsedBody[0].metrics)) { | |
| return | |
| } | |
| batches = parsedBody |
| // TODO: detect VSCODE run as an agent | ||
| /** | ||
| * Environment variables checked for agent detection (proposed standard first, then tool-specific). | ||
| * Used for metrics only. See aio-cli README "Agent detection" for full list. | ||
| */ | ||
| const AGENT_ENV_VARS = [ |
There was a problem hiding this comment.
[Re-raised] detectCopilotAgent does not guard against non-string truthy values. A non-string value (e.g. a number) would reach .includes and throw TypeError.
| // TODO: detect VSCODE run as an agent | |
| /** | |
| * Environment variables checked for agent detection (proposed standard first, then tool-specific). | |
| * Used for metrics only. See aio-cli README "Agent detection" for full list. | |
| */ | |
| const AGENT_ENV_VARS = [ | |
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue || typeof pathValue !== 'string') return null | |
| if (pathValue.includes('github.copilot-chat')) { | |
| return 'github-copilot' | |
| } | |
| return null | |
| } |
| type: 'gauge', | ||
| value: 1, | ||
| timestamp, | ||
| attributes: { | ||
| // NB: the wire attribute is `eventName`, not `eventType` — `eventType` is a New Relic | ||
| // reserved word and is dropped on Metric API ingest, so it is unqueryable in NRQL. | ||
| eventName: eventType, |
There was a problem hiding this comment.
[Re-raised] The spawned worker inherits all of process.env, including secrets and tokens present in the CLI environment. Pass only the minimal set of vars the worker actually needs.
| type: 'gauge', | |
| value: 1, | |
| timestamp, | |
| attributes: { | |
| // NB: the wire attribute is `eventName`, not `eventType` — `eventType` is a New Relic | |
| // reserved word and is dropped on Metric API ingest, so it is unqueryable in NRQL. | |
| eventName: eventType, | |
| const child = spawn(process.execPath, [path.join(__dirname, 'flush-worker.js'), flushPayload], { | |
| env: { | |
| PATH: process.env.PATH, | |
| HOME: process.env.HOME, | |
| XDG_CONFIG_HOME: process.env.XDG_CONFIG_HOME, | |
| NODE_PATH: process.env.NODE_PATH, | |
| AIO_TELEMETRY_DISABLED: '1' | |
| }, | |
| detached: true, | |
| stdio: 'ignore' | |
| }) |
| test('does not throw when fetch rejects', async () => { | ||
| fetch.mockRejectedValue(new Error('network error')) | ||
|
|
||
| process.argv = ['node', 'flush-worker.js', flushArg()] | ||
| await expect(main()).resolves.toBeUndefined() | ||
| expect(fetch).toHaveBeenCalledTimes(1) | ||
| }) |
There was a problem hiding this comment.
[Re-raised] Test assertions expect the { batches } wrapper. Once the wire format is fixed to post the bare array, update these assertions accordingly.
| test('does not throw when fetch rejects', async () => { | |
| fetch.mockRejectedValue(new Error('network error')) | |
| process.argv = ['node', 'flush-worker.js', flushArg()] | |
| await expect(main()).resolves.toBeUndefined() | |
| expect(fetch).toHaveBeenCalledTimes(1) | |
| }) | |
| const posted = JSON.parse(opts.body) | |
| expect(Array.isArray(posted)).toBe(true) | |
| expect(posted[0].metrics).toHaveLength(1) | |
| expect(posted[0].metrics[0]).toEqual(METRIC) |
Description
This PR updates the telemetry plugin to (1) send metrics via a proxy using a detached “fire-and-forget” flush worker and a persistent on-disk retry queue, and (2) add AI agent vs human invocation detection and include that context in every event.
Changes:
Replace direct HTTP posting from the main process with a detached subprocess flush worker (src/flush-worker.js) and add a persistent retry queue (src/queue-store.js).
Add agent detection via environment variables (plus Copilot PATH markers) and attach invocation_context / agent_name attributes to emitted telemetry.
Update tests and README to reflect the new flush architecture and additional telemetry attributes; remove unused runtime deps.
Related Issue
ACNA-4403ACNA-4403
Motivation and Context
We want to know who, or what is using the cli and how it is being used.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: