feat(monitoring): add logger service with log level filtering#710
feat(monitoring): add logger service with log level filtering#710sahil143 wants to merge 2 commits intokonflux-ci:mainfrom
Conversation
- Add logger with debug, info, warn, and error methods - Support log level filtering via LOG_LEVEL env variable - Default to warn in production, debug otherwise - Suppress debug console output in production environment - Integrate error logging with monitoringService - Extract LogLevel type and LOG_LEVELS constant to src/consts - Re-export LogLevel from monitoring/types for backward compat - Add comprehensive unit tests (16 test cases) Assisted-by: Cursor, Claude
sahil143
requested review from
JoaoPedroPP and
testcara
and removed request for
a team
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
New Logging Constants src/consts/log-levels.ts |
Introduces LogLevel type alias and LOG_LEVELS mapping with numeric severity values for level-based filtering. |
Logger Implementation src/monitoring/logger.ts |
New logger module exporting debug, info, warn, and error methods with environment-aware log level filtering and monitoringService integration for error reporting. |
Type Reorganization src/monitoring/types.ts |
Refactors LogLevel type from local definition to import and re-export from ~/consts/log-levels; IMonitoringProvider interface updated to reference external type. |
Legacy Constant Removal src/monitoring/const.ts |
Removes exported LogLevelValues constant, replaced by centralized log-levels.ts module. |
Logger Test Suite src/monitoring/__tests__/logger.spec.ts |
Comprehensive unit tests covering default log levels, environment-based filtering, monitoringService integration, error handling, and context propagation. |
Configuration & Documentation .gitignore, .cursor/rules/ai-assistant-workflow.mdc |
Adds Snyk security extension rules to gitignore; enhances workflow documentation with additional guidelines and examples. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Logger
participant Environment
participant Console
participant MonitoringService
Caller->>Logger: call debug/info/warn/error(message, ...)
Logger->>Environment: read LOG_LEVEL env var
Logger->>Logger: compute currentLevel & shouldLog
alt shouldLog returns true
Logger->>Console: log level-prefixed message
Note over Console: "[DEBUG]", "[INFO]", "[WARN]", "[ERROR]"
end
alt error method called
Logger->>MonitoringService: captureException(error)
MonitoringService-->>Logger: ack
end
Logger-->>Caller: void
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately summarizes the main change: introducing a logger service with log level filtering capabilities, which is the primary focus of the changeset. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
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 @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/monitoring/__tests__/logger.spec.ts (1)
107-162: Environment variable cleanup is not failure-safe insideisolateModules.If an assertion throws inside the
jest.isolateModulescallback, the cleanup lines (e.g.,process.env.NODE_ENV = 'test'/delete process.env.LOG_LEVEL) won't execute, leaking env state to subsequent tests.Wrap each block in
try/finallyto ensure cleanup:Example fix for lines 108-120
it('should not log debug messages in production environment', () => { jest.isolateModules(() => { - process.env.NODE_ENV = 'production'; - process.env.LOG_LEVEL = 'debug'; - // eslint-disable-next-line `@typescript-eslint/no-var-requires` - const prodLogger = require('../logger').logger; - prodLogger.debug('should not appear'); - - expect(consoleMock.log).not.toHaveBeenCalled(); - process.env.NODE_ENV = 'test'; - delete process.env.LOG_LEVEL; + try { + process.env.NODE_ENV = 'production'; + process.env.LOG_LEVEL = 'debug'; + // eslint-disable-next-line `@typescript-eslint/no-var-requires` + const prodLogger = require('../logger').logger; + prodLogger.debug('should not appear'); + + expect(consoleMock.log).not.toHaveBeenCalled(); + } finally { + process.env.NODE_ENV = 'test'; + delete process.env.LOG_LEVEL; + } }); });Apply the same pattern to all
isolateModulesblocks that mutateprocess.env.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/monitoring/__tests__/logger.spec.ts` around lines 107 - 162, The tests mutate process.env inside jest.isolateModules callbacks without guaranteed cleanup; wrap the body of each jest.isolateModules callback (in the 'should not log debug messages in production environment', 'should suppress debug and info when LOG_LEVEL is warn', and 'should only allow error when LOG_LEVEL is error' tests) in a try/finally and restore process.env values in finally (e.g., reset NODE_ENV to 'test' and delete LOG_LEVEL) so environment changes are reverted even if assertions throw; apply this pattern to every isolateModules block that sets process.env.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/monitoring/__tests__/logger.spec.ts`:
- Around line 219-235: The test name is misleading: it currently reads 'should
not call monitoringService when error level is suppressed' but the body requires
captureException to be called; update the test to reflect its behavior by
renaming the test description to something like 'should call
monitoringService.captureException when error level is active' (or delete the
test if it's a duplicate of the existing error-level test). Locate the test
within the describe('monitoringService integration') block in
src/monitoring/__tests__/logger.spec.ts and change the it(...) description
string to the new, accurate text (the assertions using mockCaptureException and
errorLogger.error remain unchanged).
In `@src/monitoring/logger.ts`:
- Line 1: The import in logger.ts uses a relative path; change it to the
project's path-alias form by importing LOG_LEVELS and LogLevel from the aliased
module (e.g. replace the relative import with import { LOG_LEVELS, type LogLevel
} from '~/consts/log-levels') so LOG_LEVELS and LogLevel resolve via the
configured ~/ alias and conform to project conventions; no runtime logic changes
required.
- Around line 4-5: The code currently casts process.env.LOG_LEVEL to LogLevel
and assigns it to currentLevel which can be an invalid value and cause
LOG_LEVELS[currentLevel] to be undefined; update the logic around currentLevel
(and related shouldLog/LOG_LEVELS usage) to validate that process.env.LOG_LEVEL
is one of the known keys in LOG_LEVELS, and if not, fall back to the default
computed level (NODE_ENV-based) and emit a warning (e.g., via console.warn or
the module logger) that the provided LOG_LEVEL was invalid; ensure subsequent
checks in shouldLog use the validated level so comparisons against
LOG_LEVELS[currentLevel] never encounter undefined.
---
Nitpick comments:
In `@src/monitoring/__tests__/logger.spec.ts`:
- Around line 107-162: The tests mutate process.env inside jest.isolateModules
callbacks without guaranteed cleanup; wrap the body of each jest.isolateModules
callback (in the 'should not log debug messages in production environment',
'should suppress debug and info when LOG_LEVEL is warn', and 'should only allow
error when LOG_LEVEL is error' tests) in a try/finally and restore process.env
values in finally (e.g., reset NODE_ENV to 'test' and delete LOG_LEVEL) so
environment changes are reverted even if assertions throw; apply this pattern to
every isolateModules block that sets process.env.
| describe('monitoringService integration', () => { | ||
| it('should not call monitoringService when error level is suppressed', () => { | ||
| jest.isolateModules(() => { | ||
| // There's no level higher than error that would suppress it, | ||
| // but we can verify it IS called when error level is active | ||
| process.env.LOG_LEVEL = 'error'; | ||
| process.env.NODE_ENV = 'test'; | ||
| // eslint-disable-next-line @typescript-eslint/no-var-requires | ||
| const errorLogger = require('../logger').logger; | ||
|
|
||
| errorLogger.error('critical', new Error('fail')); | ||
|
|
||
| expect(mockCaptureException).toHaveBeenCalledWith(new Error('fail'), undefined); | ||
|
|
||
| delete process.env.LOG_LEVEL; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Misleading test name — it verifies the opposite of what it describes.
The test is named 'should not call monitoringService when error level is suppressed' but actually asserts that captureException is called. The inline comment (lines 222-223) acknowledges there's no level above error to suppress it, making this test effectively a duplicate of the error-level integration test.
Consider renaming to something like 'should call monitoringService.captureException when error level is active' or removing it if redundant with the existing error tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/monitoring/__tests__/logger.spec.ts` around lines 219 - 235, The test
name is misleading: it currently reads 'should not call monitoringService when
error level is suppressed' but the body requires captureException to be called;
update the test to reflect its behavior by renaming the test description to
something like 'should call monitoringService.captureException when error level
is active' (or delete the test if it's a duplicate of the existing error-level
test). Locate the test within the describe('monitoringService integration')
block in src/monitoring/__tests__/logger.spec.ts and change the it(...)
description string to the new, accurate text (the assertions using
mockCaptureException and errorLogger.error remain unchanged).
| @@ -0,0 +1,38 @@ | |||
| import { LOG_LEVELS, type LogLevel } from '../consts/log-levels'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use absolute path alias for the import.
Per project conventions, imports from src/ should use the configured ~/ alias.
Suggested fix
-import { LOG_LEVELS, type LogLevel } from '../consts/log-levels';
+import { LOG_LEVELS, type LogLevel } from '~/consts/log-levels';As per coding guidelines: "Use absolute imports with configured path aliases: ~/components, ~/types, ~/k8s, ~/utils, ~/models, @routes".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { LOG_LEVELS, type LogLevel } from '../consts/log-levels'; | |
| import { LOG_LEVELS, type LogLevel } from '~/consts/log-levels'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/monitoring/logger.ts` at line 1, The import in logger.ts uses a relative
path; change it to the project's path-alias form by importing LOG_LEVELS and
LogLevel from the aliased module (e.g. replace the relative import with import {
LOG_LEVELS, type LogLevel } from '~/consts/log-levels') so LOG_LEVELS and
LogLevel resolve via the configured ~/ alias and conform to project conventions;
no runtime logic changes required.
| const currentLevel: LogLevel = | ||
| (process.env.LOG_LEVEL as LogLevel) || (process.env.NODE_ENV === 'production' ? 'warn' : 'debug'); |
There was a problem hiding this comment.
Invalid LOG_LEVEL values silently suppress all logging.
If process.env.LOG_LEVEL is set to an unrecognized value (e.g., 'verbose'), the cast as LogLevel won't fail at runtime. LOG_LEVELS[currentLevel] will return undefined, causing shouldLog to evaluate number >= undefined → false for every level, silently dropping all logs with no indication of misconfiguration.
Consider validating or falling back:
Suggested fix
-const currentLevel: LogLevel =
- (process.env.LOG_LEVEL as LogLevel) || (process.env.NODE_ENV === 'production' ? 'warn' : 'debug');
+const defaultLevel: LogLevel = process.env.NODE_ENV === 'production' ? 'warn' : 'debug';
+const envLevel = process.env.LOG_LEVEL as string | undefined;
+const currentLevel: LogLevel =
+ envLevel && envLevel in LOG_LEVELS ? (envLevel as LogLevel) : defaultLevel;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const currentLevel: LogLevel = | |
| (process.env.LOG_LEVEL as LogLevel) || (process.env.NODE_ENV === 'production' ? 'warn' : 'debug'); | |
| const defaultLevel: LogLevel = process.env.NODE_ENV === 'production' ? 'warn' : 'debug'; | |
| const envLevel = process.env.LOG_LEVEL as string | undefined; | |
| const currentLevel: LogLevel = | |
| envLevel && envLevel in LOG_LEVELS ? (envLevel as LogLevel) : defaultLevel; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/monitoring/logger.ts` around lines 4 - 5, The code currently casts
process.env.LOG_LEVEL to LogLevel and assigns it to currentLevel which can be an
invalid value and cause LOG_LEVELS[currentLevel] to be undefined; update the
logic around currentLevel (and related shouldLog/LOG_LEVELS usage) to validate
that process.env.LOG_LEVEL is one of the known keys in LOG_LEVELS, and if not,
fall back to the default computed level (NODE_ENV-based) and emit a warning
(e.g., via console.warn or the module logger) that the provided LOG_LEVEL was
invalid; ensure subsequent checks in shouldLog use the validated level so
comparisons against LOG_LEVELS[currentLevel] never encounter undefined.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #710 +/- ##
==========================================
+ Coverage 86.63% 86.66% +0.02%
==========================================
Files 764 765 +1
Lines 58225 58264 +39
Branches 5658 6891 +1233
==========================================
+ Hits 50441 50492 +51
+ Misses 7603 7556 -47
- Partials 181 216 +35
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 80 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Fixes
Description
Adds a structured logger service to the monitoring module, replacing raw console.* calls with level-aware logging that integrates with the monitoring/error-reporting.
Type of change
Screen shots / Gifs for design review
No UI changes
How to test or reproduce?
Browser conformance:
Summary by CodeRabbit
New Features
Documentation
Chores