feat: implement Epic 3.5 observability logging#265
Conversation
- Backend: structured logging with Pino and trace middleware - Add trace-middleware.ts for X-Trace-Id header handling - Add /api/logs endpoint for frontend log collection - Extract client IP and User-Agent from headers - Update logger.ts with createLogger(c) factory - Frontend: unified logging SDK with Trace pattern - Add logger-client.ts with Trace-based API - LogOptions supports external traceId injection - Auto-collect client context (language, timezone, screen, network) - Buffer logs and batch send to backend - API logging: unified pattern in router.ts - Simplified log helper: log = trace || logger - Log success (info) and failure (error) for all API calls - Auto-generate or reuse traceId from Trace instance - Business operation logging in pages: - Upload: success/failure tracking with counts - Library CRUD: create/delete with error handling - Playlist CRUD: create/delete with default check - Song deletion: LibraryDetailPage with context - Playlist song operations: remove and reorder Related: #249 (Backend), #250 (Frontend), Epic 3.5
There was a problem hiding this comment.
Pull request overview
This PR implements Epic 3.5's observability logging foundation with structured logging infrastructure for both backend (Pino with trace middleware, log collection endpoint) and frontend (new trace-based logger API with buffering and remote submission). The implementation adds comprehensive logging across business operations including uploads, library/playlist CRUD, and player operations.
Key changes:
- Backend: Trace middleware, enhanced logger with createLogger(c) factory, POST /api/logs endpoint for frontend log collection
- Frontend: Complete logger-client.ts rewrite with Trace API, auto-context collection, and batched submission
- Application-wide: Systematic logging migration to new structured format
[Component][method]with{ raw: {...} }pattern
Reviewed changes
Copilot reviewed 63 out of 64 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/lib/trace-middleware.ts | New middleware to extract/generate X-Trace-Id for request correlation |
| backend/src/lib/logger.ts | Enhanced Pino logger with createLogger(c) factory for structured logging |
| backend/src/routes/logs.ts | New endpoint to receive frontend logs with validation schema |
| frontend/src/lib/logger-client.ts | Complete rewrite with Trace API, buffering, and remote submission |
| frontend/src/lib/api/router.ts | Added X-Trace-Id header injection and trace option support |
| frontend/src/stores/* | Updated all stores to new logging format with raw data wrapping |
| frontend/src/pages/* | Added business operation logging (upload, CRUD operations) |
| frontend/src/lib/* | Updated utilities and services to new logging format |
| frontend/src/components/ui/dropdown-menu.tsx | Unrelated Tailwind CSS updates (contains critical typo) |
Files not reviewed (1)
- backend/package-lock.json: Language not supported
- Fix logger.error signature: pass Error objects instead of strings - Make backend schema fields optional to match frontend LogOptions - Revert dropdown-menu CSS to Tailwind v4 syntax (Copilot was wrong)
Copilot Review ResponsesAll issues have been addressed in commit ee636e6: 1. Schema inconsistency (logger-client.ts): ✅ Made col1, col2, and source optional in backend schema to match frontend LogOptions. 2-6. logger.error signature issues: ✅ Fixed all calls to pass Error objects instead of strings or undefined:
7. CSS syntax (dropdown-menu.tsx): ℹ️ This is not a typo. max-h-(--var) is the correct Tailwind CSS v4 syntax, replacing the old max-h-[var(--var)]. See IDE hint in the file confirming this. |
|
Re: SignInPage.tsx line 51 You're right. The errorCode from OAuth callback is a controlled error state (e.g., 'access_denied'), not an exception. Passing undefined as the error parameter is more accurate here since there's no actual Error object to log. Fixed to: logger.error('GitHub OAuth failed', undefined, { errorCode }) |
|
Re: dropdown-menu.tsx line 66 This is Tailwind CSS v4 syntax, not an error:
These are intentional v4 upgrades, not bugs. See Tailwind CSS v4 docs for the new arbitrary value syntax. |
|
Re: dropdown-menu.tsx line 85 You're correct. The data-[disabled] syntax is missing the colon. Should be data-[disabled]: to properly apply disabled state styling. Will fix this. |
|
CORRECTION: Re: dropdown-menu.tsx line 85 Wait - checking Tailwind CSS v4 docs. The syntax data-disabled: (without brackets) might be the new v4 data attribute variant syntax. Let me verify this is intentional v4 syntax before making changes. If data-disabled: is valid v4 syntax (similar to how max-h-() is), then this is correct and no fix needed. |
|
Re: OfflineSettings.tsx line 246 Checking if min-w-25 is valid Tailwind v4 syntax. In v4, arbitrary spacing scale might support this. Will verify against v4 docs before changing. If min-w-25 is not supported in v4, will change to min-w-[100px]. |
|
Re: backend/src/routes/logs.ts line 46 Good catch. The source field is optional in the schema but accessed directly in the logging code. However, checking the actual usage:
This is actually safe - Pino will serialize undefined as null in JSON. But for clarity, we could add optional chaining or make source required. Current behavior is acceptable (logs undefined as null), but if you prefer explicit handling, I can add: source: log.source ?? 'unknown' |
- Import useAuthStore in logger-client.ts - Add getUserId() method to collect current user ID - Include userId in LogEntry interface and submission - Remove unnecessary window.__authStore exposure
|
Addressed all 6 Copilot review issues in commit 86c5a36 + upcoming commit:
|
- backend: use conditional spread for optional source field - backend: allow raw field to accept string or object in schema - frontend: keep warn() errors in raw field (warn has no err param) - frontend: replace double type assertion with raw field in cache-sync-service
- Change PlaylistDetailPage error → warn (store handles, no throw) - Remove all meaningless 'traceId: undefined' (18 occurrences) - traceId is optional, no need to explicitly pass undefined
|
Copilot Review Response: Issue 1 (logs.ts auth/rate limiting): Rate limiting should be implemented at infrastructure layer (K8s Gateway). Will add runtime env vars to control log collection (disabled by default). Tracked in separate issue for Epic 3.6. Issue 2 (playlistStore.ts line 317): Intentional. This is a business logic error (Favorites not found), not a caught exception. Creating Error inline provides stack trace for debugging. This is acceptable practice. Issues 3 & 4 (PlaylistDetailPage undefined params): Fixed in commit 7305538. Changed to logger.warn() since store layer handles errors without throwing. Also removed all 18 occurrences of meaningless 'traceId: undefined' across codebase. Issue 5 (privacy concerns): Valid for multi-region deployment. Will add user consent checkbox during login flow. Created issue to track implementation. |
|
Re: PlaylistDetailPage.tsx line 244 (undefined params) Fixed in commit 7305538. Changed to logger.warn() since store layer handles errors without throwing. Also removed all 18 occurrences of meaningless 'traceId: undefined' across codebase. |
|
Re: logger-client.ts line 75 (privacy concerns) Valid concern for multi-region deployment. Created issue #266 to track implementation:
For self-hosted deployments, users control their data. But we should still provide transparency and choice. |
- Fix raw field usage in 4 locations (player, events, StorageManager, cache-sw-bridge) - Change ENABLE_REMOTE_LOGGING from compile-time to runtime injection via window.__ENABLE_REMOTE_LOGGING__ - Remove unnecessary user-facing UI controls (logging should not be user-controlled) - Add proper Error object wrapping for string errors - Add type guards for void/primitive payload in EventBus
- Backend: Add ENABLE_REMOTE_LOGGING env var in logger.ts (reserved for future use) - Frontend: Support two-layer config: runtime injection (prod) + env var (dev) - Update .env.example files with clear documentation - Enable convenient local testing without modifying runtime injection code
- logger.warn doesn't have error parameter, only logger.error does - Place error objects in raw field without String() wrapping to preserve error information - Fixes 3 logger.warn calls in persistence.ts
Summary
Implements Epic 3.5 structured logging and observability foundation for M3W.
Changes
Backend (Part of #249)
Note: Backend route migration to new logger format will be done in a separate PR.
Frontend (#250) ✅
Test Coverage
Related Issues
Closes #250
Part of #249 (backend route migration pending)
Part of Epic 3.5 (Production Readiness)
Testing
\\�ash
Backend tests
cd backend && npm test
Frontend tests
cd frontend && npm test
Type check
npx tsc --noEmit
\\
Next Steps