feat: Add codeOwner to logging context #17172
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
error Received malformed response from registry for "typescript". The registry may be down. WalkthroughThe changes in this pull request involve significant modifications to the code ownership functionality within the logging context. The Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17172 +/- ##
=======================================
Coverage 35.73% 35.73%
=======================================
Files 6928 6928
Lines 147824 147828 +4
Branches 42129 42130 +1
=======================================
+ Hits 52818 52826 +8
+ Misses 95006 95002 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
![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
🧹 Outside diff range and nitpick comments (4)
libs/infra-tracing/src/lib/code-owner.ts (1)
6-10: Update function documentation to reflect new behaviorThe function documentation may be outdated after refactoring
setCodeOwnertowithCodeOwner, which now accepts a callback and arguments. Please update the doc comment to accurately describe the new behavior of wrapping a callback to set the code owner for the current dd-trace span and all nested log entries during the execution of the callback.libs/logging/src/lib/context.spec.ts (3)
15-33: Consider using a constant for the team nameThe test is well-structured with clear AAA pattern. Consider extracting 'default-team' to a constant to improve maintainability and reusability across tests.
+const DEFAULT_TEAM = 'default-team' as const; it('should add default CODE_OWNER when environment variable is set', () => { // Arrange - process.env.CODE_OWNER = 'default-team' + process.env.CODE_OWNER = DEFAULT_TEAM
35-57: Consider adding runtime validation for CodeOwners typeWhile the TypeScript type assertion is good, consider adding runtime validation to ensure 'context-team' is a valid CodeOwner value.
+ const isValidCodeOwner = (value: string): value is CodeOwners => { + return Object.values(CodeOwners).includes(value as CodeOwners); + }; + it('should override default CODE_OWNER with context codeOwner', () => { // Arrange process.env.CODE_OWNER = 'default-team' const formatter = includeContextFormatter() const logInfo = { level: 'info', message: 'Test message', } - const context = { codeOwner: 'context-team' as CodeOwners } + const contextTeam = 'context-team'; + if (!isValidCodeOwner(contextTeam)) { + throw new Error(`Invalid code owner: ${contextTeam}`); + } + const context = { codeOwner: contextTeam }
Line range hint
83-122: Consider extracting common test data setupThe tests share common setup code for logInfo and formatter. Consider using beforeEach to reduce duplication and improve maintainability.
describe('Winston context', () => { const originalEnv = process.env + let formatter: ReturnType<typeof includeContextFormatter>; + let baseLogInfo: { level: string; message: string }; beforeEach(() => { process.env = { ...originalEnv } + formatter = includeContextFormatter() + baseLogInfo = { + level: 'info', + message: 'Test message', + } })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
libs/infra-tracing/src/lib/code-owner.spec.ts(2 hunks)libs/infra-tracing/src/lib/code-owner.ts(2 hunks)libs/logging/src/lib/context.spec.ts(5 hunks)libs/logging/src/lib/context.ts(1 hunks)libs/nest/core/src/lib/code-owner/code-owner.interceptor.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
libs/logging/src/lib/context.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/nest/core/src/lib/code-owner/code-owner.interceptor.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/logging/src/lib/context.spec.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/infra-tracing/src/lib/code-owner.spec.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/infra-tracing/src/lib/code-owner.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (5)
libs/nest/core/src/lib/code-owner/code-owner.interceptor.ts (1)
23-23: LGTM
Wrapping next.handle() with withCodeOwner correctly applies the code owner context to the request handling pipeline.
libs/logging/src/lib/context.ts (1)
22-27: LGTM
The code appropriately incorporates the CODE_OWNER environment variable into the logging context, ensuring that the default codeOwner is included in log entries when not specified elsewhere.
libs/infra-tracing/src/lib/code-owner.spec.ts (1)
Line range hint 1-79: LGTM
The updated tests correctly verify the new behavior of withCodeOwner, ensuring that the code owner is set on the active span and that the callback function is executed with the correct arguments. The tests also handle cases where no active span exists, confirming that the logging warning is triggered appropriately.
libs/logging/src/lib/context.spec.ts (2)
Line range hint 1-14: LGTM! Well-structured test setup
The environment setup follows testing best practices with proper environment variable management and TypeScript usage.
Line range hint 1-122: Excellent compliance with coding guidelines
The implementation effectively uses TypeScript and creates reusable test patterns, fully adhering to the coding guidelines for libs/**/* files.
Datadog ReportAll test runs ✅ 90 Total Test Services: 0 Failed, 82 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (36)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
libs/infra-tracing/src/lib/code-owner.ts (1)
6-10: Consider enhancing TypeScript documentationWhile the current documentation explains the functionality well, consider adding TypeScript-specific documentation for better IDE support:
/** * Sets a code owner for the current dd-trace span and all nested log entries. * * The assumption here is that each trace / request has only one "dynamic" * code owner. This way we skip cluttering the trace with extra spans. + * @param codeOwner - The code owner to set for the current context + * @param callback - The function to execute within the code owner context + * @param args - Arguments to pass to the callback function + * @returns The result of the callback function */libs/nest/core/src/lib/code-owner/code-owner.interceptor.spec.ts (2)
11-13: Consider adding types to mock implementationThe mock implementation could benefit from explicit typing for better type safety during tests.
jest.mock('@island.is/infra-tracing', () => ({ - withCodeOwner: jest.fn((codeOwner, callback) => callback()), + withCodeOwner: jest.fn(<R, TArgs extends unknown[]>( + codeOwner: CodeOwners, + callback: (...args: TArgs) => R, + ...args: TArgs + ) => callback(...args)), }))
Line range hint
78-92: Consider enhancing multiple requests testWhile the test effectively verifies the number of calls, consider adding verification for the order of calls to ensure consistent behavior.
// Verify that withCodeOwner was called exactly twice (for the two 'with-owner' requests) expect(withCodeOwner).toHaveBeenCalledTimes(2) - expect(withCodeOwner).toHaveBeenCalledWith( - CodeOwners.Core, - expect.any(Function), - ) + expect(withCodeOwner.mock.calls).toEqual([ + [CodeOwners.Core, expect.any(Function)], + [CodeOwners.Core, expect.any(Function)], + ])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
libs/infra-tracing/src/lib/code-owner.ts(2 hunks)libs/nest/core/src/lib/code-owner/code-owner.interceptor.spec.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/infra-tracing/src/lib/code-owner.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/nest/core/src/lib/code-owner/code-owner.interceptor.spec.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (3)
libs/infra-tracing/src/lib/code-owner.ts (2)
1-1: LGTM: Import changes align with PR objective
The addition of withLoggingContext import is appropriate for integrating code ownership with logging context.
Line range hint 11-27: Implementation looks solid, verify error handling
The implementation effectively combines tracing and logging context while maintaining type safety. The generic type parameters allow for flexible reuse across different contexts.
Let's verify the error handling by checking for similar patterns in the codebase:
✅ Verification successful
Error handling implementation is consistent and robust
The error handling in withCodeOwner follows established patterns across the codebase:
- Proper logging of missing spans with stack traces
- Consistent use of
logger.warnfor non-critical issues - Graceful fallback to logging context when tracing is unavailable
- Stack trace inclusion for better debugging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar error handling patterns with dd-trace
rg -A 2 "scope\(\)\.active\(\)" | grep -B 2 "warn"
# Check for other uses of withLoggingContext to ensure consistent pattern
ast-grep --pattern 'withLoggingContext({ $$$}, $_, $$$)'
Length of output: 215
Script:
#!/bin/bash
# Let's check for similar error handling patterns with tracer.scope()
rg -A 5 "tracer\.scope\(\)"
# Let's also check for similar stack trace logging patterns
rg -A 3 "new Error\(\)\.stack"
# And check the logger usage pattern
rg "logger\.warn\(" -A 2
Length of output: 66188
libs/nest/core/src/lib/code-owner/code-owner.interceptor.spec.ts (1)
53-76: LGTM: Test cases provide good coverage
The test cases effectively verify both the presence and absence of the CodeOwner decorator, with clear assertions and proper function call verification.
eirikurn
added
the
deprecated:automerge
|
@coderabbitai resolve |
eirikurn
removed
the
deprecated:automerge
eirikurn
added
the
deprecated:automerge
✅ Actions performedComments resolved and changes approved. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/infra-tracing/src/lib/code-owner.ts (1)
6-10: Enhance function documentation with callback parameter detailsThe documentation should include information about the callback parameter and its return value handling.
Add these details to the documentation:
/** * Sets a code owner for the current dd-trace span and all nested log entries. * * The assumption here is that each trace / request has only one "dynamic" * code owner. This way we skip cluttering the trace with extra spans. + * + * @param codeOwner - The code owner to be set + * @param callback - The function to be executed within the code owner context + * @param args - Arguments to be passed to the callback function + * @returns The result of the callback function execution */libs/infra-tracing/src/lib/code-owner.spec.ts (1)
70-81: Consider adding test cases for different return typesWhile the current test verifies basic return value handling, consider adding test cases for:
- Primitive return types (number, string, boolean)
- Promise return values
- null/undefined handling
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
libs/infra-tracing/src/lib/code-owner.spec.ts(2 hunks)libs/infra-tracing/src/lib/code-owner.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/infra-tracing/src/lib/code-owner.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/infra-tracing/src/lib/code-owner.spec.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (5)
libs/infra-tracing/src/lib/code-owner.ts (2)
11-15: LGTM! Well-typed function signature
The generic type parameters and callback function type definitions are well structured, providing both type safety and flexibility.
16-25: LGTM! Robust implementation with proper error handling
The implementation correctly:
- Handles both active and inactive span scenarios
- Suppresses warnings in development environment
- Integrates with logging context while maintaining the code owner information
libs/infra-tracing/src/lib/code-owner.spec.ts (3)
Line range hint 1-24: LGTM! Comprehensive test setup with proper mocking
The test setup is well-structured with:
- Proper mocking of all dependencies
- Clear beforeEach setup
- Appropriate mock implementation for withLoggingContext
27-43: LGTM! Thorough test coverage for the happy path
The test case properly verifies:
- Code owner tag setting on active span
- Logging context integration
- Callback execution with correct arguments
46-68: LGTM! Good error path coverage
The test case thoroughly verifies the behavior when no active span exists:
- Warning message is logged
- Callback still executes correctly
- Logging context is maintained
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/infra-tracing/src/lib/code-owner.ts (1)
6-15: Enhance function documentation with parameter descriptionsWhile the general description is good, consider adding parameter descriptions to document the generic types and callback functionality.
/** * Sets a code owner for the current dd-trace span and all nested log entries. * * The assumption here is that each trace / request has only one "dynamic" * code owner. This way we skip cluttering the trace with extra spans. + * + * @param codeOwner - The code owner to be set for the current context + * @param callback - The function to be executed within the code owner context + * @param args - Arguments to be passed to the callback function + * @returns The result of the callback execution */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
libs/infra-tracing/src/lib/code-owner.spec.ts(2 hunks)libs/infra-tracing/src/lib/code-owner.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/infra-tracing/src/lib/code-owner.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/infra-tracing/src/lib/code-owner.spec.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (5)
libs/infra-tracing/src/lib/code-owner.ts (2)
1-1: LGTM: Import statement is correctly updated
The addition of withLoggingContext import aligns with the new functionality requirements.
16-25: LGTM: Implementation follows best practices
The implementation correctly handles:
- Span tagging with error handling
- Development environment awareness
- Context propagation through withLoggingContext
Let's verify the usage of this function across the codebase:
✅ Verification successful
Implementation verified: Code owner tracing is properly integrated
The verification confirms:
withCodeOwneris correctly used in the NestJS interceptor and test files- No legacy
setCodeOwnerfunction calls remain in the codebase - The span tag
codeOwneris exclusively set in this implementation - Context propagation through
withLoggingContextis properly integrated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of the old setCodeOwner function
rg "setCodeOwner\(" --type ts
# Search for current usage patterns of withCodeOwner
rg "withCodeOwner\(" -A 2 --type ts
Length of output: 995
Script:
#!/bin/bash
# Check if there are any other functions in the codebase that set code owner tags
ast-grep --pattern 'span.setTag("codeOwner", $_)'
# Check the implementation of withLoggingContext to verify context propagation
ast-grep --pattern 'function withLoggingContext($_) { $$$ }'
# Check for any other span tag setters related to code owners
rg 'span.*setTag.*codeOwner' --type ts
Length of output: 236
libs/infra-tracing/src/lib/code-owner.spec.ts (3)
Line range hint 1-24: LGTM: Test setup is comprehensive
The test setup properly mocks all dependencies and includes the new withLoggingContext functionality.
27-43: LGTM: Test case thoroughly verifies the happy path
The test case follows AAA pattern and includes comprehensive assertions for:
- Span tag setting
- Logging context setup
- Callback execution with correct arguments
46-81: LGTM: Edge cases and return values are well tested
The test suite properly covers:
- Behavior when no active span exists
- Warning message generation
- Return value propagation
* feat: Add codeOwner to logging context * Fix tests * fix: Silence warning in development
What
Add codeOwner to our logs.
Why
See #17092
Adding the codeOwner to tracing tags didn't work as expected. It only adds it to the current span. Not nested spans, logs or metrics. We'll make do with having it on one span, but this PR adds codeOwner to our logs.
Summary by CodeRabbit
New Features
CODE_OWNERfrom the environment variable.Bug Fixes
CODE_OWNERis set or overridden.Tests