Skip to content
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

chore: move non-relevant logs to debug log level #87

Merged
merged 2 commits into from
Dec 9, 2023
Merged

Conversation

rustatian
Copy link
Member

@rustatian rustatian commented Dec 9, 2023

Reason for This PR

closes: roadrunner-server/roadrunner#1804

Description of Changes

  • Move non-relevant logs to debug log level from warn.
  • CC @SerhiiMova

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]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

Summary by CodeRabbit

  • New Features

    • Enhanced error handling for job reservation timeouts.
    • Context support added to the job statistics retrieval method.
  • Bug Fixes

    • Improved error handling logic in job processing to better manage specific error conditions.
  • Documentation

    • Updated comments for clarity and accuracy in job identification.
  • Refactor

    • Streamlined error handling in job listening with a focus on timeout errors.
  • Tests

    • Updated test configurations to a more verbose logging level for improved debugging.
    • Adjusted test suite to reflect the new application version "2023.3.0".
    • Reduced graceful shutdown timeout in tests for faster test execution.

Signed-off-by: Valery Piashchynski <[email protected]>
@rustatian rustatian added the enhancement New feature or request label Dec 9, 2023
@rustatian rustatian requested a review from wolfy-j December 9, 2023 16:19
@rustatian rustatian self-assigned this Dec 9, 2023
Copy link

coderabbitai bot commented Dec 9, 2023

Walkthrough

The changes involve updates to error handling, logging levels, and method signatures in the beanstalkjobs package, as well as version updates in test configurations. A new timeout error variable is introduced, and the Stats method now accepts a context argument. The test suite reflects a version bump and adjustments to graceful shutdown timeouts, indicating a focus on error management, code clarity, and testing against a more recent software version.

Changes

File(s) Summary
beanstalkjobs/connection.go Introduced errBeanstalkTimeout, refactored error handling with stderr.Join, updated comments, and modified Stats method to accept context.
beanstalkjobs/item.go Updated comment for Ident field to improve clarity.
beanstalkjobs/listen.go Removed import for github.com/beanstalkd/go-beanstalk, refined error handling logic for timeouts.
tests/configs/.rr-beanstalk-init.yaml Changed logging level from info to debug.
tests/jobs_beanstalk_..._test.go Updated version from "2.9.0" to "2023.3.0" and adjusted GracefulShutdownTimeout in test initializations.

🐇✨
A rabbit hopped through the code, with care,
Tweaking, testing, refining with flair.
Errors now caught with a much finer net,
A hop, a skip, in versioned steps, all set. 🌟
🐇✨


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between fcd1771 and 4c845c3.
Files ignored due to filter (3)
  • go.work.sum
  • tests/go.mod
  • tests/go.sum
Files selected for processing (6)
  • beanstalkjobs/connection.go (6 hunks)
  • beanstalkjobs/item.go (1 hunks)
  • beanstalkjobs/listen.go (2 hunks)
  • tests/configs/.rr-beanstalk-init.yaml (1 hunks)
  • tests/jobs_beanstalk_durability_test.go (1 hunks)
  • tests/jobs_beanstalk_test.go (5 hunks)
Files skipped from review due to trivial changes (1)
  • beanstalkjobs/item.go
Additional comments: 14
beanstalkjobs/connection.go (5)
  • 17-17: The introduction of errBeanstalkTimeout is consistent with the PR's objective to standardize error handling and reduce verbosity in logs.

  • 98-101: The change from errors.Errorf to stderr.Join in the Reserve method is a refinement that aligns with the PR's goal to make error handling less verbose.

  • 129-129: The Stats method now accepts a context as an argument, which is a change in the method signature. This change should be verified across the codebase to ensure that all calls to this method have been updated accordingly.


The verification process did not find any instances of the old or new method signatures for Stats in the codebase. This could mean that the method is not used elsewhere or that the search was not comprehensive enough to find the usage. However, based on the information provided, there are no indications of issues with the change to the Stats method signature.

Final review comment:

The change to the Stats method signature to include a context parameter has been made, and verification did not reveal any instances of the old or new signatures, suggesting that there may not be any direct usage of this method in the codebase or that the search was not exhaustive. It is recommended to ensure that all calls to this method, if any, are updated to match the new signature.

  • 238-246: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [210-245]

The refactoring of the checkAndRedial method with a switch statement and the handling of specific error conditions is an improvement that makes the error handling logic clearer and more maintainable.

  • 243-243: The handling of the errBeanstalkTimeout error in the checkAndRedial method is a good use of the newly introduced variable and aligns with the PR's objective to standardize error handling.
beanstalkjobs/listen.go (1)
  • 20-31: The changes in error handling align with the PR's objective to reduce log verbosity by handling errBeanstalkTimeout more succinctly and continuing the loop without logging it as an error. This should make the logs less verbose by not logging expected timeout conditions as errors.
tests/configs/.rr-beanstalk-init.yaml (1)
  • 13-18: The change in the logging level from info to debug is consistent with the PR's objective to reduce log verbosity and is correctly implemented in the configuration file.
tests/jobs_beanstalk_durability_test.go (1)
  • 39-39: The version number has been updated to "2023.3.0", which should reflect the new version of the configuration being tested. Ensure that this version is consistent with the intended test environment and that any related documentation or dependencies are also updated accordingly.
tests/jobs_beanstalk_test.go (6)
  • 41-58: The GracefulShutdownTimeout has been significantly reduced from 60 seconds to 2 seconds in the TestBeanstalkInit function. This change could potentially affect the behavior of the system during shutdown in test scenarios. Ensure that this new timeout value is sufficient for all resources to be released properly during the tests.

  • 44-47: The version number in the configuration has been updated from "2.9.0" to "2023.3.0". This change should be reflected in any documentation or other test cases that rely on the version number for consistency.

  • 381-387: The version number in the configuration has been updated from "2.9.0" to "2023.3.0" in the TestBeanstalkStats function. Ensure that this change is consistent with the intended versioning strategy and that all references to the version number are updated accordingly.

  • 498-504: The version number in the configuration has been updated from "2.9.0" to "2023.3.0" in the TestBeanstalkDeclare function. Verify that this version update is accurately represented in all related documentation and configuration files.

  • 578-584: The version number in the configuration has been updated from "2.9.0" to "2023.3.0" in the TestBeanstalkJobsError function. Confirm that this version change is properly synchronized across all test cases and documentation where the version number is mentioned.

  • 659-665: The version number in the configuration has been updated from "2.9.0" to "2023.3.0" in the TestBeanstalkNoGlobalSection function. As with the other test functions, ensure that this version update is consistent with the project's versioning policy and that all references to the version number are updated.

@@ -118,13 +120,13 @@ func (cp *ConnPool) Delete(_ context.Context, id uint64) error {
return errors.Errorf("err: %s\nerr redial: %s", err, errN)
Copy link

Choose a reason for hiding this comment

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

The use of errors.Errorf in the Delete method seems inconsistent with the PR's objective to reduce verbosity. Consider using stderr.Join or a similar approach to align with the changes made in the Reserve method.

Signed-off-by: Valery Piashchynski <[email protected]>
@rustatian rustatian changed the title chore: less verbose debug logs chore: move non-relevant logs to debug log level Dec 9, 2023
@rustatian rustatian merged commit 91e51ed into master Dec 9, 2023
8 checks passed
@rustatian rustatian deleted the chore/logs branch December 9, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🧹 CHORE]: Beanstalk jobs driver errors log
1 participant