feat(cli): Add log:list CLI command - #6136
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new log:list command to the CLI, enabling users to retrieve and display logs with filtering options like limit, trace ID, span ID, severity, and pagination. It also updates the telemetry server, log store, and runtime manager to support querying logs by severity text and number, and fixes a minor pluralization issue in the trace:list command. Feedback on the changes suggests handling potential TypeError exceptions in formatBody when stringifying objects with circular references, validating the --limit option to ensure it is a positive integer, and URL-encoding the severityText filter parameter in the runtime manager to prevent query string corruption.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new log:list CLI command to retrieve and filter logs, along with backend support in the telemetry server, file log store, and runtime manager to filter logs by severity. It also fixes a minor pluralization issue in the trace:list command. The review feedback highlights a few key issues: the /api/logs endpoint should extract and pass traceId and spanId query parameters to support standalone span filtering, the runtime manager needs to append spanId to the query when traceId is absent, and the CLI log formatter should safely check the type of log.timestamp to prevent a valid 0 timestamp from being incorrectly treated as falsy.
| const { limit, continuationToken, severityText, severityNumber } = | ||
| request.query; | ||
| response.json( | ||
| await params.logStore.list({ | ||
| limit: limit ? Number.parseInt(limit.toString()) : 100, | ||
| continuationToken: continuationToken | ||
| ? continuationToken.toString() | ||
| : undefined, | ||
| severityText: severityText ? severityText.toString() : undefined, | ||
| severityNumber: severityNumber | ||
| ? Number.parseInt(severityNumber.toString()) | ||
| : undefined, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
The /api/logs endpoint does not extract traceId or spanId from the query parameters. If a user filters logs by spanId without providing a traceId, the filter is silently ignored by the server. Extracting these query parameters and passing them to logStore.list ensures that filtering by spanId alone works correctly.
const { limit, continuationToken, severityText, severityNumber, traceId, spanId } =
request.query;
response.json(
await params.logStore.list({
limit: limit ? Number.parseInt(limit.toString()) : 100,
continuationToken: continuationToken
? continuationToken.toString()
: undefined,
severityText: severityText ? severityText.toString() : undefined,
severityNumber: severityNumber
? Number.parseInt(severityNumber.toString())
: undefined,
traceId: traceId ? traceId.toString() : undefined,
spanId: spanId ? spanId.toString() : undefined,
})
);| if (filter?.severityNumber) { | ||
| if (query !== '') query += '&'; | ||
| query += `severityNumber=${filter.severityNumber}`; | ||
| } |
There was a problem hiding this comment.
When filter.traceId is not present but filter.spanId is, the request is routed to /api/logs. We need to append spanId to the query parameters so that the telemetry server can filter by it.
if (filter?.severityNumber) {
if (query !== '') query += '&';
query += 'severityNumber=' + filter.severityNumber;
}
if (filter?.spanId && !filter?.traceId) {
if (query !== '') query += '&';
query += 'spanId=' + encodeURIComponent(filter.spanId);
}There was a problem hiding this comment.
+1. Can be simplified by treated these independently instead
| let time = 'unknown'; | ||
| if (log.timestamp) { | ||
| time = new Date(log.timestamp).toLocaleString(); | ||
| } |
There was a problem hiding this comment.
Checking log.timestamp as a truthy value can lead to a bug if the timestamp is 0 (which is a valid timestamp representing the epoch). It is safer to check if the type is a number or if it is not null/undefined.
| let time = 'unknown'; | |
| if (log.timestamp) { | |
| time = new Date(log.timestamp).toLocaleString(); | |
| } | |
| let time = 'unknown'; | |
| if (typeof log.timestamp === 'number') { | |
| time = new Date(log.timestamp).toLocaleString(); | |
| } |
| .option('--trace-id <id>', 'filter by trace ID') | ||
| .option('--span-id <id>', 'filter by span ID') |
There was a problem hiding this comment.
What is the behaviour if neither of these are specified? Will the most recent logs be printed? Worth documenting, and also worth mentioning that while traceId and spanId are optional, they are recommended to be provided (at least traceId). Not sure if there's any use case where that is not true....
| const response = await manager.listLogs(listRequest); | ||
|
|
||
| if (!response || !response.logs || response.logs.length === 0) { | ||
| logger.info('No logs found.'); |
There was a problem hiding this comment.
Would this log to stdout? If so that may be distruptive in jsonl mode?
| let time = 'unknown'; | ||
| if (log.timestamp) { | ||
| time = new Date(log.timestamp).toLocaleString(); | ||
| } |
| logger.info( | ||
| `To get the next page, use: --continuation-token ${response.continuationToken}` | ||
| ); | ||
| } else { | ||
| console.log( | ||
| `\nTo get the next page, use: --continuation-token ${response.continuationToken}` | ||
| ); |
There was a problem hiding this comment.
Worth mentioning here and in trace-list that user should use log-get or trace-get to fetch the full log?
| }); | ||
| }); | ||
|
|
||
| it('should output formatted logs without verbose flag', async () => { |
There was a problem hiding this comment.
| it('should output formatted logs without verbose flag', async () => { | |
| it('should output formatted logs without format flag', async () => { |
| if (filter?.severityNumber) { | ||
| if (query !== '') query += '&'; | ||
| query += `severityNumber=${filter.severityNumber}`; | ||
| } |
There was a problem hiding this comment.
+1. Can be simplified by treated these independently instead
| const { limit, continuationToken, severityText, severityNumber } = | ||
| request.query; | ||
| response.json( | ||
| await params.logStore.list({ | ||
| limit: limit ? Number.parseInt(limit.toString()) : 100, | ||
| continuationToken: continuationToken | ||
| ? continuationToken.toString() | ||
| : undefined, | ||
| severityText: severityText ? severityText.toString() : undefined, | ||
| severityNumber: severityNumber | ||
| ? Number.parseInt(severityNumber.toString()) | ||
| : undefined, | ||
| }) | ||
| ); |
| '--severity <severity>', | ||
| 'filter by severity (e.g., INFO, ERROR, WARNING)' |
There was a problem hiding this comment.
Is this filtering to that exact severity or to and above? I think the latter is a common pattern for logs
| traceId: z.string().optional(), | ||
| spanId: z.string().optional(), | ||
| severityText: z.string().optional(), | ||
| severityNumber: z.number().optional(), |
There was a problem hiding this comment.
Curious, who is using severityNumber? The CLI does not seem to be using it?
Add a log:list command similar to trace:list command for listing logs. Allows filtering on the following args:
Sample output:
Checklist: