feature: separation of application and server output #90
Conversation
Signed-off-by: Plakhotnikov Vladimir <[email protected]>
WalkthroughThe changes involve modifications to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
Signed-off-by: Plakhotnikov Vladimir <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
init.go (1)
21-26: LGTM with a minor suggestion: Update tonewCommandfunctionThe changes to the
newCommandfunction correctly incorporate the newappLogparameter and initialize it in the returnedcommandstruct. This is consistent with the modifications to thecommandstruct.Consider maintaining the order of fields in the struct initialization to match the order in the struct definition for better readability:
return &command{ log: log, + appLog: appLog, cfg: cfg, - appLog: appLog, }plugin.go (1)
59-62: LGTM with a minor suggestionThe initialization of
appLogis well-implemented and aligns with the goal of separating application and server logs.Consider using a constant for the "app" logger name to improve maintainability. For example:
const AppLoggerName = "app" // ... p.appLog = log.NamedLogger(AppLoggerName)This would make it easier to change the name in the future if needed and ensure consistency if it's used elsewhere in the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- init.go (2 hunks)
- plugin.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (6)
init.go (3)
16-18: LGTM: Addition ofappLogfield tocommandstructThe addition of the
appLogfield to thecommandstruct is a good step towards separating application and server output. This change aligns with the PR objectives and is correctly implemented.
Line range hint
1-124: Overall changes look good, verify impact on wider codebaseThe modifications to
init.gosuccessfully implement the separation of application and server output, which aligns with the PR objectives. The core functionality of thecommandstruct and its methods has been preserved while introducing the newappLogfor application-specific logging.To ensure the changes are fully integrated and don't introduce any inconsistencies:
- Verify that all necessary parts of the codebase have been updated to use
appLogwhere appropriate.- Check for any potential impact on existing logging practices or log consumers.
Run the following script to help identify areas that might need attention:
#!/bin/bash # Description: Identify potential areas needing updates for appLog # Expected results: List of files and contexts where logging is used echo "Files potentially needing updates for appLog:" rg -l '\blog\.' --type go | grep -v 'init.go' echo "---" echo "Contexts where 'log' is used (excluding init.go):" rg '\blog\.' -C 2 --type go | grep -v 'init.go'Review the output to ensure all relevant areas have been considered in this change.
72-78: LGTM with discussion: Updates toWritemethodThe changes to the
Writemethod effectively implement the separation of application and server output, which aligns with the PR objectives. The use ofappLogfor application output is appropriate.The comments provide valuable context for the changes and future considerations. However, there's a point that warrants discussion:
Regarding the suggestion to use
Debuglevel for all output:
- Using
Debuglevel could provide more granular control over log verbosity, but it might also hide important application output by default.- Consider the implications on production environments where
Debuglogs might be disabled.To help make an informed decision, let's analyze the current logging practices in the project:
Based on the results of this analysis, we can make a more informed decision about the appropriate log level for application output.
✅ Verification successful
Further Analysis Required: Assess Log Level Distribution
To evaluate the proposal of using the
Debuglevel for application output, we need to understand the current distribution of log levels within the project.[/run_scripts]
Action Required: Re-run Log Level Analysis with Case-Insensitive Search
To accurately count the occurrences of each log level, please execute the following updated script that performs a case-insensitive search:
[/run_scripts]
LGTM with Approval: Logging Level Adjustment Confirmed
The analysis confirms that only
log.Errorinstances are present in the current codebase. Switching the application output toDebuglevel will not obscure any existing logs and allows for more granular control over log verbosity in the future.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze current logging practices # Expected results: Overview of log level usage across the project echo "Analyzing log level usage:" rg -i '\blog\.(debug|info|warn|error|fatal)' --stats echo "---" echo "Checking for configurable log levels:" rg -i 'log.*level' -A 5 -B 5Length of output: 2329
Script:
#!/bin/bash # Description: Count occurrences of each log level in the codebase # Expected results: Number of times each log level (Debug, Info, Warn, Error, Fatal) is used echo "Counting log level usages:" rg -o '\blog\.(debug|info|warn|error|fatal)' --no-filename | sort | uniq -c | sort -nrLength of output: 151
Script:
#!/bin/bash # Description: Count occurrences of each log level in the codebase (case-insensitive) # Expected results: Number of times each log level (Debug, Info, Warn, Error, Fatal) is used echo "Counting log level usages (case-insensitive):" rg -oi '\blog\.(debug|info|warn|error|fatal)' --no-filename | sort | uniq -c | sort -nrLength of output: 208
plugin.go (3)
29-30: LGTM: Addition ofappLogfieldThe addition of the
appLogfield to thePluginstruct is a good step towards separating application and server output. This change aligns well with the PR objectives.
Line range hint
1-191: Overall assessment: Changes look goodThe modifications to
plugin.goconsistently implement the separation of application and server logs, aligning well with the PR objectives. The changes are well-structured and maintain the existing code quality.Key points:
- The
appLogfield is appropriately added to thePluginstruct.- The
Initmethod correctly initializes the new logger.- The
Servemethod is updated to use the new logger in thenewCommandcall.Please address the minor suggestion about using a constant for the logger name and verify the
newCommandfunction signature ininit.goas mentioned in the previous comments.
107-107: LGTM: UpdatednewCommandcallThe modification to include
p.appLogin thenewCommandcall is consistent with the goal of separating application and server logs.To ensure consistency across the codebase, please verify that the
newCommandfunction ininit.gohas been updated to accept this additional logger parameter. Run the following script to check thenewCommandfunction signature:This will help confirm that the changes are properly implemented across both files.
✅ Verification successful
Verification Successful:
newCommandfunction updated correctlyThe
newCommandfunction ininit.gohas been updated to acceptappLogas an additional parameter, aligning with the changes inplugin.go.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the newCommand function signature in init.go # Test: Search for the newCommand function definition rg -A 5 'func newCommand\(' init.goLength of output: 187
|
Added this repo to the @roadrunner-server/contributors team, so, you won't need to do forks. |
rustatian
changed the title
Signed-off-by: Plakhotnikov Vladimir <[email protected]>
Signed-off-by: Plakhotnikov Vladimir <[email protected]>
init.go
Outdated
| b.log.Info(string(data)) | ||
| // All output from the application does not intersect with logs from the Server plugin | ||
| // For example: destroy signal received {"timeout": 60000000000} is not necessary for logging | ||
| b.appLog.Error(string(data)) |
There was a problem hiding this comment.
I do think this'd be confusing to use. Since ERROR is not the same as STDERR. Like, every stderr log message would be treated as an error. And imagine a user's face after seeing tons of strange-looking errors which are not errors.
INFO log level is also not a good choice here, so in the discussion I mentioned, that appropriate solution for that would be to exchange a log severity information between RR and PHP worker similar to what we're doing in the temporal plugin.
There was a problem hiding this comment.
This is a fairly simple task, it is solved out of the box in PHP-FPM, see catch_workers_output.
We just need to separate the application output from the server and nothing more. No plugins or additional settings are needed for this. In essence, with this feature we get rid of the AppLogger plugin. I think this plugin was created precisely because there was no easy way to separate application logs. The level of info is quite suitable.
I still don't understand: what needs to be done as in the "temporal" plugin. The main thing is why?
There was a problem hiding this comment.
If you want to match with catch_workers_output it should be the same option and raw output to the main log (file, stdout or stderr). You can't just change the log to be shown as errors.
AppLogger was not created to handle this case, this is a completely separate feature which was requested by users to solve other needs.
There was a problem hiding this comment.
"this is a completely separate feature" look at this:
AppLogger: send raw logs to app channel from app by RPC.
This feature: send raw logs to app channel from app by stdout/err and config:
logs:
channels:
app:
level: debug
mode: raw
With a little changes, you can do the same thing that the AppLogger plugin does, but much easier: without RPC, plugin, app-logger php package and additional packages for Monolog and other loggers.
Is there any other reason for the AppLogger's existence?
There was a problem hiding this comment.
App logger was requested by PHP devs and advised to include by PHP devs. This plugin won't be removed. For the reasoning, please, use GitHub search, this is an OSS project, all discussions about all features are open for everyone.
Returning to the original issue:
Like you mentioned for the applogger, the same you can achieve with the server logger channel for your proposed change with info log level.
logs:
channels:
server:
level: info
mode: rawYour appLogger info log (in the PR) won't solve any problem here, because it'd be hidden from the user in the same way the current server log is hidden when not configured to show info logs from the server plugin.
There was a problem hiding this comment.
I did not offer to remove the AppLogger plugin, I only showed its optionality. I use it myself and is forced to additionally configure the logger through the plugins. Cool, if you can solve the problem only with the help of configuration without writing code.
I suggest not just add some hidden app channel. I propose an idea that can be implemented in the future release (maybe 2025).
Idea: There is always an app channel and it always comes out of the PHP-processes. It is configured in a certain way out of the box. This behavior is superior and does not scare users.
There was a problem hiding this comment.
I guess this is a good idea, but it should not be connected to the existing logger then. It should be controlled by the similar to FPM options (?), like catch_workers_output and just output to stderr/stdout what the worker sent.
Signed-off-by: Plakhotnikov Vladimir <[email protected]>
|
@Kaspiman Do you have plans to do something in this PR according our last discussion or it can be closed? |
Yep, I just need some free time in real life... |
Reason for This PR
[💡 FEATURE REQUEST]: Improve clarity of worker errors when starting a server
Description of Changes
Separation of logs from the application and plugin for their separate configuration. I left comments on the code.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
Bug Fixes