Skip to content

Add a seperate logger for worker processes#87

Open
tanuj-shardeum wants to merge 1 commit intodevfrom
BLUE-279
Open

Add a seperate logger for worker processes#87
tanuj-shardeum wants to merge 1 commit intodevfrom
BLUE-279

Conversation

@tanuj-shardeum
Copy link
Contributor

@tanuj-shardeum tanuj-shardeum commented Sep 24, 2024

No description provided.

@tanuj-shardeum tanuj-shardeum self-assigned this Sep 24, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Sep 24, 2024

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Logger Initialization
The workerProcessLogger is initialized in initLogger function but there is no check to ensure that the logger is correctly configured or handling of potential errors during its initialization.

Logging Level
The logging level and the type of messages logged (info, debug, error) should be reviewed to ensure they are appropriate for their use cases to avoid verbose logging in production.

Error Handling
There are multiple instances where errors are logged but not handled. For example, when no worker is available, the error is logged but the process continues without handling the absence of a worker.

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 24, 2024
View reviewed changes
src/primary-process/index.ts
if (receiptLoadTraker < config.receiptLoadTrakerLimit) {
if (config.workerProcessesDebugLog)
console.log(`Receipt load is below the limit: ${receiptLoadTraker}/${config.receiptLoadTrakerLimit}`)
Logger.workerProcessLogger.debug(`Receipt load is below the limit: ${receiptLoadTraker}/${config.receiptLoadTrakerLimit}`)

Check warning

Code scanning / CodeQL

Log injection

Log entry depends on a [user-provided value](1).

Copilot Autofix

AI over 1 year ago

To fix the log injection issue, we need to sanitize the user-provided values before logging them. Specifically, we should remove any newline characters from the config.receiptLoadTrakerLimit value before it is included in the log message. This can be done using String.prototype.replace to ensure no line endings are present in the user input.

Suggested changeset 1
src/primary-process/index.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/primary-process/index.ts b/src/primary-process/index.ts
--- a/src/primary-process/index.ts
+++ b/src/primary-process/index.ts
@@ -50,4 +50,6 @@
     if (receiptLoadTraker < config.receiptLoadTrakerLimit) {
-      if (config.workerProcessesDebugLog)
-        Logger.workerProcessLogger.debug(`Receipt load is below the limit: ${receiptLoadTraker}/${config.receiptLoadTrakerLimit}`)
+      if (config.workerProcessesDebugLog) {
+        const sanitizedLimit = String(config.receiptLoadTrakerLimit).replace(/\n|\r/g, "");
+        Logger.workerProcessLogger.debug(`Receipt load is below the limit: ${receiptLoadTraker}/${sanitizedLimit}`)
+      }
       // Kill the extra workers from the end of the array
EOF
@@ -50,4 +50,6 @@
if (receiptLoadTraker < config.receiptLoadTrakerLimit) {
if (config.workerProcessesDebugLog)
Logger.workerProcessLogger.debug(`Receipt load is below the limit: ${receiptLoadTraker}/${config.receiptLoadTrakerLimit}`)
if (config.workerProcessesDebugLog) {
const sanitizedLimit = String(config.receiptLoadTrakerLimit).replace(/\n|\r/g, "");
Logger.workerProcessLogger.debug(`Receipt load is below the limit: ${receiptLoadTraker}/${sanitizedLimit}`)
}
// Kill the extra workers from the end of the array
Copilot is powered by AI and may make mistakes. Always verify output.
jairajdev
jairajdev reviewed Sep 26, 2024
View reviewed changes
archiver-log.json
"saveConsoleOutput": true,
"dir": "archiver-logs",
"files": { "main": "", "fatal": "", "net": "" },
"files": { "main": "", "fatal": "", "net": "", "workerProcess": "" },
Copy link
Contributor

@jairajdev jairajdev Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we give the name as workerHandler instead? I think since it's more about the debug logs by the main process handling the worker processes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jairajdev jairajdev jairajdev left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@tanuj-shardeum tanuj-shardeum

Labels

Review effort [1-5]: 3

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tanuj-shardeum @jairajdev