Decouple logger levels from deployment environment defaults - #15
Conversation
📝 WalkthroughWalkthroughThe logger now resolves levels in the order of programmatic configuration, valid ChangesLog-Level Resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant Logger
participant LOG_LEVEL as process.env.LOG_LEVEL
Application->>Logger: log(message)
Logger->>Logger: resolveLevel()
Logger->>LOG_LEVEL: read level when no override exists
LOG_LEVEL-->>Logger: normalized or invalid value
Logger-->>Application: emit or suppress record
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/reference/environment.mdx`:
- Around line 144-147: Clarify the logging environment behavior in
docs/reference/environment.mdx lines 144-147: each log call rereads the running
process’s process.env.LOG_LEVEL, so only in-process mutations are observed live;
deployment-side changes require a restart or redeploy. Also qualify the
“environment changes” wording in docs/concepts/architecture.mdx lines 121-122 to
explicitly mean changes to the running process’s process.env.
In `@tests/logger.test.ts`:
- Around line 264-268: Update the invalid log-level assertions around
fresh.setLogLevel in tests/logger.test.ts (lines 264-268) and the corresponding
assertion in tests/setup.test.ts (lines 87-95) to require a TypeError while
preserving the existing error message validation.
🪄 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: CHILL
Plan: Pro
Run ID: b09dfd4b-0e21-4b7b-84cc-1bc9c1b82d7e
📒 Files selected for processing (11)
README.mddocs/concepts/architecture.mdxdocs/configuration.mdxdocs/guides/logging.mdxdocs/reference/api.mdxdocs/reference/environment.mdxsrc/logger.tssrc/setup.tstests/integration/otel-collector.test.tstests/logger.test.tstests/setup.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Cursor Bugbot
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Prefer
interfacefor defining object shapes in TypeScript rather thantypealiases
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Useas constconst assertions for immutable values and literal types
Leverage TypeScript type narrowing instead of type assertions
Use meaningful variable names instead of magic numbers; extract descriptive constants
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, and never usevar
Alwaysawaitpromises in async functions and use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code withtry-catchblocks
Don't use async functions as Promise executors
Files:
tests/setup.test.tssrc/setup.tstests/integration/otel-collector.test.tstests/logger.test.tssrc/logger.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
**/*.{js,jsx,ts,tsx}: Use camelCase for variable and function names in JavaScript/TypeScript
Use PascalCase for class and component names in JavaScript/TypeScript
Always use async/await for promise handling instead of .then() chains
Include JSDoc comments for exported functions and classes
Use meaningful variable names that clearly describe their purpose
Avoid deeply nested conditionals; use early returns or guard clauses instead
Use const by default, let when reassignment is needed, avoid var
**/*.{js,jsx,ts,tsx}: Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully; don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Keep functions focused and under reasonable cognitive complexity limits
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Group related code together and separate concerns
Addrel="noopener"when usingtarget="_blank"on links
AvoiddangerouslySetInnerHTMLunless absolutely necessary
Don't useeval()or assign directly todocument.cookie
Validate and sanitize user input
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Avoid barrel files (indexfiles that re-export everything)
Use proper image components (for example, Next.js<Image>) over<img>tags
Usenext/heador the App Router metadata API for head elements
Use Server Components for async data fetching instead of async Client Components
Files:
tests/setup.test.tssrc/setup.tstests/integration/otel-collector.test.tstests/logger.test.tssrc/logger.ts
**/*.test.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Write unit tests for all public functions and components
Files:
tests/setup.test.tstests/integration/otel-collector.test.tstests/logger.test.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests; use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat; avoid excessivedescribenesting
Files:
tests/setup.test.tstests/integration/otel-collector.test.tstests/logger.test.ts
🔇 Additional comments (11)
src/logger.ts (1)
14-15: LGTM!Also applies to: 25-29, 31-52, 54-63, 65-80
tests/logger.test.ts (1)
170-257: LGTM!src/setup.ts (1)
78-81: LGTM!Also applies to: 239-239
tests/setup.test.ts (1)
5-20: LGTM!tests/integration/otel-collector.test.ts (1)
309-312: LGTM!Also applies to: 402-402
README.md (1)
9-9: LGTM!Also applies to: 101-102, 137-172
docs/concepts/architecture.mdx (1)
93-111: LGTM!docs/configuration.mdx (1)
6-13: LGTM!Also applies to: 147-186, 260-269
docs/guides/logging.mdx (1)
100-118: LGTM!Also applies to: 194-194
docs/reference/api.mdx (1)
55-55: LGTM!Also applies to: 145-158
docs/reference/environment.mdx (1)
3-25: LGTM!Also applies to: 123-123, 141-143
| When no programmatic level is active, `LOG_LEVEL` is checked on every log call | ||
| so deployment changes take effect immediately. Both `setupOtel({ logLevel })` | ||
| and `setLogLevel()` take precedence over it. If neither source supplies a valid | ||
| level, the logger defaults to `info`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify that only in-process environment mutations are observed live.
A deployment-side environment update does not mutate an already-running Node/Bun process. State that each call rereads process.env.LOG_LEVEL; deployment changes require restart/redeploy unless application code updates process.env.
docs/reference/environment.mdx#L144-L147: replace “deployment changes take effect immediately” with the process-local behavior.docs/concepts/architecture.mdx#L121-L122: qualify “environment changes” as changes to the running process’sprocess.env.
📍 Affects 2 files
docs/reference/environment.mdx#L144-L147(this comment)docs/concepts/architecture.mdx#L121-L122
🤖 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 `@docs/reference/environment.mdx` around lines 144 - 147, Clarify the logging
environment behavior in docs/reference/environment.mdx lines 144-147: each log
call rereads the running process’s process.env.LOG_LEVEL, so only in-process
mutations are observed live; deployment-side changes require a restart or
redeploy. Also qualify the “environment changes” wording in
docs/concepts/architecture.mdx lines 121-122 to explicitly mean changes to the
running process’s process.env.
| expect(() => | ||
| Reflect.apply(fresh.setLogLevel, undefined, ["verbose"]) | ||
| ).toThrowError( | ||
| "Invalid log level; expected one of: debug, info, warn, error, silent." | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files tests/logger.test.ts tests/setup.test.ts docs/concepts/architecture.mdx docs/reference/environment.mdx src || true
printf '\n== outlines ==\n'
for f in tests/logger.test.ts tests/setup.test.ts docs/concepts/architecture.mdx docs/reference/environment.mdx; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
fi
done
printf '\n== relevant excerpts ==\n'
sed -n '230,290p' tests/logger.test.ts
printf '\n---\n'
sed -n '60,120p' tests/setup.test.ts
printf '\n---\n'
sed -n '100,160p' docs/concepts/architecture.mdx
printf '\n---\n'
sed -n '130,170p' docs/reference/environment.mdxRepository: photon-hq/otel
Length of output: 7970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== grep for log level validation ==\n'
rg -n "Invalid log level|setLogLevel|setupOtel|getLogLevel|isOtelActive|TypeError" tests src docs -SRepository: photon-hq/otel
Length of output: 17949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# show the exact implementation and any typed error assertions nearby
for f in $(git ls-files | rg '(^|/)(logger|setup)\.(test|ts)$|(^|/)logger\.ts|(^|/)setup\.ts|(^|/)otel'); do
echo "### $f"
rg -n "throw new|TypeError|Invalid log level|setLogLevel|setupOtel|getLogLevel|isOtelActive" "$f" -n -S || true
doneRepository: photon-hq/otel
Length of output: 1204
Assert the thrown error type too. toThrowError(...) still passes if these paths start throwing a plain Error, so add a TypeError assertion in both tests/logger.test.ts and tests/setup.test.ts.
📍 Affects 2 files
tests/logger.test.ts#L264-L268(this comment)tests/setup.test.ts#L87-L95
🤖 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 `@tests/logger.test.ts` around lines 264 - 268, Update the invalid log-level
assertions around fresh.setLogLevel in tests/logger.test.ts (lines 264-268) and
the corresponding assertion in tests/setup.test.ts (lines 87-95) to require a
TypeError while preserving the existing error message validation.
Summary
LOG_LEVELinfoindependently ofDEPLOYMENT_ENVTesting
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
This is a breaking behavior change for deployments that relied on LOG_LEVEL overriding code or on debug-by-default in development; logging volume and incident toggles may shift until callers adjust.
Overview
Logger configuration is SDK-first and no longer tied to deployment metadata.
setLogLevel()andsetupOtel({ logLevel })now overrideLOG_LEVEL; when nothing is set programmatically, the default is alwaysinfo(notdebugin development).DEPLOYMENT_ENVonly setsdeployment.environmenton the OTel resource and does not affect verbosity.Validation and edge cases:
LOG_LEVELis trimmed and case-insensitive; empty values are ignored; invalid env values warn once per distinct normalized value and fall back toinfo(and are not evaluated when a programmatic override is active). Invalid values passed tosetLogLevel/setupOtel({ logLevel })throwTypeErrorbefore changing state;setupOtelapplieslogLevelwhen it is explicitly provided (including levels that are falsy in other senses).Docs, README, and tests (unit + collector integration) are updated to match the new precedence and to assert programmatic levels win even when
LOG_LEVEL=silent.Reviewed by Cursor Bugbot for commit 8741ba7. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Enhancements
LOG_LEVEL.LOG_LEVELvalues are trimmed and case-insensitive; empty values use the defaultinfolevel.infowith a warning, while invalid programmatic values are rejected immediately.DEPLOYMENT_ENVnow only sets telemetry metadata and no longer affects logging.Documentation