Skip to content

Commit 426c548

Browse files
Drarig29claude
Drarig29
and
claude
authored
feat(logger): add global --log-format json option (#2372)
* Add global --log-format option with centralized logger Introduce a hidden, inherited `--log-format` (text|json, env DD_LOG_FORMAT) option on BaseCommand backed by a shared `this.logger` getter, so any command can emit JSONL logs with the level preserved (see #1991) without redeclaring the option or instantiating its own logger. - BaseCommand exposes a lazy `logger` getter (the single sanctioned stream sink) wired to the resolved log format; input validated via typanion `t.isEnum`, resolution order handled by clipanion's `{env}`. - Migrate the commands that declared their own Logger onto the inherited getter (forced by the type-level collision with the new getter). - ESLint: forbid direct this.context.stdout/stderr and process.stdout/stderr, scoped to a migrated-files allowlist that grows per PR. - createMockContext defaults `env` to a defined empty object so clipanion's `{env}` option does not crash on undefined. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * Add JSON logging migration plan Track the multi-PR migration to JSONL logging with a checklist; PR 1 (foundation) is complete. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * Route version banner through the logger so it respects --log-format The entrypoint printed the `datadog-ci v…` banner directly to stdout before the command ran, producing a non-JSON line that broke JSONL parsing. Parse argv first (`cli.process`), then emit the banner via the resolved command's logger (falling back to a default text logger for builtins / parse errors), so it becomes a JSON line under `--log-format json` and stays dim text otherwise. - Logger: add `isJsonOutput()` accessor. - BaseCommand: expose `logger` publicly so the entrypoint can reuse it. - printVersion now takes a Logger. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * Rewrite comment and also skip on `--help` * Change codeowners for `plans/` folder --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
1 parent df67891 commit 426c548

23 files changed

Lines changed: 405 additions & 64 deletions

File tree

‎.github/CODEOWNERS‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,3 +82,6 @@ packages/base/src/helpers/**tests**/user-provided-git.test.ts @DataDog/datadog-
8282
# Claude Code config (no docs review needed)
8383
CLAUDE.md @DataDog/datadog-ci-admins
8484
.claude/ @DataDog/datadog-ci-admins
85+
86+
# Agent-generated plans (no docs review needed)
87+
plans/ @DataDog/datadog-ci-admins

‎eslint.config.mjs‎

Lines changed: 39 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,38 @@ import tseslint from 'typescript-eslint'
1414
const tsFiles = ['**/*.ts', '**/*.tsx', '**/*.mjs', '**/*.cjs', '**/*.cts', '**/*.mts', '**/*.js']
1515
const yamlFiles = ['**/*.yml', '**/*.yaml']
1616

17+
// Commands must log through `this.logger` (BaseCommand) so that `--log-format json`
18+
// is honoured everywhere. Writing to stdout/stderr directly bypasses it. This list
19+
// grows as commands are migrated onto the shared logger; the rule becomes repo-wide
20+
// once the migration is complete.
21+
const noDirectStreamWriteFiles = ['packages/base/src/index.ts']
22+
23+
// Restricted-syntax selectors applied to all TS files.
24+
const baseRestrictedSyntax = [
25+
{
26+
selector: "Literal[value='/dev/null']",
27+
message: "Please use `os.devNull` instead of `'/dev/null'`.",
28+
},
29+
{
30+
// imported as `import os from 'os'`
31+
selector: "MemberExpression[object.name='os'][property.name='EOL']",
32+
message: 'Please use `\\n` instead of `os.EOL` when splitting the `stdout`/`stderr` into lines.',
33+
},
34+
]
35+
36+
// Selectors forbidding direct `this.context.stdout/stderr` and `process.stdout/stderr` access.
37+
const restrictedStreamSyntax = [
38+
{
39+
selector:
40+
"MemberExpression[object.object.type='ThisExpression'][object.property.name='context'][property.name=/^(stdout|stderr)$/]",
41+
message: 'Log through `this.logger` instead of writing to `this.context.stdout`/`this.context.stderr` directly.',
42+
},
43+
{
44+
selector: "MemberExpression[object.name='process'][property.name=/^(stdout|stderr)$/]",
45+
message: 'Log through `this.logger` instead of writing to `process.stdout`/`process.stderr` directly.',
46+
},
47+
]
48+
1749
const restrictedImports = [
1850
{
1951
// forbid using named imports for chalk
@@ -390,18 +422,7 @@ export default defineConfig(
390422
'jest/padding-around-test-blocks': 'error',
391423
'no-prototype-builtins': 'off',
392424
'no-restricted-imports': ['error', {paths: restrictedImports}],
393-
'no-restricted-syntax': [
394-
'error',
395-
{
396-
selector: "Literal[value='/dev/null']",
397-
message: "Please use `os.devNull` instead of `'/dev/null'`.",
398-
},
399-
{
400-
// imported as `import os from 'os'`
401-
selector: "MemberExpression[object.name='os'][property.name='EOL']",
402-
message: 'Please use `\\n` instead of `os.EOL` when splitting the `stdout`/`stderr` into lines.',
403-
},
404-
],
425+
'no-restricted-syntax': ['error', ...baseRestrictedSyntax],
405426
},
406427
},
407428
{
@@ -414,6 +435,12 @@ export default defineConfig(
414435
'no-restricted-imports': ['error', {paths: restrictedImports}],
415436
},
416437
},
438+
{
439+
files: noDirectStreamWriteFiles,
440+
rules: {
441+
'no-restricted-syntax': ['error', ...baseRestrictedSyntax, ...restrictedStreamSyntax],
442+
},
443+
},
417444
...yml.configs['flat/standard'],
418445
{
419446
files: yamlFiles,

‎packages/base/src/__tests__/index.test.ts‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
import {BaseCommand} from '..'
2+
import {LOG_FORMAT_ENV_VAR} from '../constants'
3+
import {makeRunCLI} from '../helpers/__tests__/testing-tools'
4+
5+
class LoggingCommand extends BaseCommand {
6+
public static paths = [['logging-test']]
7+
8+
public async execute(): Promise<number> {
9+
this.logger.info('hello')
10+
this.logger.warn('careful')
11+
this.logger.error('boom')
12+
13+
return 0
14+
}
15+
}
16+
17+
describe('BaseCommand --log-format', () => {
18+
const runCLI = makeRunCLI(LoggingCommand, ['logging-test'])
19+
20+
it('defaults to text output', async () => {
21+
const {context, code} = await runCLI([])
22+
23+
expect(code).toBe(0)
24+
const out = context.stdout.toString()
25+
expect(() => JSON.parse(out.split('\n')[0])).toThrow()
26+
expect(out).toContain('hello')
27+
})
28+
29+
it('emits one JSON object per log line with the correct level when --log-format json', async () => {
30+
const {context, code} = await runCLI(['--log-format', 'json'])
31+
32+
expect(code).toBe(0)
33+
const lines = context.stdout
34+
.toString()
35+
.split('\n')
36+
.filter((line) => line.length > 0)
37+
.map((line) => JSON.parse(line) as Record<string, string>)
38+
39+
expect(lines.map((entry) => entry.level)).toEqual(['info', 'warn', 'error'])
40+
expect(lines.map((entry) => entry.message)).toEqual(['hello', 'careful', 'boom'])
41+
})
42+
43+
it('reads the format from the DD_LOG_FORMAT environment variable', async () => {
44+
const {context, code} = await runCLI([], {[LOG_FORMAT_ENV_VAR]: 'json'})
45+
46+
expect(code).toBe(0)
47+
const firstLine = context.stdout.toString().split('\n')[0]
48+
expect(JSON.parse(firstLine)).toMatchObject({level: 'info', message: 'hello'})
49+
})
50+
51+
it('lets the CLI flag take precedence over the environment variable', async () => {
52+
const {context, code} = await runCLI(['--log-format', 'text'], {[LOG_FORMAT_ENV_VAR]: 'json'})
53+
54+
expect(code).toBe(0)
55+
expect(() => JSON.parse(context.stdout.toString().split('\n')[0])).toThrow()
56+
})
57+
58+
it('fails with a clear error on an invalid format', async () => {
59+
const {context, code} = await runCLI(['--log-format', 'yaml'])
60+
61+
expect(code).not.toBe(0)
62+
expect(context.stdout.toString()).toContain('Invalid value for --log-format')
63+
})
64+
})

‎packages/base/src/__tests__/version.test.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
import {Logger, LogLevel} from '../helpers/logger'
2+
import {cliVersion, printVersion} from '../version'
3+
4+
describe('printVersion', () => {
5+
const collect = (jsonOutput: boolean) => {
6+
const lines: string[] = []
7+
const logger = new Logger((s) => lines.push(s), LogLevel.INFO, {jsonOutput})
8+
9+
return {lines, logger}
10+
}
11+
12+
it('emits the banner as a JSON line in JSON mode', () => {
13+
const {lines, logger} = collect(true)
14+
15+
printVersion(logger)
16+
17+
expect(JSON.parse(lines[0])).toEqual({level: 'info', message: `datadog-ci v${cliVersion}`})
18+
})
19+
20+
it('emits a dim text banner in text mode', () => {
21+
const {lines, logger} = collect(false)
22+
23+
printVersion(logger)
24+
25+
expect(lines[0]).toContain(`datadog-ci v${cliVersion}`)
26+
expect(() => JSON.parse(lines[0])).toThrow()
27+
})
28+
29+
it('does not print for the version command', () => {
30+
const original = process.argv
31+
process.argv = ['node', 'datadog-ci', 'version']
32+
33+
try {
34+
const {lines, logger} = collect(false)
35+
printVersion(logger)
36+
expect(lines).toHaveLength(0)
37+
} finally {
38+
process.argv = original
39+
}
40+
})
41+
})

‎packages/base/src/commands/git-metadata/upload.ts‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import {InvalidConfigurationError} from '../../helpers/errors'
1111
import {enableFips} from '../../helpers/fips'
1212
import {ICONS} from '../../helpers/formatting'
1313
import type {RequestBuilder} from '../../helpers/interfaces'
14-
import {Logger, LogLevel} from '../../helpers/logger'
14+
import {LogLevel} from '../../helpers/logger'
1515
import type {MetricsLogger} from '../../helpers/metrics'
1616
import {getMetricsLogger} from '../../helpers/metrics'
1717
import {datadogRoute} from '../../helpers/request/datadog-route'
@@ -64,19 +64,13 @@ export class GitMetadataUploadCommand extends BaseCommand {
6464
fipsIgnoreError: toBoolean(process.env[FIPS_IGNORE_ERROR_ENV_VAR]) ?? false,
6565
}
6666

67-
private logger: Logger = new Logger((s: string) => {
68-
this.context.stdout.write(s)
69-
}, LogLevel.INFO)
70-
7167
public async execute() {
7268
const initialTime = Date.now()
7369

7470
enableFips(this.fips || this.config.fips, this.fipsIgnoreError || this.config.fipsIgnoreError)
7571

7672
if (this.verbose) {
77-
this.logger = new Logger((s: string) => {
78-
this.context.stdout.write(s)
79-
}, LogLevel.DEBUG)
73+
this.logger.setLogLevel(LogLevel.DEBUG)
8074
}
8175
if (this.dryRun) {
8276
this.logger.warn(renderDryRunWarning())

‎packages/base/src/commands/plugin/list.ts‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import chalk from 'chalk'
22
import {Command, Option} from 'clipanion'
33

4-
import {LogLevel, Logger} from '../../helpers/logger'
54
import {listAllPlugins} from '../../helpers/plugin'
65

76
import {BaseCommand} from '../..'
@@ -25,10 +24,6 @@ export class PluginListCommand extends BaseCommand {
2524
public json = Option.Boolean('--json', {required: false})
2625
public all = Option.Boolean('-a,--all', {required: false})
2726

28-
private logger: Logger = new Logger((s: string) => {
29-
this.context.stdout.write(s)
30-
}, LogLevel.INFO)
31-
3227
public async execute() {
3328
const allPlugins = listAllPlugins()
3429
const builtinPlugins = new Set(this.context.builtinPlugins)

‎packages/base/src/constants.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ export const DATADOG_SITES: string[] = [
2222
export const FIPS_ENV_VAR = 'DATADOG_FIPS'
2323
export const FIPS_IGNORE_ERROR_ENV_VAR = 'DATADOG_FIPS_IGNORE_ERROR'
2424

25+
export const LOG_FORMAT_ENV_VAR = 'DD_LOG_FORMAT'
26+
2527
export const CONTENT_TYPE_HEADER = 'Content-Type'
2628
export const CONTENT_TYPE_VALUE_PROTOBUF = 'application/x-protobuf'
2729
export const CONTENT_TYPE_VALUE_JSON = 'application/json'

‎packages/base/src/helpers/__tests__/testing-tools.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,9 @@ export const createMockContext = (opts?: MockContextOptions): MockCommandContext
3939
let err = ''
4040

4141
return {
42-
env: opts?.env,
42+
// Default to a defined (but empty) env so clipanion options using `{env: ...}` don't
43+
// crash on `context.env[...]`, while keeping tests deterministic (no shell-env leakage).
44+
env: opts?.env ?? {},
4345
stdout: {
4446
toString: () => out,
4547
write: (chunk: string) => {

‎packages/base/src/helpers/logger.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,10 @@ export class Logger {
5959
this.jsonOutput = newJsonOutput
6060
}
6161

62+
public isJsonOutput(): boolean {
63+
return this.jsonOutput
64+
}
65+
6266
public error(s: string) {
6367
if (this.loglevel <= LogLevel.ERROR) {
6468
this.emit(LogLevel.ERROR, s, chalk.red)

‎packages/base/src/index.ts‎

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
import type {BaseContext} from 'clipanion'
22

3-
import {Command} from 'clipanion'
3+
import {Command, Option} from 'clipanion'
4+
import * as t from 'typanion'
5+
6+
import {LOG_FORMAT_ENV_VAR} from './constants'
7+
import {Logger, LogLevel} from './helpers/logger'
48

59
export type CommandContext = BaseContext & {
610
builtinPlugins: string[]
@@ -9,4 +13,28 @@ export type CommandContext = BaseContext & {
913
/**
1014
* This command should be extended by **every** command in the monorepo.
1115
*/
12-
export abstract class BaseCommand extends Command<CommandContext> {}
16+
export abstract class BaseCommand extends Command<CommandContext> {
17+
// Hidden while the JSON logging migration is in progress: most commands do not
18+
// honour it yet. Unhide once coverage is broad enough.
19+
// Resolution order (handled by clipanion): CLI flag > DD_LOG_FORMAT env var > default.
20+
protected logFormat = Option.String('--log-format', 'text', {
21+
env: LOG_FORMAT_ENV_VAR,
22+
hidden: true,
23+
description: "Output format for logs: 'text' (default) or 'json' (one JSON object per line).",
24+
validator: t.isEnum(['text', 'json'] as const),
25+
})
26+
27+
private _logger?: Logger
28+
29+
public get logger(): Logger {
30+
if (!this._logger) {
31+
// The sanctioned sink: this is the single place allowed to write to the raw stream.
32+
// eslint-disable-next-line no-restricted-syntax
33+
this._logger = new Logger((s) => this.context.stdout.write(s), LogLevel.INFO, {
34+
jsonOutput: this.logFormat === 'json',
35+
})
36+
}
37+
38+
return this._logger
39+
}
40+
}

0 commit comments

Comments
 (0)