feat: add @evlog/cli observability package#360
Open
HugoRCD wants to merge 4 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Contributor
|
Thank you for following the naming conventions! 🙏 |
Contributor
📝 WalkthroughWalkthroughThis PR introduces Changes
Demo CLI Application and Documentation
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Binary
participant Setup as setupEvlog()
participant Invoke as setup.invoke()
participant Command as Command Handler
participant Logger as useLogger()
participant Drain as Drain Pipeline
CLI->>Setup: config: { service, version, drain, catalogs, redact }
Setup-->>CLI: EvlogSetup handle
CLI->>Invoke: command: 'doctor', fn: handlerFn
activate Invoke
Invoke->>Invoke: bootstrap logger per invocation
Invoke->>Command: executes handler
activate Command
Command->>Logger: useLogger() for telemetry
Logger->>Logger: log.set({ field: value })
Logger->>Logger: log.audit({ type: 'CHECKED' })
Command-->>Invoke: returns/throws result
deactivate Command
Invoke->>Invoke: finish({ status: 0 }) or finish({ error: err })
Invoke->>Drain: emit wide event with context/status/duration
Invoke-->>CLI: event persisted
deactivate Invoke
CLI->>Setup: setup.flush()
Setup->>Drain: drain.flush()
Drain-->>CLI: buffered events flushed
CLI->>CLI: exit(code)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/cli-package-v1.md:
- Line 5: Add a top-level H1 heading at the very start of the changeset file so
the file begins with a Markdown heading (for example add a first line like "#
Add `@evlog/cli`" or "# CLI package v1") before the existing content "Add
`@evlog/cli` — CLI observability for evlog."; update the file named
cli-package-v1.md in the .changeset set so the first line is a single "#"
heading followed by the rest of the changeset content.
In `@apps/docs/content/3.integrate/frameworks/17.cli.md`:
- Around line 97-103: The example misses importing exitWithError used in the
final catch; add an import for exitWithError from '`@evlog/cli`' alongside the
existing imports so the snippet that calls runMain(main, setup).then(() =>
setup.flush()).catch(err => exitWithError(err)) compiles; update the top of the
file to import exitWithError so symbols exitWithError, runMain, setup and main
are all available.
In `@examples/cli/src/commands/sync.ts`:
- Line 27: Replace the current coercion for count with explicit parsing and
validation: parse args.records once (e.g. using parseInt/Number), ensure the
result is an integer and within the allowed range (reject negative values and
non-integers), and if invalid, exit or throw a clear error; update the variable
referenced as count and validate args.records before it flows into any
loop/payload logic so that 0, negatives, and fractional values are handled
correctly.
In `@examples/cli/src/index.ts`:
- Around line 7-14: The catch handler currently calls exitWithError immediately
and skips flushing; make the .catch callback async and perform a best-effort
await setup.flush() before calling exitWithError(error) (e.g. change
.catch((error: unknown) => { ... }) to .catch(async (error: unknown) => { if
(process.env.DEBUG) console.error(parseCliError(error)); await
setup.flush().catch(() => {}); exitWithError(error); }), referencing runMain,
setup, parseCliError and exitWithError so the error path flushes before exit.
In `@packages/cli/package.json`:
- Around line 18-36: Add a "typesVersions" field to packages/cli package.json
mapping the public exports to their .d.mts type files: create "typesVersions": {
"*": { ".": ["./dist/index.d.mts"], "citty": ["./dist/citty.d.mts"], "http":
["./dist/http.d.mts"] } } so the existing exports for ".", "./citty" and
"./http" (and the package-level "types") correctly resolve to dist/*.d.mts for
TypeScript consumers.
In `@packages/cli/README.md`:
- Around line 167-171: The diff adds a .catch(exitWithError) to the runMain call
but exitWithError is not imported, causing a compile error; fix it by adding an
import for exitWithError (or exporting it if missing) and use that symbol in the
top imports alongside runMain and setup so the final file imports runMain,
exitWithError and setup before calling runMain(main, setup).then(() =>
setup.flush()).catch(exitWithError).
In `@packages/cli/src/enrichers.ts`:
- Around line 16-20: The CLI enrichers currently emit the raw argv in the `cli`
payload (the `argv` field built from `options.argv`), which can leak secrets;
remove the raw `argv` from the emitted `cli` object or replace it with a
sanitized version (e.g., produce `sanitizedArgv` by parsing
`options.argv`/`options.flags` and redacting values for sensitive flags like
token, password, secret, key, or any positional args matched by a sensitive flag
list), and only include `command`, `flags` (sanitized), and `version`; update
the `cli` construction that references `options.argv` so events never contain
unredacted `argv`.
- Around line 34-35: applyCliContext is using an unsafe cast "as never" when
calling RequestLogger.set; instead adjust buildCliContext's return type to match
RequestLogger.set's expected FieldContext<T> (e.g., change buildCliContext(...)
to return FieldContext<Record<string, unknown>> or the precise FieldContext<T>
matching RequestLogger) so you can call logger.set(buildCliContext(options))
directly; update the buildCliContext signature (and any related types like
CliContextFields) so the types align and remove the "as never" escape hatch in
applyCliContext.
In `@packages/cli/src/flush.ts`:
- Around line 49-51: resetFlushOnExitForTests currently only flips the flags but
leaves process listeners attached; change it to remove the installed listeners
before clearing state: call process.removeListener('beforeExit', flushHandler)
and remove the same flushHandler for any signal names the module installs (e.g.
'SIGINT', 'SIGTERM', 'SIGHUP' — match the exact list used where listeners are
added), then set flushHandler = undefined and listenersInstalled = false; ensure
you reference the module's flushHandler and the same signal list used by the
install logic so the exact handler functions are removed.
- Around line 31-40: The current signal handler (handleSignal) re-emits the same
signal with process.kill(process.pid, signal) while the listener remains
registered, causing an endless loop; fix it by removing/unregistering the
installed listener before re-emitting or restoring default behavior: inside the
process.on callback (both in the !handler branch and the finally of the handler
promise) call process.removeListener(signal, /* the same callback reference used
in handleSignal */) or process.off(signal, /* callback */) (reference the
handleSignal installation code and the flushHandler variable) and only then call
process.kill(process.pid, signal) so the signal will run with default behavior
instead of re-entering the handler.
- Around line 7-12: resolveFlushFn currently rejects any non-function drain
early so objects exposing a .flush() are ignored; change the logic in
resolveFlushFn to accept either a function drain or an object with a flush
method: if drain is a function return a wrapper that calls drain(), otherwise
treat drain as candidate = drain as { flush?: () => Promise<void> } and only
return a wrapper that calls candidate.flush() when typeof candidate.flush ===
'function'; keep the existing return type and null/undefined behavior otherwise.
In `@packages/cli/src/http.ts`:
- Around line 19-22: resolveUrl can throw when constructing new URL(request,
baseURL) if baseURL is invalid; wrap the URL construction in a guard (try/catch)
inside the resolveUrl function and, on failure, fall back to returning the
original request string (or a safe default) instead of letting the exception
propagate; ensure you only attempt new URL when baseURL is truthy and valid, and
reference resolveUrl to locate the change.
In `@packages/cli/src/index.ts`:
- Around line 43-47: The env object currently spreads config.env last which
allows config.env to overwrite explicit top-level fields; change the spread
order so config.env is spread first and then set service, environment, and
version using the existing expressions (service: config.service ??
config.env?.service ?? 'cli', environment: config.environment ??
config.env?.environment, version: config.version ?? config.env?.version) so
top-level config.* values take precedence while still falling back to
config.env.* and defaults; update the env object near the config usage in
packages/cli/src/index.ts accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5a6c0896-e59b-41e8-b0dc-a150afd0eefb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (45)
.changeset/cli-package-v1.md.github/pull_request_template.md.github/workflows/ci.yml.github/workflows/semantic-pull-request.ymlAGENTS.mdapps/docs/content/1.start/3.installation.mdapps/docs/content/3.integrate/frameworks/00.overview.mdapps/docs/content/3.integrate/frameworks/17.cli.mdexamples/cli/.env.exampleexamples/cli/README.mdexamples/cli/package.jsonexamples/cli/src/catalogs/actor.tsexamples/cli/src/catalogs/audit.tsexamples/cli/src/catalogs/errors.tsexamples/cli/src/commands/deploy.tsexamples/cli/src/commands/doctor.tsexamples/cli/src/commands/index.tsexamples/cli/src/commands/pull.tsexamples/cli/src/commands/sync.tsexamples/cli/src/drain.tsexamples/cli/src/evlog.tsexamples/cli/src/index.tsexamples/cli/tsconfig.jsonpackage.jsonpackages/cli/README.mdpackages/cli/package.jsonpackages/cli/src/citty.tspackages/cli/src/enrichers.tspackages/cli/src/errors.tspackages/cli/src/flush.tspackages/cli/src/http.tspackages/cli/src/index.tspackages/cli/src/integration.tspackages/cli/src/presentation.tspackages/cli/src/redact.tspackages/cli/src/types.tspackages/cli/test/citty.test.tspackages/cli/test/cli.test.tspackages/cli/test/helpers/drain.tspackages/cli/test/http.test.tspackages/cli/tsconfig.jsonpackages/cli/tsdown.config.tspackages/cli/vitest.config.tspackages/evlog/README.mdpackages/evlog/test/README.md
HugoRCD
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli/src/flush.ts (1)
37-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefresh the stored flush function on every install.
Line 38 returns before
flushHandleris updated, so the firstinstallFlushOnExit()call wins permanently. If the process bootstraps@evlog/climore than once, shutdown hooks will flush the wrong pipeline and can drop the later setup's buffered events.Proposed fix
export function installFlushOnExit(flush: () => Promise<void>): void { - if (listenersInstalled) return - listenersInstalled = true flushHandler = flush + if (listenersInstalled) return + listenersInstalled = true const runFlush = () => { if (!flushHandler) return void flushHandler().catch(err => console.error('[evlog/cli] flush failed:', err)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/flush.ts` around lines 37 - 40, The installFlushOnExit function currently returns early when listenersInstalled is true and therefore never updates the stored flush function; change it so that flushHandler is always updated on every call to installFlushOnExit (assign the incoming flush to the module-level flushHandler unconditionally), and then only short-circuit returning when listenersInstalled is true; keep the listenersInstalled flag and existing listener installation logic intact so listeners are still only installed once. Ensure you reference/modify the existing installFlushOnExit function and the module-level flushHandler and listenersInstalled variables.packages/cli/README.md (1)
99-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFlush on the error path too.
Both snippets only await
setup.flush()after a successfulrunMain(). If the command rejects, copied integrations will jump straight toexitWithError()and can lose the final wide event for the failing invocation.Suggested doc fix
-runMain(main, setup) // was: runMain(main) - .then(() => setup.flush()) // flush batched drain writes - .catch(error => exitWithError(error)) +runMain(main, setup) // was: runMain(main) + .then(() => setup.flush()) // flush batched drain writes + .catch(async (error) => { + await setup.flush().catch(() => {}) + exitWithError(error) + })Also applies to: 168-172
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/README.md` around lines 99 - 101, The current flow calls setup.flush() only on the success path after runMain(main, setup) and skips it when runMain rejects, causing lost final events; change the control flow to always await setup.flush() regardless of success or failure (e.g., use a try/catch/finally or Promise.finally) so that setup.flush() runs before calling exitWithError(error) or before resolving; locate the call site using runMain(main, setup), setup.flush, and exitWithError to implement the guaranteed flush in the error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/docs/content/3.integrate/frameworks/17.cli.md`:
- Around line 355-360: The example currently calls useLogger() at module load
time which binds a logger outside the command-scoped context; move the call to
useLogger() (and the ofetch.create(createOutboundHooks(...)) call that uses it)
inside the command handler or runMain()/invoke() callback so the logger is
created within the active command context; specifically instantiate api by
calling ofetch.create(createOutboundHooks(useLogger())) inside the handler where
the command is executed (rather than at top-level module evaluation) so
createOutboundHooks and useLogger() are invoked with the correct command-scoped
logger.
In `@packages/cli/test/flush.test.ts`:
- Around line 35-45: Add a regression test that installs two different flush
handlers via installFlushOnExit and verifies the second (latest) handler is the
one invoked and that resetFlushOnExitForTests cleans up both listeners: create
two spies (e.g., flushA and flushB), call installFlushOnExit(flushA) then
installFlushOnExit(flushB), emit a 'beforeExit' (or otherwise trigger the flush
behavior) and assert only flushB was called (and flushA not called), then call
resetFlushOnExitForTests() and assert process.listenerCount('beforeExit')
returns to beforeCount; reference installFlushOnExit and
resetFlushOnExitForTests to locate code.
---
Outside diff comments:
In `@packages/cli/README.md`:
- Around line 99-101: The current flow calls setup.flush() only on the success
path after runMain(main, setup) and skips it when runMain rejects, causing lost
final events; change the control flow to always await setup.flush() regardless
of success or failure (e.g., use a try/catch/finally or Promise.finally) so that
setup.flush() runs before calling exitWithError(error) or before resolving;
locate the call site using runMain(main, setup), setup.flush, and exitWithError
to implement the guaranteed flush in the error path.
In `@packages/cli/src/flush.ts`:
- Around line 37-40: The installFlushOnExit function currently returns early
when listenersInstalled is true and therefore never updates the stored flush
function; change it so that flushHandler is always updated on every call to
installFlushOnExit (assign the incoming flush to the module-level flushHandler
unconditionally), and then only short-circuit returning when listenersInstalled
is true; keep the listenersInstalled flag and existing listener installation
logic intact so listeners are still only installed once. Ensure you
reference/modify the existing installFlushOnExit function and the module-level
flushHandler and listenersInstalled variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 78682747-cb49-4750-9bb5-029959a73bfd
📒 Files selected for processing (13)
.changeset/cli-package-v1.mdapps/docs/content/3.integrate/frameworks/17.cli.mdexamples/cli/src/commands/sync.tsexamples/cli/src/index.tspackages/cli/README.mdpackages/cli/package.jsonpackages/cli/src/enrichers.tspackages/cli/src/flush.tspackages/cli/src/http.tspackages/cli/src/index.tspackages/cli/test/cli.test.tspackages/cli/test/flush.test.tspackages/cli/test/http.test.ts
Comment on lines
+355
to
+360
| import { ofetch } from 'ofetch' | ||
| import { useLogger } from '@evlog/cli' | ||
| import { createOutboundHooks } from '@evlog/cli/http' | ||
|
|
||
| const api = ofetch.create(createOutboundHooks(useLogger())) | ||
| await api('https://api.example.com/v1/records') |
Contributor
There was a problem hiding this comment.
Move useLogger() inside the command handler.
This example calls useLogger() at module evaluation time, before runMain()/invoke() establishes the command-scoped context. Copied as-is, it will either bind the wrong logger or throw outside an active command.
Suggested doc fix
-const api = ofetch.create(createOutboundHooks(useLogger()))
-await api('https://api.example.com/v1/records')
+async run() {
+ const api = ofetch.create(createOutboundHooks(useLogger()))
+ await api('https://api.example.com/v1/records')
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/docs/content/3.integrate/frameworks/17.cli.md` around lines 355 - 360,
The example currently calls useLogger() at module load time which binds a logger
outside the command-scoped context; move the call to useLogger() (and the
ofetch.create(createOutboundHooks(...)) call that uses it) inside the command
handler or runMain()/invoke() callback so the logger is created within the
active command context; specifically instantiate api by calling
ofetch.create(createOutboundHooks(useLogger())) inside the handler where the
command is executed (rather than at top-level module evaluation) so
createOutboundHooks and useLogger() are invoked with the correct command-scoped
logger.
Comment on lines
+35
to
+45
| describe('resetFlushOnExitForTests', () => { | ||
| it('removes beforeExit listeners installed by installFlushOnExit', () => { | ||
| const flush = vi.fn(async () => {}) | ||
| const beforeCount = process.listenerCount('beforeExit') | ||
|
|
||
| installFlushOnExit(flush) | ||
| expect(process.listenerCount('beforeExit')).toBe(beforeCount + 1) | ||
|
|
||
| resetFlushOnExitForTests() | ||
| expect(process.listenerCount('beforeExit')).toBe(beforeCount) | ||
| }) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a double-install regression case.
This suite verifies extraction and cleanup, but it still misses the case where installFlushOnExit() is called twice and should flush the latest handler. That gap would let the stale-handler bug in packages/cli/src/flush.ts regress unnoticed.
Based on learnings: Every code change must have a matching test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/test/flush.test.ts` around lines 35 - 45, Add a regression test
that installs two different flush handlers via installFlushOnExit and verifies
the second (latest) handler is the one invoked and that resetFlushOnExitForTests
cleans up both listeners: create two spies (e.g., flushA and flushB), call
installFlushOnExit(flushA) then installFlushOnExit(flushB), emit a 'beforeExit'
(or otherwise trigger the flush behavior) and assert only flushB was called (and
flushA not called), then call resetFlushOnExitForTests() and assert
process.listenerCount('beforeExit') returns to beforeCount; reference
installFlushOnExit and resetFlushOnExitForTests to locate code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@evlog/cli— wide events per command,setupEvlog()+useLogger(), citty (runMain) and ofetch (createOutboundHooks) entrypointsexamples/cli,pnpm example:cli) with fs/Axiom/Datadog/OTLP drain routing via env/integrate/frameworks/cli(drain setup, catalogs, progressive adoption)npm publish --dry-runfor@evlog/cliTest plan
pnpm --filter @evlog/cli run test(14 tests)pnpm --filter @evlog/cli run typecheckpnpm --filter @evlog/cli run lintnpm publish --dry-runonpackages/clipnpm example:cli doctor— wide event in.evlog/logs/@evlog/clion this PRSummary by CodeRabbit
New Features
@evlog/clifor CLI observability (per-command telemetry, optional console logging via --log, error/audit helpers).Documentation
Chores