feat: trace contextId across services #1509
Conversation
…of http request Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
WalkthroughThis PR moves NATS interception from global interceptor usage to a NATSClient wrapper, adds DI-based Logger usage, enhances context ID resolution across headers with logging, and updates module providers/imports (including MICRO_SERVICE_NAME, Prisma, and Config changes). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIGW as API Gateway
participant OIDSvc as OID4VC Service
participant NATS
Note over Client,APIGW: Old (interceptor-based)
Client->>APIGW: HTTP request
APIGW->>APIGW: NatsInterceptor (global) intercepts
APIGW->>NATS: publish message (interceptor)
NATS->>OIDSvc: deliver
Note over Client,OIDSvc: New (service-level NATSClient)
Client->>APIGW: HTTP request
APIGW->>OIDSvc: controller -> service
OIDSvc->>NATS: NATSClient.sendNatsMessage()
NATS->>OIDSvc: response
OIDSvc->>Client: HTTP response
sequenceDiagram
participant Request
participant Middleware as Context Middleware
participant Headers as HTTP Headers
participant Store as Context Storage
Request->>Middleware: incoming request
Middleware->>Headers: check headers (contextid / context-id / contextId / x-correlation-id)
alt header present
Headers-->>Middleware: header value
Middleware->>Store: setContextId(header value)
Middleware->>Middleware: logger.debug("using header")
else header absent
Middleware->>Middleware: generate UUID
Middleware->>Store: setContextId(new UUID)
Middleware->>Middleware: logger.debug("generated id")
end
Middleware-->>Request: continue
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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)
apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts (1)
57-59: Inconsistency: Logger is not dependency-injected.The AI summary indicates that the Logger should be dependency-injected via the constructor, but the code still shows direct instantiation with
private readonly logger = new Logger('Oid4vcVerificationController'). The constructor only injectsOid4vcVerificationService.For consistency with the broader DI pattern changes in this PR, consider injecting Logger via the constructor:
- private readonly logger = new Logger('Oid4vcVerificationController'); - - constructor(private readonly oid4vcVerificationService: Oid4vcVerificationService) {} + constructor( + private readonly oid4vcVerificationService: Oid4vcVerificationService, + private readonly logger: Logger + ) {}Ensure that
Loggeris provided in the module's providers array.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/agent-service/src/agent-service.module.ts(3 hunks)apps/api-gateway/src/app.module.ts(2 hunks)apps/api-gateway/src/main.ts(0 hunks)apps/api-gateway/src/oid4vc-verification/oid4vc-verification.controller.ts(1 hunks)apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts(1 hunks)apps/oid4vc-verification/src/main.ts(1 hunks)apps/oid4vc-verification/src/oid4vc-verification.controller.ts(1 hunks)apps/oid4vc-verification/src/oid4vc-verification.module.ts(2 hunks)apps/oid4vc-verification/src/oid4vc-verification.service.ts(7 hunks)libs/common/src/nats.interceptor.ts(1 hunks)libs/context/src/contextInterceptorModule.ts(2 hunks)libs/context/src/contextModule.ts(2 hunks)libs/logger/src/logging.interceptor.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/api-gateway/src/main.ts
🧰 Additional context used
🪛 ESLint
libs/context/src/contextInterceptorModule.ts
[error] 10-10: Expected no linebreak before this expression.
(implicit-arrow-linebreak)
🔇 Additional comments (9)
apps/api-gateway/src/oid4vc-verification/oid4vc-verification.service.ts (1)
2-2: Refactoring from BaseService to direct Logger is correct and contextId propagation is verified.The removal of BaseService is safe—it only provides a protected logger initialized with a name. The direct Logger instantiation with
new Logger('Oid4vcVerificationService')is working correctly.contextId propagation is handled automatically by NATSClient.sendNatsMessage(), which retrieves the contextId from ContextStorageService and sets it in message headers. No explicit contextId extraction is needed in this service.
Optional improvement: Consider dependency injection for the Logger (rather than
new Logger(...)) for better testability, though the current approach is functional.apps/oid4vc-verification/src/oid4vc-verification.service.ts (1)
35-41: LGTM: Constructor injection properly updated.The
NATSClientis correctly injected via constructor DI. Ensure thatNATSClientis provided in the module's providers array (verified inoid4vc-verification.module.ts).apps/agent-service/src/agent-service.module.ts (2)
26-28: LGTM: Module imports enhanced for logging and context tracking.The addition of
LoggerModule,PlatformConfig, andContextInterceptorModulealigns with the PR's objective to trace contextId across services.
52-56: LGTM: Global interceptor registration follows NestJS best practices.The
NatsInterceptoris properly registered as a global interceptor using theAPP_INTERCEPTORtoken. This ensures consistent NATS message interception across the agent service. TheNATSClientprovider enables dependency injection for NATS messaging operations.apps/api-gateway/src/app.module.ts (2)
36-37: LGTM: Imports added for global interceptor pattern.The
APP_INTERCEPTORandNatsInterceptorimports enable consistent NATS message interception across the API gateway service.
82-85: LGTM: Global interceptor registration is consistent.The
NatsInterceptoris registered as a global interceptor using theAPP_INTERCEPTORtoken, matching the pattern used in other modules (e.g.,agent-service.module.tsandoid4vc-verification.module.ts). This ensures uniform context tracking and correlation ID handling across services.apps/oid4vc-verification/src/oid4vc-verification.controller.ts (1)
11-14: LGTM: Logger properly injected via constructor DI.The Logger is now dependency-injected via the constructor, replacing the previous in-class instantiation. This aligns with NestJS best practices and the broader DI pattern changes in this PR. Ensure that
Loggeris provided in the module's providers array (verified inoid4vc-verification.module.tsat line 45).apps/oid4vc-verification/src/oid4vc-verification.module.ts (2)
21-25: LGTM: Module imports properly configured.The addition of
ConfigModule.forRoot(),ContextInterceptorModule,PlatformConfig,LoggerModule, andCacheModule.register()provides the necessary infrastructure for context tracking, logging, and configuration management across the verification service.
41-55: LGTM: Providers comprehensively configured for DI.All necessary providers are properly configured:
Loggerenables dependency-injected logging (used in controller at line 13)NATSClientenables the NATS client wrapper pattern (used in service at line 37)MICRO_SERVICE_NAMEprovides service identification for logging/tracingAPP_INTERCEPTORwithNatsInterceptorensures consistent NATS message interceptionThis configuration fully supports the contextId tracing feature across services.
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libs/context/src/contextModule.ts (1)
16-18: Remove the redundantcontextIdheader check and fix TypeScript typing.Line 18 checks
req.headers['contextId'](with capital I), but as noted in the previous review, Express normalizes all header keys to lowercase. This check will never match and serves no purpose. Additionally, theRequesttype from the global namespace lacks proper typing for the headers property—you should use Express'sRequesttype from@nestjs/commonorexpress.Apply this diff:
- idGenerator: (req: Request) => { + idGenerator: (req: { headers: Record<string, string | string[] | undefined> }) => { // TODO: Check if we want the x-correlation-id or the correlationId - const contextIdHeader = req.headers['contextid'] ?? req.headers['context-id'] ?? req.headers['contextId']; + const contextIdHeader = req.headers['contextid'] ?? req.headers['context-id']; const correlationIdHeader = req.headers['x-correlation-id'];
🧹 Nitpick comments (3)
libs/context/src/contextModule.ts (1)
25-30: Replace console.log with structured logging.Using
console.logbypasses the application's logging infrastructure, making it harder to control log levels, formatting, and output destinations in different environments.Consider using NestJS Logger instead:
+ idGenerator: (req: { headers: Record<string, string | string[] | undefined> }) => { + const logger = new Logger('ContextModule'); // TODO: Check if we want the x-correlation-id or the correlationId const contextIdHeader = req.headers['contextid'] ?? req.headers['context-id']; const correlationIdHeader = req.headers['x-correlation-id']; let resolvedContextId = (Array.isArray(contextIdHeader) ? contextIdHeader[0] : contextIdHeader) ?? (Array.isArray(correlationIdHeader) ? correlationIdHeader[0] : correlationIdHeader); if (resolvedContextId) { - // eslint-disable-next-line no-console - console.log('ContextId received in request headers::::', resolvedContextId); + logger.debug(`ContextId received in request headers: ${resolvedContextId}`); } else { resolvedContextId = v4(); - // eslint-disable-next-line no-console - console.log('ContextId not received in request headers, generated a new one::::', resolvedContextId); + logger.debug(`ContextId not received in request headers, generated a new one: ${resolvedContextId}`); } return resolvedContextId; }libs/context/src/contextInterceptorModule.ts (2)
23-23: Avoid creating a Logger instance on every request.Instantiating a new
Loggerfor every RPC call adds unnecessary overhead. Consider declaring it once at module scope or injecting it if possible.Apply this diff:
+const logger = new Logger('ContextInterceptorModule'); + @Global() @Module({ imports: [ ClsModule.forRoot({ global: true, interceptor: { mount: true, generateId: true, idGenerator: (context: ExecutionContext) => { try { - const logger = new Logger('ContextInterceptorModule'); const rpcContext = context.switchToRpc().getContext();
37-40: Replace console.log with structured error logging.Using
console.logfor error handling bypasses the application's logging infrastructure and makes it difficult to capture, filter, or alert on these errors in production.Apply this diff:
} catch (error) { - // eslint-disable-next-line no-console - console.log('[idGenerator] Error in idGenerator: ', error); + logger.error('[idGenerator] Error in idGenerator: ', error); + return uuid(); // Fallback to new UUID on error }Note: You should also return a fallback UUID when an error occurs to prevent returning
undefined.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/oid4vc-verification/src/oid4vc-verification.module.ts(2 hunks)libs/context/src/contextInterceptorModule.ts(2 hunks)libs/context/src/contextModule.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/oid4vc-verification/src/oid4vc-verification.module.ts
🧰 Additional context used
🪛 ESLint
libs/context/src/contextInterceptorModule.ts
[error] 10-10: Expected no linebreak before this expression.
(implicit-arrow-linebreak)
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
* feat: trace contextId across services, including passed from headers of http request Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: import issues caused by merge Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: contextId refactoring and coderabbit resolve Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: make contextInterceptorModule resilient Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> --------- Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
* feat: trace contextId across services, including passed from headers of http request Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: import issues caused by merge Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: contextId refactoring and coderabbit resolve Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: make contextInterceptorModule resilient Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> --------- Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
* feat: trace contextId across services, including passed from headers of http request Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: import issues caused by merge Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: contextId refactoring and coderabbit resolve Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> * fix: make contextInterceptorModule resilient Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com> --------- Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
, including passed from headers of http request
Summary by CodeRabbit
Refactor
Enhancements