Implemented consistent logging patterns by Juwonlo · Pull Request #442 · rinafcode/teachLink_contract

Juwonlo · 2026-04-26T02:09:18Z

Closes #373

Fixed Issue #373

drips-wave · 2026-04-26T02:09:44Z

@Juwonlo Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

Copilot

Pull request overview

Implements a proposed structured logging strategy and adds a NestJS logger implementation, along with some unrelated refactors in the recommendation explainability layer.

Changes:

Added a documented logging strategy (levels + structured JSON schema) under docs/.
Added a custom NestJS LoggerService/LoggerModule intended to produce structured JSON logs (production) and colored logs (dev).
Refactored parts of the recommendation explainability generator (extracted dominant-signal logic; switch-based branching).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.

File	Description
recommendation-system/src/explainability/explainability.ts	Refactors explanation generation logic (not directly related to logging standardization).
docs/logger.service.ts	Adds a custom NestJS logger with formatting, log-level filtering, and optional structured metadata.
docs/logger.module.ts	Declares a global Nest module exporting the custom logger provider.
docs/LOGGING_STRATEGY.md	Documents intended logging levels and structured JSON format expectations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-26T02:14:00Z

+    const { primaryReason, supportingSignals, featureAttribution } = this.extractDominantSignalExplanation(
+      dominantSignal,
+      rankingSignal,
+      userProfile,
+      similarUsers


This PR is titled/positioned as implementing consistent logging patterns, but this refactor in the recommendation explainability layer is unrelated to logging. Consider splitting these changes into a separate PR (or updating the PR description) so the logging work and its review/risk are isolated.

Copilot · 2026-04-26T02:14:00Z

+  private extractDominantSignalExplanation(
+    dominantSignal: string,
+    rankingSignal: any,
+    userProfile: Types.UserProfile,
+    similarUsers?: string[]


extractDominantSignalExplanation widens rankingSignal to any, even though generateExplanation already has a concrete { collaborativeSignal/contentSignal/... } type. Keeping the same explicit type (or a shared alias) will prevent accidental missing/typoed signal keys and improve editor support.

Copilot · 2026-04-26T02:14:01Z

+
+  private shouldLog(level: string): boolean {
+    const levels = ['debug', 'info', 'warn', 'error'];
+    const configuredIndex = levels.indexOf(this.logLevel.toLowerCase());


shouldLog() behaves incorrectly when the configured logLevel is not one of the supported values: levels.indexOf(...) returns -1, which makes targetIndex >= configuredIndex always true and logs everything. Add a fallback (e.g., treat unknown configured levels as info) before comparing indices.

Suggested change

const configuredIndex = levels.indexOf(this.logLevel.toLowerCase());

const normalizedLogLevel = this.logLevel.toLowerCase();

const effectiveLogLevel = levels.includes(normalizedLogLevel) ? normalizedLogLevel : 'info';

const configuredIndex = levels.indexOf(effectiveLogLevel);

Copilot · 2026-04-26T02:14:01Z

+    const colorCode = this.getColorCode(level);
+    const resetCode = '\x1b[0m';
+    const metadataStr = Object.keys(metadata).length > 0 ? `\n\x1b[33m[Data]: ${JSON.stringify(metadata, null, 2)}\x1b[0m` : '';
+


JSON.stringify(metadata, null, 2) can also throw on non-serializable values (circular refs, BigInt). Since this is in the log path, ensure formatting cannot throw—use a safe serializer/fallback for dev output too.

Copilot · 2026-04-26T02:14:01Z

+## 2. Structured Logging Format
+
+All off-chain services MUST output logs in **Structured JSON format** when running in production (`NODE_ENV=production`). This ensures seamless ingestion by modern log aggregators and monitoring tools (e.g., Datadog, ELK, CloudWatch).
+


This strategy doc states that all off-chain services MUST emit structured JSON logs in production, but the PR doesn’t actually integrate the provided LoggerService into any service (e.g., the Nest indexer still uses @nestjs/common Logger). Either update the documentation to describe the current state, or wire the implementation into the runtime services so the doc is accurate.

Copilot · 2026-04-26T02:14:01Z

+import { Module, Global } from '@nestjs/common';
+import { ConfigModule } from '@nestjs/config';
+import { LoggerService } from './logger.service';
+


These NestJS LoggerModule/LoggerService implementations live under docs/ and are not imported anywhere, so they won’t actually standardize runtime logging for services like the indexer. If this is intended for production use, move it into the Nest app source (e.g., indexer/src/logger/) and wire it into DI / NestFactory.create({ logger: ... }).

Copilot · 2026-04-26T02:14:02Z

+    if (!userProfile) return ''; // Guard clause
+


userProfile is a required parameter in the method signature, so if (!userProfile) return '' is unreachable under TypeScript typing and can silently hide programming errors at runtime. Either remove this guard to fail fast, or change the signature to accept userProfile?: Types.UserProfile and handle the undefined case intentionally.

Suggested change

if (!userProfile) return ''; // Guard clause

Copilot · 2026-04-26T02:14:02Z

+      // Extract trailing string arguments (usually used as the context internally by NestJS)
+      if (typeof optionalParams[optionalParams.length - 1] === 'string') {


formatMessage() assumes the trailing string in optionalParams is always a Nest context and pops it. For error(message, trace) calls (a common Nest signature), the stack trace is a string and will be incorrectly treated as the context, losing the trace and corrupting the context field. Handle error() separately (accept trace?: string, context?: string), or only treat the last string as context when there are 2+ trailing strings / a known context is provided.

Suggested change

// Extract trailing string arguments (usually used as the context internally by NestJS)

if (typeof optionalParams[optionalParams.length - 1] === 'string') {

// Only treat the final string as context when there are multiple trailing strings.

// This preserves Nest's common error(message, trace) signature, where the single

// trailing string is a stack trace rather than a context value.

let trailingStringCount = 0;

for (let i = optionalParams.length - 1; i >= 0; i--) {

if (typeof optionalParams[i] === 'string') {

trailingStringCount++;

} else {

break;

}

}

if (trailingStringCount >= 2) {

Copilot · 2026-04-26T02:14:02Z

+      level,
+      context: currentContext,
+      message: typeof message === 'object' ? JSON.stringify(message) : message,
+      ...(Object.keys(metadata).length > 0 ? { data: metadata } : {}),
+    };


JSON.stringify(message) can throw (e.g., circular references, BigInt) and would cause the logger to throw during normal operation. Wrap serialization in a try/catch and fall back to a safe serializer so logging is best-effort and never crashes the process.

Copilot · 2026-04-26T02:14:02Z

+@Injectable({ scope: Scope.TRANSIENT })
+export class LoggerService implements NestLoggerService {
+  private context?: string;


The new provider is named LoggerService, which is easy to confuse with Nest’s LoggerService interface and with other logger implementations. Consider renaming this class to something more specific (e.g., StructuredLoggerService / AppLogger) to avoid ambiguous imports and improve clarity.

Juwonlo added 2 commits April 26, 2026 04:04

Fixed Issue rinafcode#370

174858a

Fixed Issue rinafcode#373

000fcb5

Copilot AI review requested due to automatic review settings April 26, 2026 02:09

Copilot started reviewing on behalf of Juwonlo April 26, 2026 02:09 View session

Copilot AI reviewed Apr 26, 2026

View reviewed changes

Xhristin3 approved these changes Apr 27, 2026

View reviewed changes

Xhristin3 merged commit 74c74e0 into rinafcode:main Apr 27, 2026
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented consistent logging patterns#442

Implemented consistent logging patterns#442
Xhristin3 merged 2 commits into
rinafcode:mainfrom
Juwonlo:impit

Juwonlo commented Apr 26, 2026 •

edited

Loading

Uh oh!

drips-wave Bot commented Apr 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-    const configuredIndex = levels.indexOf(this.logLevel.toLowerCase());
+    const normalizedLogLevel = this.logLevel.toLowerCase();
+    const effectiveLogLevel = levels.includes(normalizedLogLevel) ? normalizedLogLevel : 'info';
+    const configuredIndex = levels.indexOf(effectiveLogLevel);

		## 2. Structured Logging Format

		All off-chain services MUST output logs in Structured JSON format when running in production (`NODE_ENV=production`). This ensures seamless ingestion by modern log aggregators and monitoring tools (e.g., Datadog, ELK, CloudWatch).

		// Extract trailing string arguments (usually used as the context internally by NestJS)
		if (typeof optionalParams[optionalParams.length - 1] === 'string') {

-      // Extract trailing string arguments (usually used as the context internally by NestJS)
-      if (typeof optionalParams[optionalParams.length - 1] === 'string') {
+      // Only treat the final string as context when there are multiple trailing strings.
+      // This preserves Nest's common error(message, trace) signature, where the single
+      // trailing string is a stack trace rather than a context value.
+      let trailingStringCount = 0;
+      for (let i = optionalParams.length - 1; i >= 0; i--) {
+        if (typeof optionalParams[i] === 'string') {
+          trailingStringCount++;
+        } else {
+          break;
+        }
+      }
+      if (trailingStringCount >= 2) {

Conversation

Juwonlo commented Apr 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drips-wave Bot commented Apr 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Juwonlo commented Apr 26, 2026 •

edited

Loading