Skip to content

Commit c1f7d3b

Browse files
committed
refactor(log-list): address review feedback (#329)
- Add comment explaining ms → ns conversion (timestamp_precise) - Replace .then()/.catch() with async/await IIFE in executeFollowMode - DRY fetchInitial/fetchPoll into single fetch(statsPeriod, afterTimestamp?) callback
1 parent 816fdc1 commit c1f7d3b

File tree

2 files changed

+21
-32
lines changed

2 files changed

+21
-32
lines changed

AGENTS.md

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -659,9 +659,6 @@ mock.module("./some-module", () => ({
659659
<!-- lore:019c9a88-bf99-7322-b192-aafe4636c600 -->
660660
* **getsentry/codecov-action enables JUnit XML test reporting by default**: The \`getsentry/codecov-action@main\` has \`enable-tests: true\` by default, which searches for JUnit XML files matching \`\*\*/\*.junit.xml\`. If the test framework doesn't produce JUnit XML, the action emits 3 warnings on every CI run: "No files found matching pattern", "No JUnit XML files found", and "Please ensure your test framework is generating JUnit XML output". Fix: either set \`enable-tests: false\` in the action inputs, or configure the test runner to output JUnit XML. For Bun, add \`\[test.reporter] junit = "test-results.junit.xml"\` to \`bunfig.toml\` and add \`\*.junit.xml\` to \`.gitignore\`.
661661
662-
<!-- lore:019cb3e6-da66-7534-a573-30d2ecadfd53 -->
663-
* **Returning bare promises loses async function from error stack traces**: When an \`async\` function returns another promise without \`await\`, the calling function disappears from error stack traces if the inner promise rejects. A function that drops \`async\` and does \`return someAsyncCall()\` loses its frame entirely. Fix: keep the function \`async\` and use \`return await someAsyncCall()\`. This matters for debugging — the intermediate function name in the stack trace helps locate which code path triggered the failure. ESLint rule \`no-return-await\` is outdated; modern engines optimize \`return await\` in async functions.
664-
665662
### Pattern
666663
667664
<!-- lore:dbd63348-2049-42b3-bb99-d6a3d64369c7 -->
@@ -680,7 +677,7 @@ mock.module("./some-module", () => ({
680677
* **PR review workflow: reply, resolve, amend, force-push**: PR review workflow: (1) Read unresolved threads via GraphQL, (2) make code changes, (3) run lint+typecheck+tests, (4) create a SEPARATE commit per review round (not amend) for incremental review, (5) push normally, (6) reply to comments via REST API, (7) resolve threads via GraphQL \`resolveReviewThread\`. Only amend+force-push when user explicitly asks or pre-commit hook modified files.
681678
682679
<!-- lore:019cb858-e780-7945-a0ee-a01f85be5585 -->
683-
* **Testing infinite loops (while-true) in streaming functions via AuthError escape hatch**: Follow-mode streaming in \`sentry log list\` uses \`setTimeout\`-based recursive scheduling. \`scheduleNextPoll()\` sets a timer; SIGINT handler calls \`clearTimeout()\` + \`resolve()\`. No \`process.exit(0)\` needed — clearing the timer drains the event loop naturally. \`Bun.sleep()\` cannot be cancelled (no AbortSignal support), so \`setTimeout\`/\`clearTimeout\` is required for cancellable delays. Tests use \`interceptSigint()\` helper that captures \`process.once('SIGINT', handler)\` via spy, then \`trigger()\` invokes it directly. Poll tests use \`Bun.sleep(1200)\` to wait for the real 1-second timer. \`executeFollowMode\<T>(config: FollowConfig\<T>)\` is generic — parameterized by \`fetchInitial\`, \`fetchPoll\`, \`extractNew\` callbacks for both project-scoped and trace-scoped streaming.
680+
* **Testing infinite loops (while-true) in streaming functions via AuthError escape hatch**: Follow-mode streaming in \`sentry log list\` uses \`setTimeout\`-based recursive scheduling. \`scheduleNextPoll()\` sets a timer; SIGINT handler calls \`clearTimeout()\` + \`resolve()\`. No \`process.exit(0)\` needed — clearing the timer drains the event loop naturally. A \`stopped\` boolean guard prevents orphaned poll loops when SIGINT fires during an in-flight fetch (e.g., \`fetchInitial\`). Both \`scheduleNextPoll()\` and \`poll()\` check \`stopped\` before doing work. \`Bun.sleep()\` cannot be cancelled (no AbortSignal support), so \`setTimeout\`/\`clearTimeout\` is required. Tests use \`interceptSigint()\` helper that captures \`process.once('SIGINT', handler)\` via spy, then \`trigger()\` invokes it directly. Poll tests use \`Bun.sleep(1200)\` to wait for the real 1-second timer. \`executeFollowMode\<T>(config: FollowConfig\<T>)\` is generic — parameterized by \`fetchInitial\`, \`fetchPoll\`, \`extractNew\` callbacks.
684681
685682
### Preference
686683

src/commands/log/list.ts

Lines changed: 20 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -184,10 +184,13 @@ type FollowConfig<T extends LogLike> = {
184184
bannerText: string;
185185
/** Whether to show the trace-ID column in table output */
186186
includeTrace: boolean;
187-
/** Fetch the initial batch of logs */
188-
fetchInitial: () => Promise<T[]>;
189-
/** Fetch new logs after the given nanosecond timestamp */
190-
fetchPoll: (lastTimestamp: number) => Promise<T[]>;
187+
/**
188+
* Fetch logs with the given time window.
189+
* @param statsPeriod - Time window (e.g., "1m" for initial, "10m" for polls)
190+
* @param afterTimestamp - Only return logs newer than this (nanoseconds).
191+
* Standard mode passes this for server-side dedup; trace mode ignores it.
192+
*/
193+
fetch: (statsPeriod: string, afterTimestamp?: number) => Promise<T[]>;
191194
/** Extract only the genuinely new entries from a poll response */
192195
extractNew: (logs: T[], lastTimestamp: number) => T[];
193196
};
@@ -221,6 +224,7 @@ function executeFollowMode<T extends LogLike>(
221224
const table = plain ? undefined : createLogStreamingTable();
222225

223226
let headerPrinted = false;
227+
// timestamp_precise is nanoseconds; Date.now() is milliseconds → convert
224228
let lastTimestamp = Date.now() * 1_000_000;
225229
let pendingTimer: ReturnType<typeof setTimeout> | null = null;
226230
let stopped = false;
@@ -272,7 +276,7 @@ function executeFollowMode<T extends LogLike>(
272276
return;
273277
}
274278
try {
275-
const rawLogs = await config.fetchPoll(lastTimestamp);
279+
const rawLogs = await config.fetch("10m", lastTimestamp);
276280
const newLogs = config.extractNew(rawLogs, lastTimestamp);
277281
writeNewLogs(newLogs);
278282
scheduleNextPoll();
@@ -289,9 +293,9 @@ function executeFollowMode<T extends LogLike>(
289293
}
290294
}
291295

292-
config
293-
.fetchInitial()
294-
.then((initialLogs) => {
296+
(async () => {
297+
try {
298+
const initialLogs = await config.fetch("1m");
295299
if (!flags.json && initialLogs.length > 0) {
296300
stdout.write(table ? table.header() : formatLogsHeader());
297301
headerPrinted = true;
@@ -308,11 +312,11 @@ function executeFollowMode<T extends LogLike>(
308312
lastTimestamp = initialLogs[0].timestamp_precise;
309313
}
310314
scheduleNextPoll();
311-
})
312-
.catch((error: unknown) => {
315+
} catch (error) {
313316
process.removeListener("SIGINT", stop);
314317
reject(error);
315-
});
318+
}
319+
})();
316320
});
317321
}
318322

@@ -460,17 +464,11 @@ export const listCommand = buildListCommand("log", {
460464
flags,
461465
bannerText: `Streaming logs for trace ${traceId}...`,
462466
includeTrace: false,
463-
fetchInitial: () =>
464-
listTraceLogs(org, traceId, {
465-
query: flags.query,
466-
limit: flags.limit,
467-
statsPeriod: "1m",
468-
}),
469-
fetchPoll: () =>
467+
fetch: (statsPeriod) =>
470468
listTraceLogs(org, traceId, {
471469
query: flags.query,
472470
limit: flags.limit,
473-
statsPeriod: "10m",
471+
statsPeriod,
474472
}),
475473
extractNew: (logs, lastTs) =>
476474
logs.filter((l) => l.timestamp_precise > lastTs),
@@ -494,18 +492,12 @@ export const listCommand = buildListCommand("log", {
494492
flags,
495493
bannerText: "Streaming logs...",
496494
includeTrace: true,
497-
fetchInitial: () =>
498-
listLogs(org, project, {
499-
query: flags.query,
500-
limit: flags.limit,
501-
statsPeriod: "1m",
502-
}),
503-
fetchPoll: (lastTs) =>
495+
fetch: (statsPeriod, afterTimestamp) =>
504496
listLogs(org, project, {
505497
query: flags.query,
506498
limit: flags.limit,
507-
statsPeriod: "10m",
508-
afterTimestamp: lastTs,
499+
statsPeriod,
500+
afterTimestamp,
509501
}),
510502
extractNew: (logs) => logs,
511503
});

0 commit comments

Comments
 (0)