Improve coordinator logs and extend structured logger #1389
Conversation
🦋 Changeset detectedLatest commit: fc25131 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request focus on enhancing the structured logging capabilities within the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
nicktrn
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
.changeset/proud-yaks-thank.md (1)
1-5: LGTM! Consider adding more details to the description.The changeset file is correctly formatted and appropriately marks this as a patch update. The description aligns with the PR objectives and summary.
To provide more context for future reference, consider expanding the description slightly. For example:
-Improvements to structured logger and conditional payload logging +Improvements to structured logger and conditional payload logging: +- Enhanced structured logging capabilities +- Introduced conditional logging of full payloads in socket handlers +- Extended structured logger API to support additional fieldspackages/core/src/v3/utils/structuredLogger.ts (1)
78-82: LGTM: Improved log structure with a minor suggestionThe changes to the
#structuredLogmethod enhance the structure of the logged object:
- Prefixing
nameandlevelwith$helps distinguish them as metadata fields.- Placing
timestampfirst in the log object is a good practice for chronological sorting.- The modification to args spreading handles the single argument case more explicitly.
These improvements align well with the PR objective of enhancing coordinator logging.
Consider adding a comment explaining the significance of the
$prefix for$nameand$level. This would improve code readability and maintain consistency if this convention is used elsewhere. For example:const structuredLog = { timestamp: new Date(), // Prefix metadata fields with $ for distinction $name: this.name, message, $level: level, ...(args.length === 1 ? args[0] : args), ...this.fields, };packages/core/src/v3/zodNamespace.ts (1)
44-44: LGTM! Consider adding documentation for the new property.The addition of the
logHandlerPayloadsproperty to theZodNamespaceOptionsinterface and its usage in the constructor are correct and align with the PR objectives. This change allows for conditional logging of handler payloads, which can be useful for debugging and monitoring purposes.Consider adding a brief comment above the
logHandlerPayloadsproperty in the interface to explain its purpose and impact. For example:/** * When set to true, enables logging of handler payloads. * This can be useful for debugging but may increase log verbosity. */ logHandlerPayloads?: boolean;This documentation will help other developers understand the purpose and implications of using this option.
Also applies to: 100-100
packages/core/src/v3/zodSocket.ts (3)
71-71: LGTM! Consider adding JSDoc comment for clarity.The addition of the
logPayloadsoption is a good improvement for configurable logging. It allows users to control whether payloads are logged, which can be crucial for debugging and security purposes.Consider adding a JSDoc comment to explain the purpose and impact of this option:
/** * If true, log full payloads of incoming messages. * Use with caution as it may log sensitive information. */ logPayloads?: boolean;
103-103: LGTM! Consider simplifying the boolean conversion.The initialization of
#logPayloadsis well-implemented, providing multiple levels of configuration. The use of an environment variable as a fallback is a good practice.For clarity, you might consider simplifying the boolean conversion:
this.#logPayloads = options.logPayloads ?? (process.env.LOG_SOCKET_HANDLER_PAYLOADS === 'true') ?? false;This explicitly checks for the string 'true', which might be clearer than relying on truthy/falsy conversion of environment variable strings.
200-202: LGTM! Good implementation of conditional logging.The addition of conditional logging based on
#logPayloadsis a good improvement. It allows for more granular control over logging potentially sensitive information.For clarity, you might consider extracting the logging logic into a separate method:
private logIncomingEvent(eventName: string, message: any, hasCallback: boolean) { const logData = { eventName, ...(this.#logPayloads ? { eventMessage: message } : {}), hasCallback, }; this.#logger.info(`Incoming event ${eventName}`, logData); }Then call this method in
registerHandlers:this.logIncomingEvent(eventName, message, !!callback);This would make the
registerHandlersmethod cleaner and easier to read.apps/coordinator/src/exec.ts (1)
Line range hint
122-122: Typo in log messageThere's a typographical error in the log message:
"initiaized"should be"initialized".Apply this diff to correct the typo:
- this.logger.log("initiaized", { opts }); + this.logger.log("initialized", { opts });apps/coordinator/src/index.ts (2)
395-397: Avoid Variable Shadowing by Renaming the LocalloggerVariableThe
loggervariable is redefined within theonConnectionmethod, which can cause confusion due to shadowing the module-levellogger. To improve clarity, consider renaming the local variable to something likeconnectionLoggerorsocketLogger.Apply this change:
-const logger = new SimpleStructuredLogger("prod-worker", undefined, { +const connectionLogger = new SimpleStructuredLogger("prod-worker", undefined, { socketId: socket.id, });And update references within the method accordingly.
146-146: Ensure Consistent Logging and Error Handling for Missing SocketsThe logging statements for missing sockets use different log levels and messages:
- Line 146:
log.debug("Socket for attempt not found");- Line 238 & 258:
logger.log("Socket for attempt/run not found", { ... });To maintain consistency and proper log level usage, consider standardizing these statements. If the absence of a socket is an unexpected condition, using
log.warnorlog.errormight be more appropriate.Suggested change:
-log.debug("Socket for attempt not found"); +log.warn("Socket for attempt not found");And similarly for other instances.
Also applies to: 238-238, 258-258
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- .changeset/proud-yaks-thank.md (1 hunks)
- apps/coordinator/src/checkpointer.ts (3 hunks)
- apps/coordinator/src/exec.ts (4 hunks)
- apps/coordinator/src/index.ts (21 hunks)
- apps/docker-provider/src/index.ts (1 hunks)
- packages/core/src/v3/utils/structuredLogger.ts (1 hunks)
- packages/core/src/v3/zodNamespace.ts (2 hunks)
- packages/core/src/v3/zodSocket.ts (7 hunks)
🧰 Additional context used
🔇 Additional comments (16)
packages/core/src/v3/utils/structuredLogger.ts (1)
63-68: LGTM: NewaddFieldsmethod enhances logger flexibilityThe addition of the
addFieldsmethod is a valuable enhancement to theSimpleStructuredLoggerclass. It allows for dynamic addition of fields to the logger, which aligns well with the PR objective of extending the structured logger API. The implementation is clean and correctly merges new fields with existing ones.apps/docker-provider/src/index.ts (1)
23-23: Approve change with verification suggestion.The modification to set the default value of
FORCE_CHECKPOINT_SIMULATIONto "false" is a good change. It aligns with the PR objectives by allowing for more realistic behavior by default, potentially improving the accuracy of logs and system behavior.However, this change may significantly impact the system's operation, particularly in environments where the
FORCE_CHECKPOINT_SIMULATIONenvironment variable is not explicitly set.To ensure this change doesn't introduce unexpected behavior, please:
- Verify that all environments where this code will be deployed have the
FORCE_CHECKPOINT_SIMULATIONenvironment variable explicitly set if simulation mode is required.- Test the system thoroughly with this new default to ensure it behaves correctly when using actual checkpointing instead of simulation.
- Monitor the system's performance after deployment, as increased usage of actual checkpointing might have performance implications.
packages/core/src/v3/zodSocket.ts (3)
96-96: LGTM! Good use of private field.The addition of the
#logPayloadsprivate field is a good implementation detail. It appropriately encapsulates the logging configuration within the class.
126-126: LGTM! Improved error logging.The switch from
console.errorto the structured logger is a good improvement. It enhances consistency in logging practices and allows for better integration with logging systems.
Line range hint
1-458: Overall, excellent improvements to logging capabilities.The changes in this file significantly enhance the logging system of the
ZodSocketimplementation. The addition of configurable payload logging and the use of structured logging throughout improve both flexibility and consistency. The changes are well-implemented and consider security aspects by making payload logging optional.A few minor suggestions have been made for further improvements:
- Adding JSDoc comments for new options
- Simplifying boolean conversions
- Extracting some logging logic into separate methods
These suggestions are minor and don't detract from the overall quality of the changes. Great work on improving the logging capabilities!
apps/coordinator/src/checkpointer.ts (4)
11-11: LGTM: Import of SimpleStructuredLogger addedThe addition of
SimpleStructuredLoggerimport aligns with the PR objective of improving structured logging capabilities.
90-90: LGTM: Logger updated to use SimpleStructuredLoggerThe
#loggerproperty is correctly updated to useSimpleStructuredLoggerwith a more descriptive name "checkpointer". This change aligns with the PR objective of converting coordinator logs to a structured format.
141-141: LGTM: Error logging updated to use structured formatThe error logging in the
initmethod has been updated to pass an object containing the error, which aligns with the structured logging approach. This change enhances the error information captured in the logs.
Line range hint
1-624: Overall assessment: Structured logging successfully implementedThe changes in this file successfully implement structured logging in the Checkpointer class, aligning with the PR objectives. The modifications include:
- Importing the SimpleStructuredLogger
- Updating the logger initialization
- Adjusting the error logging format
These changes will improve the clarity and usability of log data from the Checkpointer class.
apps/coordinator/src/exec.ts (7)
1-1: ImportSimpleStructuredLoggerThe addition of
SimpleStructuredLoggerimport ensures that structured logging capabilities are available throughout the file.
21-21: Update logger type inExecOptionsinterfaceThe
loggerproperty inExecOptionshas been updated toSimpleStructuredLogger. Please verify that all implementations ofExecOptionsare updated to pass aSimpleStructuredLoggerinstance to prevent type conflicts.
29-29: Update logger property inExecclassThe
loggerproperty in theExecclass is nowSimpleStructuredLogger, aligning with the structured logging enhancement.
37-37: Initialize logger withSimpleStructuredLoggerinExecThe logger is correctly initialized with
SimpleStructuredLogger, providing a default identifier of"exec"when no logger is supplied.
106-106: Update logger property inBuildahclassThe
loggerproperty in theBuildahclass has been updated toSimpleStructuredLogger, enhancing logging consistency.
223-223: Update logger property inCrictlclassThe
loggerproperty in theCrictlclass is nowSimpleStructuredLogger, aligning it with the structured logging updates.
Line range hint
238-238: Typo in log messageThere's a typographical error in the log message:
"initiaized"should be"initialized".Apply this diff to correct the typo:
- this.logger.log("initiaized", { opts }); + this.logger.log("initialized", { opts });
commit: |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
packages/core/src/v3/utils/structuredLogger.ts (1)
79-82: Approved: Improved structure and handling in#structuredLogThe changes to the
#structuredLogmethod enhance the logging structure and improve argument handling:
- Renaming
nameandlevelto$nameand$levelhelps avoid potential conflicts with user-defined fields.- The new property order (timestamp, $name, $level, fields, args) is more logical and consistent.
- The modified args spreading (
args.length === 1 ? args[0] : args) elegantly handles both single and multiple arguments.These improvements align well with the PR objective of enhancing structured logging capabilities.
Consider extracting the args spreading logic into a separate variable for improved readability:
const argsObject = args.length === 1 ? args[0] : args; const structuredLog = { timestamp: new Date(), $name: this.name, $level: level, ...this.fields, ...argsObject, };This minor refactoring would make the
structuredLogobject construction cleaner and more self-explanatory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- apps/coordinator/src/checkpointer.ts (3 hunks)
- apps/coordinator/src/cleaner.ts (4 hunks)
- apps/coordinator/src/index.ts (44 hunks)
- packages/core/src/v3/utils/structuredLogger.ts (1 hunks)
- packages/core/src/v3/zodNamespace.ts (2 hunks)
- packages/core/src/v3/zodSocket.ts (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/coordinator/src/checkpointer.ts
- packages/core/src/v3/zodNamespace.ts
- packages/core/src/v3/zodSocket.ts
🧰 Additional context used
📓 Learnings (1)
apps/coordinator/src/index.ts (1)
Learnt from: nicktrn PR: triggerdotdev/trigger.dev#1389 File: apps/coordinator/src/index.ts:0-0 Timestamp: 2024-10-08T13:40:56.493Z Learning: Currently, we are focusing on updating `SimpleLogger` to `SimpleStructuredLogger` only in the coordinator files.
🔇 Additional comments (8)
packages/core/src/v3/utils/structuredLogger.ts (2)
63-68: LGTM: NewaddFieldsmethod enhances logger flexibilityThe new
addFieldsmethod is a valuable addition to theSimpleStructuredLoggerclass. It allows for dynamic addition of fields to the logger instance, which aligns well with the PR objective of extending the structured logger capabilities. The implementation is concise and correctly merges new fields with existing ones.
Line range hint
1-87: Summary: Effective enhancements to structured logging capabilitiesThe changes made to the
SimpleStructuredLoggerclass in this file successfully address the PR objectives of improving coordinator logs and extending the structured logger:
- The new
addFieldsmethod allows for dynamic addition of fields, enhancing the logger's flexibility.- Modifications to the
#structuredLogmethod improve the structure of logged objects and handle arguments more effectively.These enhancements will contribute to better clarity and usability of log data, as well as more detailed logging capabilities. The changes are well-implemented and align closely with the stated goals of the pull request.
apps/coordinator/src/index.ts (6)
6-6: Improved logging with SimpleStructuredLoggerThe change from
SimpleLoggertoSimpleStructuredLoggeraligns with the PR objective of enhancing structured logging. This will provide more detailed and easier-to-parse log outputs.Also applies to: 20-20, 33-33
405-429: Improved logging and centralized exit logicThe initialization of a structured logger for each connection and the introduction of the
exitRunfunction improve logging consistency and centralize the exit logic. This makes the code more maintainable and easier to debug.Also applies to: 432-441
537-550: Enhanced event logging with structured formatThe event handlers now use structured logging, which will greatly improve log readability and searchability. The use of the
omitfunction to exclude sensitive or verbose fields is a good practice for managing log content.Also applies to: 602-608, 683-694, 794-803, 818-824, 859-865, 931-937, 1011-1017, 1092-1098, 1122-1128, 1139-1145, 1170-1176, 1182-1188
Line range hint
559-570: Improved error handling and loggingError handling has been enhanced with more contextual information in logs. This will significantly aid in debugging and troubleshooting issues in production.
Also applies to: 617-628, 646-650, 760-773, 888-901, 962-975, 1042-1055, 1114-1114, 1155-1155
1244-1244: Consistent logging in HTTP serverThe HTTP server logging now uses the structured logging approach, maintaining consistency with the rest of the file. This unified approach will make log analysis more straightforward.
Also applies to: 1274-1274
Line range hint
1-1289: Overall improvement in logging and code structureThe changes in this file significantly enhance the logging capabilities by implementing structured logging throughout. This consistent approach will greatly improve debugging, monitoring, and analysis of the application's behavior. The code structure has also been improved, with centralized logic for certain operations like exiting runs. These changes align well with the PR objectives and should lead to better maintainability and observability of the system.
Summary by CodeRabbit
Release Notes
New Features
ZodNamespaceandZodSocketimplementations.Improvements
Checkpointer,Exec,Buildah, andCrictlclasses for better data handling.TaskCoordinatorclass and HTTP server.These changes collectively improve the robustness and maintainability of the logging framework in the application.