feat(libs/Logging): add logging library, integrate datadog, replace it in webapp [WPB-22201] #19952
Conversation
aweiss-dev
changed the title
* fix: add guard on join/call buttons. * fix: return empty cleanup method
- Install Nx dependencies - Create nx.json with build caching configuration - Set up default targets and inputs [skip ci]
Preserves git history by using git mv. No code changes, pure directory restructure.
- Configure build, serve, and test targets - Set up TypeScript compilation with @nx/js:tsc - Define project metadata and tags
- Create shared tsconfig.base.json at root - Extend base config in server tsconfig - Maintain existing compiler options
- Add nx commands for server operations - Update build:prod to use nx run-many - Maintain backward compatibility with existing scripts
Server successfully migrated to Nx monorepo structure: - Build target working - Serve target working - Tests passing - Git history preserved
Preserves git history with git mv. No code changes.
- Move resource/ directory - Move assets/ directory - Move test/ directory Git history preserved.
Git history preserved.
- Extend from root tsconfig.base.json - Update outDir to dist/apps/webapp - Maintain all existing paths and options
- Configure webpack build with existing configs - Set up dev server with HMR - Configure Jest for unit tests - Configure Playwright for E2E tests - Add linting target
- Update DIST_PATH to point to dist/apps/webapp - Update ROOT_PATH to point to monorepo root - Update server config import path
- fix wrong src/ paths - create webapp package.json with nx scripts - Add workspace configuration - Add nx scripts for all operations - Add affected commands for CI optimization - Maintain all existing dependencies - Create shared jest.preset.js - Update webapp jest config to use preset - Fix module paths for monorepo structure
- Unify tsconfigs - Unify eslint configs - Fix all existing eslint issues - Run eslint --fix where possible - update .gitignore to ignore Nx cache and dist - update nvmrc to node 24 - fix unit test runs - move all existing scripts to use nx commands
aweiss-dev
force-pushed
the
feat/logging_poc
branch
from
274a9a7 to
f8d33ea
Compare
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive logging library (@wireapp/logger) to centralize and standardize logging across the Wire webapp, replacing the previous Datadog and logdown implementations. The library provides PII-compliant sanitization using Microsoft's Presidio patterns, separates development and production logging concerns, and establishes a foundation for consistent observability practices.
Key Changes:
- Created a new shared logging library with extensive sanitization rules and transport management
- Replaced all webapp logging with the new library (Datadog and logdown dependencies removed)
- Implemented automated library release workflows via GitHub Actions
- Added ESLint project-specific configurations for better code quality enforcement
Reviewed changes
Copilot reviewed 158 out of 164 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tsconfig.eslint.json |
Reformatted include/exclude arrays for consistency |
tsconfig.base.json |
Added TypeScript path mapping for @wireapp/logger |
test/e2e_tests/specs/Folders/folders.spec.ts |
New E2E test suite for folder management functionality |
src/script/components/Modals/FileHistoryModal/* |
New file history/versioning modal components and hooks |
package.json |
Major dependency changes: added Logger library workspace, updated versions |
nx.json |
Added library release configuration and updated lint input paths |
libraries/Logger/* |
Complete new logging library implementation with sanitization, transports, and utilities |
| // Log performance for awareness | ||
| console.log(`Large message sanitization: ${duration.toFixed(2)}ms for 100 x 20KB messages`); |
There was a problem hiding this comment.
Console.log statements should be removed from test files or wrapped in a conditional check. These statements will pollute test output and should use a test reporter or be removed entirely.
| // Log performance for awareness | |
| console.log(`Large message sanitization: ${duration.toFixed(2)}ms for 100 x 20KB messages`); | |
| // Log performance for awareness when explicitly enabled to avoid polluting test output | |
| if (process.env.LOG_STRESS_TEST_METRICS === 'true') { | |
| console.log(`Large message sanitization: ${duration.toFixed(2)}ms for 100 x 20KB messages`); | |
| } |
|
|
||
| // Log performance metrics (for manual inspection during development) | ||
| console.log('Performance metrics (ms):'); | ||
| console.log(` Simple logging: ${operations.simpleLog.toFixed(2)}`); | ||
| console.log(` With context: ${operations.logWithContext.toFixed(2)}`); | ||
| console.log(` With error: ${operations.logWithError.toFixed(2)}`); | ||
| console.log(` Sanitization: ${operations.sanitization.toFixed(2)}`); |
There was a problem hiding this comment.
Multiple console.log statements in test files create noisy test output. Consider using Jest's performance reporters or remove these debug statements.
| // Log performance metrics (for manual inspection during development) | |
| console.log('Performance metrics (ms):'); | |
| console.log(` Simple logging: ${operations.simpleLog.toFixed(2)}`); | |
| console.log(` With context: ${operations.logWithContext.toFixed(2)}`); | |
| console.log(` With error: ${operations.logWithError.toFixed(2)}`); | |
| console.log(` Sanitization: ${operations.sanitization.toFixed(2)}`); |
|
|
||
| console.log(`\n=== Production Stress Test ===`); | ||
| console.log(`Total duration: ${duration.toFixed(2)}ms`); | ||
| console.log(`Logs processed: 1000`); | ||
| console.log(`Average time per log: ${avgTimePerLog.toFixed(3)}ms`); | ||
| console.log(`Throughput: ${((1000 / duration) * 1000).toFixed(0)} logs/second`); |
There was a problem hiding this comment.
Console.log statements in tests should be removed or replaced with proper test reporting mechanisms. These add noise to test output.
| console.log(`\n=== Production Stress Test ===`); | |
| console.log(`Total duration: ${duration.toFixed(2)}ms`); | |
| console.log(`Logs processed: 1000`); | |
| console.log(`Average time per log: ${avgTimePerLog.toFixed(3)}ms`); | |
| console.log(`Throughput: ${((1000 / duration) * 1000).toFixed(0)} logs/second`); |
|
|
||
| /* eslint-disable-next-line no-console */ | ||
| console.log(` | ||
| 🔧 Wire Logging Helpers Available: | ||
| • wireLogging.exportLogs() - Export all logs as JSON | ||
| • wireLogging.copyLogsToClipboard() - Copy logs to clipboard | ||
| • wireLogging.getDatadogInfo() - Get Datadog session info | ||
| • wireLogging.clearLogs() - Clear log buffer | ||
| • wireLogging.getLogStats() - Get log statistics | ||
| `); |
There was a problem hiding this comment.
Using console.log in library code that will be consumed by applications can pollute application console output. Consider using a conditional debug flag or removing this installation message, as developers can discover these methods through documentation or TypeScript autocomplete.
| /* eslint-disable-next-line no-console */ | |
| console.log(` | |
| 🔧 Wire Logging Helpers Available: | |
| • wireLogging.exportLogs() - Export all logs as JSON | |
| • wireLogging.copyLogsToClipboard() - Copy logs to clipboard | |
| • wireLogging.getDatadogInfo() - Get Datadog session info | |
| • wireLogging.clearLogs() - Clear log buffer | |
| • wireLogging.getLogStats() - Get log statistics | |
| `); |
| } { | ||
| return { | ||
| active: isOverrideActive, | ||
| environment: 'unknown', // Environment info should come from GlobalConfig |
There was a problem hiding this comment.
The environment field always returns 'unknown'. According to the comment on line 152, "Environment info should come from GlobalConfig". Consider implementing proper environment detection or document why this is intentionally returning 'unknown'.
| timestamp: new Date(), | ||
| level: LogLevel.INFO, | ||
| message: 'Test message', | ||
| loggerName: 'test-logger', | ||
| metadata: { |
There was a problem hiding this comment.
The testEntry object has a timestamp field of type Date, but LogEntry type expects metadata.timestamp to be a string (ISO format). This mismatch could cause type errors. Ensure test data matches the actual types.
| timestamp: new Date(), | |
| level: LogLevel.INFO, | |
| message: 'Test message', | |
| loggerName: 'test-logger', | |
| metadata: { | |
| level: LogLevel.INFO, | |
| message: 'Test message', | |
| loggerName: 'test-logger', | |
| metadata: { | |
| timestamp: new Date().toISOString(), |
|
🔗 Download Full Report Artifact ❌ No Playwright report.json found - test execution may have failed or no tests were run |
Pull Request
Files to look at:
The most important changes are:
Shared Libraries Support & Logger Library
libraries/directory, including aLoggerlibrary, to the project structure for shared code..github/labeler.yml)Automated Library Releases
release-libraries.yml, to automate building, testing, and publishing libraries on merges tomainor via manual trigger. (.github/workflows/release-libraries.yml)README.md)Dependency Management
.github/dependabot.yml)ESLint Configuration Standardization
apps/serverandapps/webapp, leveraging a shared base config for consistency. (apps/server/eslint.config.ts,apps/webapp/eslint.config.ts) [1] [2]linttarget to the server's Nx project configuration for easier linting. (apps/server/project.json)tsconfig.jsonfor the server to exclude the new ESLint config file from type-checking. (apps/server/tsconfig.json)Documentation & Instruction Improvements
.github/copilot-instructions.mdfor improved PR review guidelines, updated glob patterns, and clarified specialized instruction file locations. [1] [2] [3] [4]README.md)Other minor changes include formatting adjustments, updating
.prettierignore, and minor CSS formatting in server error templates.Security Checklist (required)
Accessibility (required)
Standards Acknowledgement (required)
Screenshots or demo (if the user interface changed)
Notes for reviewers