Repository navigation
Cli logging improvements - #445
Merged
Merged
Conversation
Contributor
## Walkthrough
The changes adjust logging behaviors and error handling across several modules. Logging levels for missing files and configuration issues are reduced from warnings to debug, and certain log messages are removed or refined. The `load_yaml_file` utility now accepts a parameter to suppress warnings for missing files. Error messages in toolset prerequisite checks are made more descriptive.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------|
| holmes/config.py | Suppressed warnings for missing model files, adjusted logging to only report non-empty model lists, removed toolset status logs, and changed missing custom toolset config log level to debug. |
| holmes/core/tools.py | Added comment about private attributes in `Toolset`, improved error messages and logging in `check_prerequisites`, refined control flow to avoid premature returns. |
| holmes/plugins/toolsets/grafana/base_grafana_toolset.py | Changed log level for missing Grafana configuration from warning to debug. |
| holmes/plugins/toolsets/prometheus/prometheus.py | Simplified error messages for connection failures in `_is_healthy`, removed exception details from one error log. |
| holmes/utils/file_utils.py | Added `warn_not_found` parameter to `load_yaml_file` to control warning logs for missing files. |
| tests/test_holmes_sync_toolsets.py | Removed assertion checking for specific substring in error message of failed prerequisites test. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Config
participant FileUtils
Config->>FileUtils: load_yaml_file(path, raise_error, warn_not_found)
alt File not found and warn_not_found=True
FileUtils->>Config: Log warning
else File not found and warn_not_found=False
FileUtils->>Config: No warning
end
FileUtils-->>Config: Return dict or raise errorsequenceDiagram
participant Toolset
participant Logger
Toolset->>Toolset: check_prerequisites()
Toolset->>Logger: Log status ENABLED
alt Prerequisite fails
Toolset->>Logger: Log error with specific command/env
Toolset->>Toolset: Set status DISABLED or FAILED
Toolset-->>Toolset: Return status
else All prerequisites pass
Toolset->>Logger: Log info "Toolset enabled"
Toolset-->>Toolset: Return status ENABLED
end
Suggested reviewers
|
aantn
marked this pull request as ready for review
May 23, 2025 22:17
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
holmes/core/tools.py (2)
423-427: Improved error messages with specific command details.The error messages now include the actual command that failed, making debugging much easier. However, consider also including the stderr output from failed commands for even better diagnostics.
Consider enhancing the error message further by including stderr:
except subprocess.CalledProcessError as e: self._status = ToolsetStatusEnum.FAILED - self._error = f"`{prereq.command}` returned {e.returncode}" + self._error = f"`{prereq.command}` returned {e.returncode}. Error: {e.stderr.strip() if e.stderr else 'No error output'}"
441-444: Clarify the CallablePrerequisite error handling logic.The current logic allows setting an error message even when the prerequisite doesn't fail (
enabled=Truebuterror_messageis provided). This could be confusing - is this intended for warnings, or should the prerequisite fail if there's an error message?Consider making the intent clearer:
elif isinstance(prereq, CallablePrerequisite): (enabled, error_message) = prereq.callable(self.config) if not enabled: self._status = ToolsetStatusEnum.FAILED - if error_message: - self._error = f"{error_message}" + self._error = f"{error_message}" if error_message else "Callable prerequisite check failed" + elif error_message: + # Log warning but don't fail the prerequisite + logging.warning(f"Toolset {self.name} prerequisite warning: {error_message}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
holmes/config.py(3 hunks)holmes/core/tools.py(3 hunks)holmes/plugins/toolsets/grafana/base_grafana_toolset.py(1 hunks)holmes/plugins/toolsets/prometheus/prometheus.py(1 hunks)holmes/utils/file_utils.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
holmes/config.py (1)
holmes/utils/file_utils.py (1)
load_yaml_file(19-56)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.9)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.9)
- GitHub Check: build (3.12)
🔇 Additional comments (12)
holmes/plugins/toolsets/grafana/base_grafana_toolset.py (1)
40-40: LGTM! Appropriate log level adjustment.Changing from
warningtodebuglevel for missing configuration is appropriate since this is expected behavior when the Grafana toolset is not configured, rather than an error condition.holmes/config.py (3)
155-155: LGTM! Appropriate warning suppression.Adding
warn_not_found=Falsealigns with the newload_yaml_fileparameter to suppress warnings for missing model files, which is expected behavior when the models file doesn't exist.
226-227: LGTM! Conditional logging improvement.Only logging loaded models when
self._model_listis not empty prevents unnecessary log noise when no models are configured.
582-584: LGTM! Consistent log level adjustment.Changing from
warningtodebuglevel for missing custom toolset files is consistent with similar changes across the codebase and appropriate since this is expected behavior when custom toolsets are not configured.holmes/utils/file_utils.py (2)
19-21: LGTM! Well-designed parameter addition.Adding the
warn_not_foundparameter with a default value ofTruemaintains backward compatibility while allowing callers to suppress warnings when missing files are expected behavior.
31-32: LGTM! Proper conditional warning implementation.The conditional warning based on the
warn_not_foundflag is correctly implemented and supports the logging improvements throughout the codebase.holmes/plugins/toolsets/prometheus/prometheus.py (2)
797-801: LGTM! Simplified error handling and messaging.Removing the unused exception parameter and simplifying the error message improves readability and consistency. The essential information (failed initialization with URL) is preserved while reducing verbosity.
805-805: LGTM! Consistent error message format.Removing the "Toolset" prefix makes the error message format consistent with the RequestException handling above and improves clarity.
holmes/core/tools.py (4)
345-346: Excellent documentation improvement.This warning comment is very helpful as it alerts developers to a subtle but important behavior of Pydantic's
PrivateAttrfields. This can prevent hard-to-debug issues when toolsets are extended or copied.
404-404: Good practice to explicitly set initial status.Setting the status to
ENABLEDat the start makes the method's behavior more predictable and self-contained, rather than relying on the default private attribute value.
432-432: Improved error message specificity.Including the specific environment variable name in the error message is a great improvement for debugging configuration issues.
446-454: Improved control flow and logging.The new logging approach provides clear feedback about toolset status, and the early return prevents unnecessary prerequisite checking once one fails. This aligns well with the PR's goal of improving CLI logging clarity.
The emoji-based success/failure indicators (✅/❌) make the output more user-friendly and easier to scan.
aantn
commented
May 23, 2025
arikalon1
previously approved these changes
May 24, 2025
arikalon1
left a comment
arikalon1
left a comment
Collaborator
There was a problem hiding this comment.
great improvement, thank you
arikalon1
previously approved these changes
May 24, 2025
arikalon1
approved these changes
May 24, 2025
Contributor
4 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This cleans up cli logs a bit. It involves some flow changes as well to toolset configuration, so it should be carefully reviewed and tested. I did a sanity check on the cli, but did not check the in-cluster server behaviour.
Summary by CodeRabbit