-
Notifications
You must be signed in to change notification settings - Fork 89
Refactor/log #1025
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: naga
Are you sure you want to change the base?
Refactor/log #1025
Conversation
// BEFORE
process.env.LOG_LEVEL = 'debug2';
logger.debug2('verbose console output');
// AFTER
process.env.LOG_LEVEL = 'debug_text'; // clearer name; debug2 still works
logger.debugText('verbose console output');
Signed-off-by: Anson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR centralizes logging across the Lit Protocol SDK by introducing a new @lit-protocol/logger package and consolidating CURL debugging functionality for easier request replay during development and debugging.
Key Changes
- Implemented centralized structured logging with
pinobackend supporting both Node.js and browser environments - Added support for custom log transports (DataDog, Sentry, OpenTelemetry, etc.)
- Consolidated CURL debugging helpers from multiple locations into
@lit-protocol/logger - Migrated console.log/console.error calls to structured logging throughout runtime library code
- Added CLI tool (
pnpm debug:curl) to retrieve stored curl commands by request ID
Reviewed changes
Copilot reviewed 43 out of 45 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/debug-curl.js | New CLI tool for retrieving curl commands by request ID with fuzzy matching support |
| packages/logger/src/lib/curlDebug.ts | Centralized curl command generation and file writing utilities for Node.js |
| packages/logger/src/lib/logger.ts | Core logger implementation with pino backend, custom transports, and debug_text mode |
| packages/logger/src/lib/logger.spec.ts | Updated tests to validate new logger functionality |
| packages/logger/README.md | Comprehensive documentation with usage examples including DataDog and OpenTelemetry |
| packages/networks/src/networks/shared/logger.ts | Refactored to re-export from centralized logger package |
| packages/wrapped-keys/src/lib/service-client/utils.ts | Integrated curl debugging for wrapped keys service requests |
| packages/lit-client/src/lib/LitNodeClient/LitNodeApi/src/helper/sendNodeRequest.ts | Integrated curl debugging for Lit node requests |
| packages/crypto/src/lib/misc.ts | Migrated logging functions to use centralized logger |
| packages/auth/src/lib/storage/*.ts | Replaced console logging with structured logger |
| packages/auth/src/lib/authenticators/**/*.ts | Replaced console.error with structured logger for Stytch authenticators |
| packages/networks/src/networks/vNaga/**/*.ts | Removed debug console.log statements and replaced with structured logging |
| docs/guides/debugging-node-requests.mdx | New guide documenting curl debugging workflow |
| package.json | Added debug:curl script |
| .gitignore | Added debug directory to ignore list |
| pnpm-lock.yaml | Updated dependencies, removed direct pino dependencies from multiple packages |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const headers = headerEntries | ||
| .map(([key, value]) => `-H "${key}: ${value}"`) | ||
| .join(' '); | ||
|
|
||
| const body = typeof init.body === 'string' ? `--data '${init.body}'` : ''; | ||
|
|
||
| return `curl -X ${method} ${headers} ${body} "${url}"`.trim(); |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The curl command generation does not escape special characters in header values and request body. If header values or the body contain single quotes, double quotes, or shell metacharacters, the generated curl command could be malformed or potentially execute unintended commands when copied to a shell.
Consider properly escaping the values:
- Header values should escape double quotes and backslashes
- The body should escape single quotes and backslashes when using single-quoted strings
- Alternatively, consider using safer shell escaping libraries or base64 encoding for complex payloads
| const filePath = path.join(dirPath, safeRequestId); | ||
| const idHeaderName = params.idHeaderName ?? 'request-id'; | ||
|
|
||
| await fs.mkdir(path.dirname(filePath), { recursive: true }); |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When creating the directory for the debug file, only the parent directory is created. If filePath points to just dirPath/safeRequestId (a file directly in the debug directory), then path.dirname(filePath) returns dirPath. However, if there are subdirectories in the sanitized request ID (which shouldn't happen with the current sanitization), this logic assumes the parent exists.
While the current sanitization replaces all special characters with underscores, making subdirectories impossible, it would be safer to create the full dirPath instead of path.dirname(filePath) for clarity and to avoid potential issues if sanitization logic changes in the future.
| await fs.mkdir(path.dirname(filePath), { recursive: true }); | |
| await fs.mkdir(dirPath, { recursive: true }); |
| if (typeof first === 'string') { | ||
| if (second && typeof second === 'object') { | ||
| return { msg: first, data: second }; | ||
| } | ||
| return { msg: first }; | ||
| } |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extractMsgAndData function doesn't follow the standard pino logging signature correctly. In pino, the first argument should be an object (for structured data) and the second should be a message string. The current implementation reverses this order in line 175-179, treating a string first argument as the message and an object second argument as data.
This is the opposite of pino's signature: logger.info({ data }, 'message'). While this may be intentional to support console.log-style calls, it creates confusion and inconsistency with pino's conventions. The code should either follow pino's standard signature or clearly document this deviation.
| typeof moduleName === 'string' ? `${name}:${moduleName}` : name; | ||
| return createConsoleLogger(childName); | ||
| }, | ||
| isLevelEnabled: () => true, |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The console logger's isLevelEnabled method always returns true regardless of the configured log level. This means when using debug_text mode, all log levels will be output even if a higher threshold is configured.
The method should check against the configured level, similar to the shouldLog function used for transport-based logging. This inconsistency could cause unexpected log output when users expect silent or higher log levels to filter messages.
1. Centralized logging
WHAT
pinologging, but you can attach custom transports (DataDog, Sentry, your own system) and it works in both Node.js and browsers.import { getChildLogger } from '@lit-protocol/logger';instead ofconsole.log.USAGE
Centralized logging for the Lit Protocol SDK. The default backend is structured
pinologging, but you can attach custom transports (DataDog, Sentry, your own system) and it works in both Node.js and browsers.Basic usage
Log levels
Logging verbosity is controlled by:
process.env.LOG_LEVELglobalThis.LOG_LEVELSupported levels:
silent,fatal,error,warn,info,debug,trace,debug_text.debug_textswitches the default output to console-style text (not JSON).debug2is a deprecated alias fordebug_text.Configuration
Use
setLoggerOptionsat app startup to change level/name or add metadata:2. Consolidate CURL debugging
WHAT
@lit-protocol/loggerand point both Lit node and Wrapped Keys request paths at the shared implementation, preserving the opt-inLIT_DEBUG_CURLbehavior while eliminating duplicated fs/path/env logic.USAGE
After your run, copy the correlation id (for example
X-Request-Idfrom node calls, orx-correlation-id/ theRequest(<id>)value from Wrapped Keys errors) and print the stored curl command:If you only have a prefix/substring,
pnpm debug:curlwill try to match it (and will list matches if more than one file fits).