Extracted logging config and trace context into separate files#561
Merged
Conversation
WalkthroughThis PR extracts logger configuration into src/utils/logger-config.ts (exporting getLoggerConfig) and trace-context logic into src/utils/trace-context.ts (exporting extractTraceContext). src/utils/logger.ts now exports a default pino logger constructed with getLoggerConfig. src/utils/index.ts updates re-exports. Import paths in src/app.ts, src/worker-app.ts, and src/plugins/logging.ts are updated. New unit tests cover getLoggerConfig and extractTraceContext; existing logger tests were adapted. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/logger-config.ts`:
- Around line 65-82: The custom formatters.log is overwriting the GCP config's
formatters (including the important formatters.level from
createGcpLoggingPinoConfig/gcpConfig), so merge/compose rather than replace: in
the formatters.log implementation call gcpConfig.formatters?.log(object) if
present to get the base record (so any existing level mapping remains), then
extract trace_id/span_id/trace_flags from that base and return the augmented
record; ensure you do not drop gcpConfig.formatters.level (i.e. preserve or
delegate to it) so severity mapping to GCP values remains intact.
- Around line 48-55: The err serializer object is duplicated in both development
and production serializer blocks; extract the logic into a shared helper (e.g.,
function serializeError(error: Error)) that returns {message, name, code, stack}
and then assign err: serializeError in both serializer definitions (replace the
inline err method in the development and production serializer objects). Ensure
the helper preserves the existing code extraction ('code' in error) and types so
behavior remains identical.
In `@test/unit/utils/logger.test.ts`:
- Around line 22-34: The test mirrors implementation by asserting
expect(config.level).toBe(process.env.LOG_LEVEL || 'info'); instead assert the
concrete expected default (e.g. 'info') so the test fails if the default
changes; update the development test (and the production/test on line 41) to
expect the literal 'info' (or another explicit level you intend) for
config.level returned by getLoggerConfig() rather than using
process.env.LOG_LEVEL || 'info' to make the assertion deterministic and
intention-revealing.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/logger-config.ts`: - Around line 48-55: The err serializer object is duplicated in both development and production serializer blocks; extract the logic into a shared helper (e.g., function serializeError(error: Error)) that returns {message, name, code, stack} and then assign err: serializeError in both serializer definitions (replace the inline err method in the development and production serializer objects). Ensure the helper preserves the existing code extraction ('code' in error) and types so behavior remains identical. In `@test/unit/utils/logger.test.ts`: - Around line 22-34: The test mirrors implementation by asserting expect(config.level).toBe(process.env.LOG_LEVEL || 'info'); instead assert the concrete expected default (e.g. 'info') so the test fails if the default changes; update the development test (and the production/test on line 41) to expect the literal 'info' (or another explicit level you intend) for config.level returned by getLoggerConfig() rather than using process.env.LOG_LEVEL || 'info' to make the assertion deterministic and intention-revealing.test/unit/utils/logger.test.ts (1)
22-34: Consider asserting concrete expected values instead of mirroring the implementation logic.Lines 27 and 41 use
process.env.LOG_LEVEL || 'info'in the assertion, which mirrors the production code rather than asserting a known expected value. SinceLOG_LEVELisn't set in these tests, this always resolves to'info'— using the literal would make the test intent clearer and catch regressions if the default changes.Suggested tweak
- expect(config.level).toBe(process.env.LOG_LEVEL || 'info'); + expect(config.level).toBe('info');(Same for the production test on line 41.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/utils/logger.test.ts` around lines 22 - 34, The test mirrors implementation by asserting expect(config.level).toBe(process.env.LOG_LEVEL || 'info'); instead assert the concrete expected default (e.g. 'info') so the test fails if the default changes; update the development test (and the production/test on line 41) to expect the literal 'info' (or another explicit level you intend) for config.level returned by getLoggerConfig() rather than using process.env.LOG_LEVEL || 'info' to make the assertion deterministic and intention-revealing.src/utils/logger-config.ts (1)
48-55: Duplicated error serializer logic.The
errserializer is identical in both the development and production paths. Consider extracting it into a shared helper to reduce duplication.Suggested refactor
function serializeError(error: Error) { return { message: error.message, name: error.name, code: 'code' in error ? (error as Error & {code: string}).code : undefined, stack: error.stack }; }Then reference
err: serializeErrorin both serializer objects.Also applies to: 85-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/logger-config.ts` around lines 48 - 55, The err serializer object is duplicated in both development and production serializer blocks; extract the logic into a shared helper (e.g., function serializeError(error: Error)) that returns {message, name, code, stack} and then assign err: serializeError in both serializer definitions (replace the inline err method in the development and production serializer objects). Ensure the helper preserves the existing code extraction ('code' in error) and types so behavior remains identical.
cmraible
force-pushed
the
refactor-logging-configuration
branch
from
16a00bf to
5f350a0
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/logging.ts (1)
37-45:⚠️ Potential issue | 🔴 CriticalRemove or reintroduce the request-body helpers (currently undefined).
getContentLength,REQUEST_BODY_LOG_THRESHOLD_BYTES,getSerializedSizeBytes, andsummarizeRequestBodyare referenced but not defined/imported, so this hook won’t compile. Either restore the helpers or drop this hook if body logging was intentionally removed.🛠️ Suggested fix (remove the now-defunct hook)
- fastify.addHook('preHandler', async (request) => { - const contentLength = getContentLength(request.headers['content-length']); - if (contentLength && contentLength > REQUEST_BODY_LOG_THRESHOLD_BYTES) { - request.log.debug({ - event: 'IncomingRequestBody', - requestBodySize: contentLength, - parsedBodySize: getSerializedSizeBytes(request.body), - bodySummary: summarizeRequestBody(request.body) - }); - } - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/logging.ts` around lines 37 - 45, The preHandler hook references undefined helpers REQUEST_BODY_LOG_THRESHOLD_BYTES, getContentLength, getSerializedSizeBytes, and summarizeRequestBody inside the fastify.addHook call; either restore/import/implement those helpers and ensure they are exported/available to logging.ts (define REQUEST_BODY_LOG_THRESHOLD_BYTES constant and implement functions getContentLength, getSerializedSizeBytes, summarizeRequestBody) and keep the hook, or remove the entire fastify.addHook('preHandler', ...) block that logs request bodies (including the object with event: 'IncomingRequestBody', requestBodySize, parsedBodySize, bodySummary) so the file compiles; update imports/exports accordingly to match the chosen approach.
♻️ Duplicate comments (1)
src/utils/logger-config.ts (1)
60-82:⚠️ Potential issue | 🟠 MajorPreserve GCP formatter behavior when customizing
formatters.log.Overriding
formatterscan dropgcpConfig.formatters.level, which may remove GCP severity mapping. Consider merging/composing the existing formatters before adding your customlogformatter.In Pino, are formatter objects merged or replaced? If you override `formatters`, does it drop `formatters.level` from an existing config (e.g., GCP config)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/logger-config.ts` around lines 60 - 82, The current return replaces gcpConfig.formatters with a new object, which drops any existing formatters like gcpConfig.formatters.level (losing GCP severity mapping); instead merge the existing formatters by using the existing gcpConfig.formatters and only override/add the log formatter: keep createGcpLoggingPinoConfig() as gcpConfig, then set formatters to a merged object (e.g., spread gcpConfig.formatters and provide a new log function) so formatters.level from gcpConfig is preserved while adding your custom formatters.log implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/plugins/logging.ts`:
- Around line 37-45: The preHandler hook references undefined helpers
REQUEST_BODY_LOG_THRESHOLD_BYTES, getContentLength, getSerializedSizeBytes, and
summarizeRequestBody inside the fastify.addHook call; either
restore/import/implement those helpers and ensure they are exported/available to
logging.ts (define REQUEST_BODY_LOG_THRESHOLD_BYTES constant and implement
functions getContentLength, getSerializedSizeBytes, summarizeRequestBody) and
keep the hook, or remove the entire fastify.addHook('preHandler', ...) block
that logs request bodies (including the object with event:
'IncomingRequestBody', requestBodySize, parsedBodySize, bodySummary) so the file
compiles; update imports/exports accordingly to match the chosen approach.
---
Duplicate comments:
In `@src/utils/logger-config.ts`:
- Around line 60-82: The current return replaces gcpConfig.formatters with a new
object, which drops any existing formatters like gcpConfig.formatters.level
(losing GCP severity mapping); instead merge the existing formatters by using
the existing gcpConfig.formatters and only override/add the log formatter: keep
createGcpLoggingPinoConfig() as gcpConfig, then set formatters to a merged
object (e.g., spread gcpConfig.formatters and provide a new log function) so
formatters.level from gcpConfig is preserved while adding your custom
formatters.log implementation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/app.tssrc/plugins/logging.tssrc/utils/index.tssrc/utils/logger-config.tssrc/utils/logger.tssrc/utils/trace-context.tssrc/worker-app.tstest/unit/utils/logger.test.tstest/unit/utils/trace-context.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/worker-app.ts
- src/utils/trace-context.ts
- src/app.ts
- src/utils/index.ts
Collaborator
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/utils/logger.test.ts (1)
9-36: Stub LOG_LEVEL in tests to make assertions deterministic.Right now the assertions mirror the ambient environment, so the LOG_LEVEL override isn’t explicitly validated and can vary by CI config. Stubbing LOG_LEVEL makes the tests deterministic and verifies the override path.
💡 Suggested update
it('should disable logging in test environment', () => { vi.stubEnv('NODE_ENV', 'testing'); + vi.stubEnv('LOG_LEVEL', 'debug'); expect(getLoggerConfig()).toEqual({level: 'silent'}); }); it('should return pretty logger config in development', () => { vi.stubEnv('NODE_ENV', 'development'); + vi.stubEnv('LOG_LEVEL', 'debug'); const config = getLoggerConfig(); - expect(config.level).toBe(process.env.LOG_LEVEL || 'info'); + expect(config.level).toBe('debug'); expect(config.transport).toMatchObject({ target: 'pino-pretty' }); expect(config.serializers).toHaveProperty('req'); expect(config.serializers).toHaveProperty('res'); expect(config.serializers).toHaveProperty('err'); }); it('should return gcp logger config outside development and test', () => { vi.stubEnv('NODE_ENV', 'production'); + vi.stubEnv('LOG_LEVEL', 'warn'); const config = getLoggerConfig(); - expect(config.level).toBe(process.env.LOG_LEVEL || 'info'); + expect(config.level).toBe('warn'); expect(config.formatters).toHaveProperty('log'); expect(config.serializers).toHaveProperty('err'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/utils/logger.test.ts` around lines 9 - 36, Stub LOG_LEVEL in the tests that assert dynamic logger levels so assertions are deterministic: for the "should return pretty logger config in development" and "should return gcp logger config outside development and test" tests, call vi.stubEnv('LOG_LEVEL', '<desired-level>') (e.g., 'warn' or 'info') before invoking getLoggerConfig(), then assert config.level equals that stubbed value (instead of process.env.LOG_LEVEL || 'info'); reference getLoggerConfig and the two it blocks by name to locate where to add the vi.stubEnv calls and update the expectations accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/unit/utils/logger.test.ts`:
- Around line 9-36: Stub LOG_LEVEL in the tests that assert dynamic logger
levels so assertions are deterministic: for the "should return pretty logger
config in development" and "should return gcp logger config outside development
and test" tests, call vi.stubEnv('LOG_LEVEL', '<desired-level>') (e.g., 'warn'
or 'info') before invoking getLoggerConfig(), then assert config.level equals
that stubbed value (instead of process.env.LOG_LEVEL || 'info'); reference
getLoggerConfig and the two it blocks by name to locate where to add the
vi.stubEnv calls and update the expectations accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/unit/utils/logger.test.tstest/unit/utils/trace-context.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/utils/trace-context.test.ts
cmraible
force-pushed
the
refactor-logging-configuration
branch
from
b2227af to
47a2dc0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/unit/utils/logger.test.ts (1)
30-39: TheLOG_LEVELdefault fallback (|| 'info') is not explicitly testedBoth the development test (line 17,
LOG_LEVEL='warn') and the production test (line 32,LOG_LEVEL='info') stubLOG_LEVELexplicitly. The production stub happens to equal the default, so theprocess.env.LOG_LEVEL || 'info'fallback path is never exercised. A test that stubsLOG_LEVELto''(empty, therefore falsy) verifies the default branch.💡 Suggested addition
+ it('should default log level to info when LOG_LEVEL is not set', () => { + vi.stubEnv('NODE_ENV', 'production'); + vi.stubEnv('LOG_LEVEL', ''); + + const config = getLoggerConfig(); + + expect(config.level).toBe('info'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/utils/logger.test.ts` around lines 30 - 39, Add a new unit test that verifies the fallback of process.env.LOG_LEVEL to 'info' by stubbing LOG_LEVEL to an empty string (falsy) and calling getLoggerConfig(); assert that config.level === 'info' and other expectations similar to the existing production test (formatters.log and serializers.err). Locate the test suite using the existing test that calls getLoggerConfig() (the "should return gcp logger config outside development and test" case) and add a separate it() case that uses vi.stubEnv('LOG_LEVEL', '') to exercise the fallback branch.test/unit/utils/trace-context.test.ts (1)
135-147: Consider adding a test forX-Cloud-Trace-Contextwith only a trace ID (no/)There's already a test for the header-with-empty-traceId edge case (line 173), but the inverse — a header that contains only a
traceIdwith no/separator at all (e.g.,'traceIdOnly') — isn't covered. In the implementation,split('/')would yieldspanId = undefined, so only thetracekey would be set. The malformed-header test (T9) implicitly covers this path since a header lacking/behaves identically, but an explicit, semantically clear test would strengthen confidence.💡 Suggested addition
+ it('should extract only trace when X-Cloud-Trace-Context has no span ID', () => { + const mockRequest = { + headers: { + 'x-cloud-trace-context': '105445aa7843bc8bf206b12000100000' + } + } as unknown as FastifyRequest; + + const result = extractTraceContext(mockRequest); + + expect(result).toEqual({ + 'logging.googleapis.com/trace': 'projects/test-project/traces/105445aa7843bc8bf206b12000100000' + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/utils/trace-context.test.ts` around lines 135 - 147, Add a focused unit test in trace-context.test.ts that verifies extractTraceContext handles an X-Cloud-Trace-Context header containing only a traceId (no '/' separator) — e.g., header value 'traceIdOnly' — and asserts the returned object contains only the 'logging.googleapis.com/trace' key with value 'projects/test-project/traces/traceIdOnly'; place this alongside the existing malformed-header test to explicitly cover the trace-only edge case and reference the extractTraceContext function in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/unit/utils/logger.test.ts`:
- Around line 30-39: Add a new unit test that verifies the fallback of
process.env.LOG_LEVEL to 'info' by stubbing LOG_LEVEL to an empty string (falsy)
and calling getLoggerConfig(); assert that config.level === 'info' and other
expectations similar to the existing production test (formatters.log and
serializers.err). Locate the test suite using the existing test that calls
getLoggerConfig() (the "should return gcp logger config outside development and
test" case) and add a separate it() case that uses vi.stubEnv('LOG_LEVEL', '')
to exercise the fallback branch.
In `@test/unit/utils/trace-context.test.ts`:
- Around line 135-147: Add a focused unit test in trace-context.test.ts that
verifies extractTraceContext handles an X-Cloud-Trace-Context header containing
only a traceId (no '/' separator) — e.g., header value 'traceIdOnly' — and
asserts the returned object contains only the 'logging.googleapis.com/trace' key
with value 'projects/test-project/traces/traceIdOnly'; place this alongside the
existing malformed-header test to explicitly cover the trace-only edge case and
reference the extractTraceContext function in the test.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/utils/logger-config.tstest/unit/utils/logger.test.tstest/unit/utils/trace-context.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/logger-config.ts
cmraible
changed the title
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.
This is a refactor for the sake of readability and better separation of concerns; no user-facing behavioral changes.
Currently the logger.ts handles:
This refactor separates these into better named, more focused files:
request.logproxy