fix(toolset): fix toolset config is not initialized when check prerequisites is not called #535
Conversation
WalkthroughThis change adds an abstract Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ToolsetManager
participant Toolset
participant Tool
User->>ToolsetManager: Load toolsets
ToolsetManager->>Toolset: Instantiate Toolset
ToolsetManager->>Toolset: Call init_config(config)
ToolsetManager->>Toolset: Call check_prerequisites()
Toolset->>Tool: For each prerequisite, call callable()
Toolset->>ToolsetManager: Prerequisite check result
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (7)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (9)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
104-112:init_server_toolssignature is now incompatible with the newCallablePrerequisiteinvocation
Toolset.check_prerequisiteswas changed to execute callables with no arguments, yetinit_server_toolsstill expectsconfig: dict[str, Any]. This will raise
TypeError: init_server_tools() missing 1 required positional argument: 'config'
as soon as the prerequisite is evaluated.- # used as a CallablePrerequisite, config added for that case. - def init_server_tools(self, config: dict[str, Any]) -> Tuple[bool, str]: + # used as a CallablePrerequisite + def init_server_tools(self) -> Tuple[bool, str]:Also remove the now-unused
configdoc-comment above the method.holmes/core/tools.py (1)
288-290: Type hint is out of sync with runtime behaviour
CallablePrerequisite.callableis still typed asCallable[[dict[str, Any]], …], but callables are now invoked with zero arguments (see line 415). Static analysers / IDEs will flag every compliant callable as type-incompatible.-class CallablePrerequisite(BaseModel): - callable: Callable[[dict[str, Any]], Tuple[bool, str]] +class CallablePrerequisite(BaseModel): + callable: Callable[[], Tuple[bool, str]]holmes/plugins/toolsets/opensearch/opensearch.py (1)
210-229: Config casting & client list accumulation can break subsequent calls
- Casting:
OpenSearchConfig(**self.config)will crash on the second run for
the same reasons mentioned in other toolsets.- Clients:
self.clientsis never cleared; every run appends duplicates.- try: - os_config = OpenSearchConfig(**self.config) + try: + if isinstance(self.config, dict): + self.config = OpenSearchConfig(**self.config) + os_config: OpenSearchConfig = self.config + + # idempotent – start fresh + self.clients.clear()Apply the same
isinstanceguard ininit_config()(247-253).holmes/plugins/toolsets/grafana/base_grafana_toolset.py (1)
34-45: Guard against re-casting config and drop redundant debugSame pattern: after the first
init_config()call,self.configis a
GrafanaConfiginstance; re-casting will fail.- self.init_config() + self.init_config()and in
init_config()(55-57):- self._grafana_config = self.config_class(**self.config) + if isinstance(self.config, dict): + self._grafana_config = self.config_class(**self.config) + else: + self._grafana_config = self.configAlso, the debug message on line 36 may reveal internal toolset names; consider
raising at INFO level only when troubleshooting.holmes/plugins/toolsets/prometheus/prometheus.py (1)
802-814:init_configfails whenself.configis already aPrometheusConfiginstance
PrometheusConfig(**self.config)expects a mapping, butself.config
might already be a parsed model (e.g., after cache reload).
This crashes withTypeError: PrometheusConfig object is not a mapping.- if self.config: - self.config = PrometheusConfig(**self.config) + if self.config: + if isinstance(self.config, PrometheusConfig): + # Already parsed – nothing to do + pass + else: + self.config = PrometheusConfig(**self.config)holmes/plugins/toolsets/newrelic.py (2)
205-218:prerequisites_callable()can explode on the second call
toolset_manager.init_config()is invoked once during load, andprerequisites_callable()callsself.init_config()again.
Ifself.configwas already converted to aNewrelicConfiginstance by the first call, the second call raisesTypeError: NewrelicConfig object is not a mapping(see pylint E1134).Either make
init_config()idempotent or guard against the model instance here.- if not self.config: - return False, "No configuration provided" - - try: - self.init_config() + try: + # Initialise only once – safe even if already initialised + self.init_config() + if not self.config: + return False, "No configuration provided"
223-227:init_config()not idempotent & trips on**self.configWhen
self.configis already aNewrelicConfigobject,NewrelicConfig(**self.config)fails. Make the method accept bothdictandNewrelicConfig, persist the parsed model, and become a no-op on subsequent calls.- def init_config(self): - nr_config = NewrelicConfig(**self.config) - self.nr_account_id = nr_config.nr_account_id - self.nr_api_key = nr_config.nr_api_key + def init_config(self): + """ + Idempotently populate nr_account_id / nr_api_key. + Accepts either a raw dict or an already-parsed NewrelicConfig. + """ + if not self.config: + return + if isinstance(self.config, NewrelicConfig): + nr_config = self.config + else: + nr_config = NewrelicConfig(**self.config) # type: ignore[arg-type] + self.config = nr_config + + self.nr_account_id = nr_config.nr_account_id + self.nr_api_key = nr_config.nr_api_keyholmes/plugins/toolsets/kafka.py (1)
502-537: Duplication &E1134inprerequisites_callable()The entire client-construction block is duplicated in
init_config().
Running both methods back-to-back creates clients twice and still fails ifself.configis an already-parsedKafkaConfig(same**modelissue).Refactor to delegate all heavy lifting to
init_config()and make the latter resilient to repeated invocations.- errors = [] - try: - kafka_config = KafkaConfig(**self.config) - for cluster in kafka_config.kafka_clusters: - ... - return len(self.clients) > 0, "\n".join(errors) + try: + self.init_config() # idempotent + return (len(self.clients) > 0, "")holmes/plugins/toolsets/opensearch/opensearch_utils.py (1)
32-48:prerequisites_callable()repeats config parsing & still suffers from**modelThe logic here mirrors
init_config()and will fail whenself.configis already anOpenSearchIndexConfig. Delegate toinit_config()and rely on the casted model for the health check.- env_url = os.environ.get("OPENSEARCH_LOGS_URL", None) - ... - else: - self.config = OpenSearchIndexConfig(**self.config) - return opensearch_health_check(self.config) + self.init_config() + if not self.config: + return False, "Missing opensearch traces URL. Check your config" + return opensearch_health_check(self.opensearch_config)
🧹 Nitpick comments (6)
holmes/plugins/toolsets/datetime.py (1)
50-51: Consider adding at least a brief doc-string or comment toinit_config
init_configis currently a no-op (pass). Even one-liner guidance (e.g. “# nothing to initialise for now”) helps future maintainers understand that the emptiness is intentional rather than forgotten logic.holmes/plugins/toolsets/robusta/robusta.py (1)
237-238: Optional doc-string forinit_configSame remark as for other toolsets: a tiny comment stating “nothing to initialise” clarifies intent.
holmes/plugins/toolsets/internet/internet.py (1)
258-261: Guard against double-initialisationIf
init_config()is invoked twice (e.g. once byToolsetManagerand again from
prerequisites_callable), the second call re-readsself.configbut does not
check whetherself.configis already a concrete object instead of a mapping.
While harmless here (dict.get()exists), mirroring the pattern used in other
toolsets keeps behaviour consistent:- if self.config: - self.additional_headers = self.config.get("additional_headers", {}) + if isinstance(self.config, dict): + self.additional_headers = self.config.get("additional_headers", {})holmes/plugins/toolsets/bash/bash_toolset.py (1)
202-205:prerequisites_callableshould rely solely oninit_config()Because
ToolsetManageralready callsinit_config()on load, calling it again
here is redundant and may mask errors that occur only on the first call.
Consider removing the extra invocation or ensuring idempotency.holmes/core/toolset_manager.py (1)
401-406: Duplicate dictionary assignment introduces dead codeBoth lines assign the same
Toolsetinstance toexisting_toolsets_by_name[new_toolset.name].
The second assignment is redundant, adds noise to the diff, and may confuse future readers.- else: - existing_toolsets_by_name[new_toolset.name] = new_toolset - existing_toolsets_by_name[new_toolset.name] = new_toolset + else: + existing_toolsets_by_name[new_toolset.name] = new_toolsetholmes/plugins/toolsets/opensearch/opensearch_utils.py (1)
54-67: Makeinit_config()tolerant of repeated calls
OpenSearchIndexConfig(**self.config)explodes on the second call. Handle both dict and model types and avoid overwriting an existing parsed model.- else: - self.config = OpenSearchIndexConfig(**self.config) + elif self.config and not isinstance(self.config, OpenSearchIndexConfig): + # First time: parse dict ➜ model, store for future idempotent calls + self.config = OpenSearchIndexConfig(**self.config)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
holmes/core/tools.py(4 hunks)holmes/core/toolset_manager.py(5 hunks)holmes/plugins/toolsets/bash/bash_toolset.py(2 hunks)holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py(1 hunks)holmes/plugins/toolsets/datadog.py(2 hunks)holmes/plugins/toolsets/datetime.py(2 hunks)holmes/plugins/toolsets/git.py(3 hunks)holmes/plugins/toolsets/grafana/base_grafana_toolset.py(3 hunks)holmes/plugins/toolsets/internet/internet.py(2 hunks)holmes/plugins/toolsets/kafka.py(2 hunks)holmes/plugins/toolsets/mcp/toolset_mcp.py(2 hunks)holmes/plugins/toolsets/newrelic.py(3 hunks)holmes/plugins/toolsets/opensearch/opensearch.py(2 hunks)holmes/plugins/toolsets/opensearch/opensearch_logs.py(1 hunks)holmes/plugins/toolsets/opensearch/opensearch_utils.py(3 hunks)holmes/plugins/toolsets/prometheus/prometheus.py(2 hunks)holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py(3 hunks)holmes/plugins/toolsets/robusta/robusta.py(2 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
holmes/plugins/toolsets/prometheus/prometheus.py
[error] 803-803: Access to member 'config' before its definition line 804
(E0203)
holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py
[refactor] 202-202: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[error] 203-203: Access to member 'config' before its definition line 226
(E0203)
holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py
[refactor] 149-152: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
holmes/plugins/toolsets/newrelic.py
[error] 224-224: Non-mapping value self.config is used in a mapping context
(E1134)
holmes/plugins/toolsets/opensearch/opensearch.py
[error] 215-215: Non-mapping value self.config is used in a mapping context
(E1134)
[error] 248-248: Non-mapping value self.config is used in a mapping context
(E1134)
holmes/plugins/toolsets/datadog.py
[error] 154-154: Non-mapping value self.config is used in a mapping context
(E1134)
holmes/plugins/toolsets/kafka.py
[error] 507-507: Non-mapping value self.config is used in a mapping context
(E1134)
[error] 542-542: Non-mapping value self.config is used in a mapping context
(E1134)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (3.12)
🔇 Additional comments (4)
holmes/plugins/toolsets/opensearch/opensearch_logs.py (1)
1-23: Import-only changes look fineRe-ordering and grouping imports does not alter behaviour and keeps the file PEP-8 compliant.
holmes/core/tools.py (1)
413-417: Missing fallback for legacy callables
(enabled, error_message) = prereq.callable()presumes the new signature. If any external/third-party toolset still passes a one-arg function, aTypeErrorwill bubble up and mark the whole toolset as FAILED.Add a small compatibility shim:
try: - (enabled, error_message) = prereq.callable() + try: + (enabled, error_message) = prereq.callable() + except TypeError: + # backward-compat: legacy callables expected config + (enabled, error_message) = prereq.callable(self.config) # type: ignore[arg-type]This preserves backwards compatibility while encouraging migration.
holmes/plugins/toolsets/grafana/base_grafana_toolset.py (1)
55-57: Idempotentinit_configSee diff above – making the method safe for multiple invocations prevents
unexpected TypeErrors during runtime.holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py (1)
202-226: Mixed return paths – some return a value, some don’t
init_confignow has code paths thatreturnearly and others that fall
through, leading to inconsistent return types (NonevsTuple[bool,str])
and pylint warning R1710.
After adopting the above fix, remove all explicitreturnstatements and
let the function returnNone.
mainred
force-pushed
the
fix-bash-toolset
branch
from
0a6de93 to
98e9260
Compare
|
Thank you for this @mainred. I agree the current approach is not ideal and we want to steer away from it but I also think that adding another function to initialize the config will make toolset initialization more confusing than it already is. |
|
check_prerequisites takes time, while our toolset cache is to reduce the loading time taken by the check_prerequisites. So I instead create a init_config method, and assume that the check has been done during the status refresh stage |
ok makes sense. In that case the current change can be improve by making sure the init_config method is idempotent. For example in the case of prometheus, calling What about changing |
|
Also note that the pre-commit checks fail |
holmes/plugins/toolsets/git.py
Outdated
| def prerequisites_callable(self) -> Tuple[bool, str]: | ||
| if not self.config and not ( | ||
| os.getenv("GIT_REPO") and os.getenv("GIT_CREDENTIALS") | ||
| ): |
There was a problem hiding this comment.
It looks like the env checks belong to init_config
There was a problem hiding this comment.
I think I have changed git toolset. It was reverted somehow.
There was a problem hiding this comment.
We can discuss. In prerequisite check, we check if the config and env is correct. in init_config, should we also check these prerequisites? Or assume the enabled toolset has meet the requirement.
There was a problem hiding this comment.
I think you can assume it has met the requirements because you want to skip the prerequisites liveness checks.
There is some caveats that the config dict may not match the expected pydantic BaseModel and in that case the toolset should go in the proper 'failed' state with a meaningful error message.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
tests/utils/toolsets.py (1)
4-14: Signatures still use the oldconfigparameter – will break onceCallablePrerequisiteexpects a zero-arg callableAccording to the refactor, prerequisite callables are now expected to have the type
Callable[[], Tuple[bool, str]].
callable_success,callable_failure_with_message, andcallable_failure_no_messagestill accept aconfigargument, so the moment the new typing (or the framework) enforces the arity, these functions will raise aTypeError.Proposed quick fix:
-from typing import Any, Dict, Tuple +from typing import Tuple @@ -def callable_success(config: Dict[str, Any]) -> Tuple[bool, str]: - return True, "" +def callable_success() -> Tuple[bool, str]: + return True, "" @@ -def callable_failure_with_message(config: Dict[str, Any]) -> Tuple[bool, str]: - return False, "Callable check failed" +def callable_failure_with_message() -> Tuple[bool, str]: + return False, "Callable check failed" @@ -def callable_failure_no_message(config: Dict[str, Any]) -> Tuple[bool, str]: - return False, "" +def callable_failure_no_message() -> Tuple[bool, str]: + return False, ""(You can remove the unused
Any/Dictimports after the change.)tests/test_mcp_toolset.py (1)
55-58: Typo in test URL – use colon, not slash
"http://0.0.0.0/3005"is not a valid host-and-port URL; the port separator must be:.
Although the URL is not dereferenced in the happy-path thanks to the monkey-patch, leaving an invalid URI is misleading and risks future copy-paste errors.- url="http://0.0.0.0/3005", + url="http://0.0.0.0:3005",
🧹 Nitpick comments (2)
tests/plugins/prompt/test_toolsets_instructions.py (1)
41-42: Add a small docstring to clarify the no-opinit_configA one-liner docstring will make it explicit that the method is intentionally empty for this mock implementation and prevents “missing-docstring” linter noise.
def init_config(self): - pass + """MockToolset never needs extra configuration.""" + passtests/test_mcp_toolset.py (1)
1-4: Minor import-ordering nitPEP 8 groups standard-library, third-party and local imports separately.
mcp.typesandholmes.*are both third-party/local, so the blank line after the first import is superfluous.Non-blocking, but running
ruff/flake8 –-select Iwill autofix this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/llm/utils/mock_toolset.py(1 hunks)tests/plugins/prompt/test_toolsets_instructions.py(2 hunks)tests/test_check_prerequisites.py(1 hunks)tests/test_holmes_sync_toolsets.py(1 hunks)tests/test_mcp_toolset.py(3 hunks)tests/utils/toolsets.py(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- tests/llm/utils/mock_toolset.py
- tests/test_check_prerequisites.py
- tests/test_holmes_sync_toolsets.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
🔇 Additional comments (1)
tests/plugins/prompt/test_toolsets_instructions.py (1)
10-10: Good import-grouping improvement
load_and_render_promptis now grouped after the stdlib/third-party/first-party block, matching the typical isort layout. Nice tidy-up.
mainred
force-pushed
the
fix-bash-toolset
branch
2 times, most recently
from
0aa4810 to
ace3b7a
Compare
…uisites is not called
|
@nherment could you please take another look? |
mainred
force-pushed
the
fix-bash-toolset
branch
from
1d89436 to
16db3b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py (2)
146-153: Remove unnecessaryelseblock and ensure idempotent configuration initialization.The
elseblock after thereturnstatement is unnecessary and the current implementation will fail on repeated calls.Apply this fix to address both issues:
def prerequisites_callable(self) -> Tuple[bool, str]: if not self.config: return False, TOOLSET_CONFIG_MISSING_ERROR self.init_config() - if self.config.api_key: - return health_check(domain=self.config.domain, api_key=self.config.api_key) - else: - return False, "Missing configuration field 'api_key'" + if not self.config.api_key: + return False, "Missing configuration field 'api_key'" + return health_check(domain=self.config.domain, api_key=self.config.api_key)
155-160: Makeinit_configidempotent to prevent double-casting errors.The current implementation will fail if called multiple times because
CoralogixConfig(**self.config)expects a dictionary butself.configbecomes aCoralogixConfiginstance after the first call.def init_config(self): if not self.config: logging.error("The coralogix/logs toolset is not configured") return - self.config = CoralogixConfig(**self.config) + if isinstance(self.config, dict): + self.config = CoralogixConfig(**self.config)holmes/plugins/toolsets/opensearch/opensearch.py (1)
210-232: Centralize client creation to avoid duplication and ensure type safety.Both
prerequisites_callableandinit_configcreate OpenSearchClient instances, causing redundant work and multiple connections. The type error occurs becauseself.configmay not be a mapping.Centralize client creation in
init_configand modifyprerequisites_callableto delegate:def prerequisites_callable(self) -> Tuple[bool, str]: if not self.config: return False, TOOLSET_CONFIG_MISSING_ERROR - - try: - os_config = OpenSearchConfig(**self.config) - errors = [] - for cluster in os_config.opensearch_clusters: - try: - logging.info("Setting up OpenSearch client") - cluster_kwargs = cluster.model_dump() - client = OpenSearchClient(**cluster_kwargs) - if client.client.cluster.health(params={"timeout": 5}): - self.clients.append(client) - except Exception as e: - message = f"Failed to set up opensearch client {str(cluster.hosts)}. {str(e)}" - logging.exception(message) - errors.append(message) - - return len(self.clients) > 0, "\n".join(errors) - except Exception as e: - logging.exception("Failed to set up OpenSearch toolset") - return False, str(e) + + self.init_config() + return len(self.clients) > 0, "Failed to connect to any OpenSearch clusters" if len(self.clients) == 0 else ""holmes/plugins/toolsets/kafka.py (1)
541-560: Makeinit_configidempotent and prevent connection leaks.The current implementation recreates Kafka clients on every call, causing connection leaks and potential errors with double-casting.
def init_config(self): - kafka_config = KafkaConfig(**self.config) + if not self.config: + return + + if isinstance(self.config, KafkaConfig): + kafka_config = self.config + elif isinstance(self.config, dict): + kafka_config = KafkaConfig(**self.config) + self.config = kafka_config + else: + return for cluster in kafka_config.kafka_clusters: + # Skip if client already exists to prevent connection leaks + if cluster.name in self.clients: + continue + admin_config = { "bootstrap.servers": cluster.kafka_broker, "client.id": cluster.kafka_client_id, } if cluster.kafka_security_protocol: admin_config["security.protocol"] = cluster.kafka_security_protocol if cluster.kafka_sasl_mechanism: admin_config["sasl.mechanisms"] = cluster.kafka_sasl_mechanism if cluster.kafka_username and cluster.kafka_password: admin_config["sasl.username"] = cluster.kafka_username admin_config["sasl.password"] = cluster.kafka_password client = AdminClient(admin_config) - self.clients[cluster.name] = client # Store in dictionary + self.clients[cluster.name] = clientholmes/plugins/toolsets/datadog.py (1)
153-156: Makeinit_configidempotent and handle different config types safely.The current implementation will fail if
self.configis already aDatadogConfiginstance, asDatadogConfig(**self.config)expects a mapping.def init_config(self): - dd_config = DatadogConfig(**self.config) + if isinstance(self.config, DatadogConfig): + dd_config = self.config + elif isinstance(self.config, dict): + dd_config = DatadogConfig(**self.config) + else: + dd_config = DatadogConfig() + self.dd_api_key = dd_config.dd_api_key self.dd_app_key = dd_config.dd_app_key
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
holmes/core/tools.py(5 hunks)holmes/core/toolset_manager.py(5 hunks)holmes/plugins/toolsets/bash/bash_toolset.py(2 hunks)holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py(2 hunks)holmes/plugins/toolsets/datadog.py(2 hunks)holmes/plugins/toolsets/git.py(3 hunks)holmes/plugins/toolsets/grafana/base_grafana_toolset.py(3 hunks)holmes/plugins/toolsets/internet/internet.py(2 hunks)holmes/plugins/toolsets/kafka.py(2 hunks)holmes/plugins/toolsets/mcp/toolset_mcp.py(3 hunks)holmes/plugins/toolsets/newrelic.py(3 hunks)holmes/plugins/toolsets/opensearch/opensearch.py(2 hunks)holmes/plugins/toolsets/opensearch/opensearch_logs.py(1 hunks)holmes/plugins/toolsets/opensearch/opensearch_utils.py(3 hunks)holmes/plugins/toolsets/prometheus/prometheus.py(2 hunks)holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py(3 hunks)holmes/plugins/toolsets/robusta/robusta.py(2 hunks)tests/llm/utils/mock_toolset.py(1 hunks)tests/plugins/prompt/test_toolsets_instructions.py(2 hunks)tests/test_check_prerequisites.py(2 hunks)tests/test_holmes_sync_toolsets.py(1 hunks)tests/test_mcp_toolset.py(3 hunks)tests/utils/toolsets.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- tests/test_holmes_sync_toolsets.py
- tests/plugins/prompt/test_toolsets_instructions.py
- tests/test_check_prerequisites.py
- tests/llm/utils/mock_toolset.py
- holmes/plugins/toolsets/robusta/robusta.py
- holmes/plugins/toolsets/opensearch/opensearch_logs.py
- holmes/plugins/toolsets/bash/bash_toolset.py
- tests/test_mcp_toolset.py
- holmes/plugins/toolsets/git.py
- holmes/plugins/toolsets/internet/internet.py
- holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py
- holmes/core/toolset_manager.py
- holmes/plugins/toolsets/opensearch/opensearch_utils.py
- holmes/plugins/toolsets/grafana/base_grafana_toolset.py
- tests/utils/toolsets.py
- holmes/plugins/toolsets/mcp/toolset_mcp.py
- holmes/core/tools.py
🧰 Additional context used
🪛 Pylint (3.3.7)
holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py
[refactor] 150-153: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
holmes/plugins/toolsets/datadog.py
[error] 154-154: Non-mapping value self.config is used in a mapping context
(E1134)
holmes/plugins/toolsets/kafka.py
[error] 507-507: Non-mapping value self.config is used in a mapping context
(E1134)
[error] 542-542: Non-mapping value self.config is used in a mapping context
(E1134)
holmes/plugins/toolsets/newrelic.py
[error] 227-227: Non-mapping value self.config is used in a mapping context
(E1134)
holmes/plugins/toolsets/opensearch/opensearch.py
[error] 215-215: Non-mapping value self.config is used in a mapping context
(E1134)
[error] 248-248: Non-mapping value self.config is used in a mapping context
(E1134)
holmes/plugins/toolsets/prometheus/prometheus.py
[error] 803-803: Access to member 'config' before its definition line 804
(E0203)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (3.12)
🔇 Additional comments (5)
holmes/plugins/toolsets/opensearch/opensearch.py (1)
247-253: Add error handling and health checks toinit_config.Move the complete client setup logic with health checks and error handling to
init_config:def init_config(self): - os_config = OpenSearchConfig(**self.config) - for cluster in os_config.opensearch_clusters: - logging.info("Setting up OpenSearch client") - cluster_kwargs = cluster.model_dump() - client = OpenSearchClient(**cluster_kwargs) - self.clients.append(client) + if not self.config: + return + + if isinstance(self.config, dict): + os_config = OpenSearchConfig(**self.config) + else: + return + + for cluster in os_config.opensearch_clusters: + try: + logging.info("Setting up OpenSearch client") + cluster_kwargs = cluster.model_dump() + client = OpenSearchClient(**cluster_kwargs) + if client.client.cluster.health(params={"timeout": 5}): + self.clients.append(client) + except Exception as e: + logging.exception(f"Failed to set up opensearch client {str(cluster.hosts)}. {str(e)}")holmes/plugins/toolsets/prometheus/prometheus.py (1)
802-813: Makeinit_configidempotent and type-safe.The current implementation has the same double-casting issue as other toolsets and the pylint error indicates accessing
self.configbefore proper definition.def init_config(self): - if self.config: - self.config = PrometheusConfig(**self.config) + if self.config and isinstance(self.config, dict): + self.config = PrometheusConfig(**self.config) + elif not self.config: + prometheus_url = os.environ.get("PROMETHEUS_URL") + if not prometheus_url: + prometheus_url = self.auto_detect_prometheus_url() + self.config = PrometheusConfig( + prometheus_url=prometheus_url, + headers=add_prometheus_auth(os.environ.get("PROMETHEUS_AUTH_HEADER")), + ) + logging.info(f"Prometheus auto discovered at url {prometheus_url}")This ensures the method is idempotent and handles the case where
self.configis already aPrometheusConfiginstance.holmes/plugins/toolsets/newrelic.py (3)
2-5: Import reordering looks good.The imports have been properly organized with standard library imports first, followed by third-party imports, then local imports.
9-9: Import additions are correctly placed.The
StructuredToolResultandToolResultStatusimports have been moved to an appropriate location in the import list.Also applies to: 12-12
205-218: Method signature change aligns with the refactor pattern.The removal of the
configparameter fromprerequisites_callableand the reliance onself.configis consistent with the broader refactor described in the PR objectives. The logic correctly checks for configuration presence and calls the newinit_configmethod.
There was a problem hiding this comment.
HI @mainred , a few more comments, including some that were made before. I'm happy to jump on a call and go over them and/or the PR with you.
The main issue is the use of self.config = TypedConfig(**self.config) which mutates the type of self.config. In other words:
- The design of
init_configshould be resilient to being called multiple times and provide the same outcome toolset.config(or wherever the config is stored) should always be typed and not a dict. Some of the toolsets 'fix' that by saving the typed config in a different field. I understand that some of that is actually legacy code and not something you introduced. However the introduction of the toolset caching and this PR change the code enough that the overall implementation should address this legacy.
Also, in some cases, code is duplicated between init_config and the prerequisites_callable which should be avoided. prerequisites_callable should not have to initialize or validate the config.
Given the pattern is the same across all toolsets, it would be better to always call init_config at the beginning of check_prerequisites.
| if not config: | ||
| return True, "" | ||
| self.additional_headers = config.get("additional_headers", {}) | ||
| def prerequisites_callable(self) -> Tuple[bool, str]: |
There was a problem hiding this comment.
prerequisites_callable can be removed if it's not checking anything
holmes/plugins/toolsets/datadog.py
Outdated
| dd_config = DatadogConfig(**config) | ||
| self.dd_api_key = dd_config.dd_api_key | ||
| self.dd_app_key = dd_config.dd_app_key | ||
| self.init_config() |
There was a problem hiding this comment.
Is it not better to call toolset.init_config() within toolset.check_prerequisites()?
What happens where there are multiple prerequisites (possible by design)? There is an implicit guarantee thyat prerequisites are run in order but it's just not ideal to have to call init_config within a callable prerequisite.
There was a problem hiding this comment.
Once init_config is called inside check_prerequisites it is important to catch any config validation error and return an appropriate error message that helps users remediating the error.
holmes/plugins/toolsets/kafka.py
Outdated
| return False, TOOLSET_CONFIG_MISSING_ERROR | ||
| errors = [] | ||
| try: | ||
| kafka_config = KafkaConfig(**config) | ||
| kafka_config = KafkaConfig(**self.config) |
There was a problem hiding this comment.
This should be an indication that the current design is incorrect.
Why does prerequisites callable need to initialize and validate the config when it's the role of init_config? It's repeated code and requires consistent error handling (user should see the same error for the same misconfiguration).
Rather here the pattern should be the one established. init_config validates and creates a typed config which is then used in prerequisites_callable.
holmes/plugins/toolsets/kafka.py
Outdated
| admin_config["sasl.password"] = cluster.kafka_password | ||
|
|
||
| client = AdminClient(admin_config) | ||
| self.clients[cluster.name] = client # Store in dictionary |
There was a problem hiding this comment.
The code here is a copy paste of the code in prerequisites_callable. It should be extracted in its a private method to avoid duplication.
| return False, "Missing opensearch traces URL. Check your config" | ||
| elif not config and env_url: | ||
| elif not self.config and env_url: |
There was a problem hiding this comment.
It's the role of init_config to consolidate a valid config so there is no longer any need to check for env vars here so the code here is duplicated unnecessarily .
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py (1)
159-165: Remove unnecessary else block after returnThe
elsebranch is unnecessary since thereturnstatement already exits the function.- if self.typed_config.api_key: # type: ignore - return health_check( - domain=self.typed_config.domain, # type: ignore - api_key=self.typed_config.api_key, # type: ignore - ) - else: - return False, "Missing configuration field 'api_key'" + if not self.typed_config.api_key: # type: ignore + return False, "Missing configuration field 'api_key'" + return health_check( + domain=self.typed_config.domain, # type: ignore + api_key=self.typed_config.api_key, # type: ignore + )
🧹 Nitpick comments (2)
holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py (1)
170-171: Consider making init_config idempotentWhile the current implementation works correctly, consider adding a guard to make it idempotent as mentioned in the retrieved learnings.
def init_config(self, config: dict[str, Any]): - self.typed_config = CoralogixConfig(**config) + if not isinstance(self.typed_config, CoralogixConfig): + self.typed_config = CoralogixConfig(**config)holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py (1)
37-45: Simplify control flow by removing unnecessary elifThe
elifafterreturncan be simplified toiffor better readability.- elif not cluster_id and len(cluster_ids) > 0: + if not cluster_id and len(cluster_ids) > 0:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
holmes/core/tools.py(6 hunks)holmes/core/toolset_manager.py(5 hunks)holmes/plugins/toolsets/bash/bash_toolset.py(5 hunks)holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py(5 hunks)holmes/plugins/toolsets/datadog.py(2 hunks)holmes/plugins/toolsets/git.py(3 hunks)holmes/plugins/toolsets/grafana/base_grafana_toolset.py(2 hunks)holmes/plugins/toolsets/internet/internet.py(2 hunks)holmes/plugins/toolsets/kafka.py(1 hunks)holmes/plugins/toolsets/mcp/toolset_mcp.py(2 hunks)holmes/plugins/toolsets/newrelic.py(2 hunks)holmes/plugins/toolsets/opensearch/opensearch.py(2 hunks)holmes/plugins/toolsets/opensearch/opensearch_logs.py(7 hunks)holmes/plugins/toolsets/opensearch/opensearch_traces.py(4 hunks)holmes/plugins/toolsets/opensearch/opensearch_utils.py(3 hunks)holmes/plugins/toolsets/prometheus/prometheus.py(15 hunks)holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py(7 hunks)holmes/plugins/toolsets/robusta/robusta.py(2 hunks)holmes/plugins/toolsets/runbook/runbook_fetcher.py(1 hunks)tests/core/test_toolset_manager.py(1 hunks)tests/llm/utils/mock_toolset.py(1 hunks)tests/plugins/prompt/test_toolsets_instructions.py(2 hunks)tests/test_check_prerequisites.py(2 hunks)tests/test_holmes_sync_toolsets.py(1 hunks)tests/test_mcp_toolset.py(3 hunks)tests/utils/toolsets.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/core/test_toolset_manager.py
🚧 Files skipped from review as they are similar to previous changes (22)
- tests/llm/utils/mock_toolset.py
- tests/test_check_prerequisites.py
- tests/plugins/prompt/test_toolsets_instructions.py
- holmes/plugins/toolsets/opensearch/opensearch_logs.py
- tests/test_holmes_sync_toolsets.py
- holmes/plugins/toolsets/runbook/runbook_fetcher.py
- holmes/plugins/toolsets/git.py
- holmes/plugins/toolsets/mcp/toolset_mcp.py
- tests/test_mcp_toolset.py
- holmes/plugins/toolsets/internet/internet.py
- holmes/plugins/toolsets/robusta/robusta.py
- holmes/plugins/toolsets/grafana/base_grafana_toolset.py
- holmes/core/toolset_manager.py
- holmes/plugins/toolsets/datadog.py
- holmes/core/tools.py
- holmes/plugins/toolsets/bash/bash_toolset.py
- holmes/plugins/toolsets/opensearch/opensearch_utils.py
- holmes/plugins/toolsets/opensearch/opensearch.py
- holmes/plugins/toolsets/newrelic.py
- holmes/plugins/toolsets/prometheus/prometheus.py
- tests/utils/toolsets.py
- holmes/plugins/toolsets/kafka.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: nherment
PR: robusta-dev/holmesgpt#535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.531Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🪛 Pylint (3.3.7)
holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py
[refactor] 159-165: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py
[refactor] 37-45: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.12)
🔇 Additional comments (4)
holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py (1)
1-1: Well-structured refactoring for configuration managementThe addition of the logging import, typed configuration attribute, and centralized
init_configmethod aligns well with the PR objectives. The error handling inprerequisites_callableis robust and the separation of concerns is clear.Also applies to: 34-34, 156-171
holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py (2)
213-231: Excellent environment variable fallback implementationThe
init_configmethod elegantly handles the case where no clusters are configured by falling back to environment variables with sensible defaults. The logging message provides good visibility into the configuration source.
123-123: Clean refactoring to typed configurationThe introduction of
typed_configand the delegation toinit_configin the prerequisites method improves type safety and follows the established pattern from the PR objectives.Also applies to: 158-162
holmes/plugins/toolsets/opensearch/opensearch_traces.py (1)
1-1: Import reordering appears clean.The import statements have been reorganized logically, grouping standard library imports first and third-party imports together. The changes look appropriate for code organization.
Also applies to: 3-3, 8-8, 12-15
There was a problem hiding this comment.
Hi @mainred ,
There is a previous comment (albeit minor) that is unaddressed: https://github.com/robusta-dev/holmesgpt/pull/535/files#r2162958028
Some other comments, mostly around not ignoring type checks and around reporting config errors to users.
Overall the approach looks good. Well done!
holmes/plugins/toolsets/kafka.py
Outdated
| admin_config["sasl.password"] = cluster.kafka_password | ||
|
|
||
| client = AdminClient(admin_config) | ||
| self.clients[cluster.name] = client |
There was a problem hiding this comment.
It's great that the code is no longer duplicated. However I think there is a regression: the reporting of configuration errors is no longer complete as the code now would only report a failure for the first cluster that is misconfigued as opposed to all of them.
One of our big focus is to smooth the onboarding and ease of configuration for our users and I feel this is a backward step.
There was a problem hiding this comment.
Yeah, I agree. But I did not find a better way to organize all the errors since errors are raised in init_config, and it seems to be no better way to let init_config pass the errors to prerequisites_callable
There was a problem hiding this comment.
It's not great but would it work to gather all errors and then raise an exception? It's not too different than how a pydantic validation error would behave. It's also somewhat simple to implement.
| cluster_kwargs = cluster.model_dump() | ||
| client = OpenSearchClient(**cluster_kwargs) | ||
| if client.client.cluster.health(params={"timeout": 5}): | ||
| self.clients.append(client) |
There was a problem hiding this comment.
Putting the health check inside the config is fine by me be isn't what you want to avoid by separating the config?
What is important though is to make sure that if more than one cluster cannot be initialized that all the failures are reported to the user for the same reason that I mentioned about the Kafka clusters initialisation.
Looks like there is a bug here as well and I've recorded it separately: there is no error if a cluster reports unhealthy but it won't be part of the initialized clients.
There was a problem hiding this comment.
@arikalon1 I think we need your opinion around handling these cases.
There was a problem hiding this comment.
@mainred I think we need to be aligned with the previous behavior
I think previously, it was checking health on all clusters.
If a cluster is healthy - it's added to the list.
If not - the error is returned.
Currently, I think there are 2 issues:
- The loop in line 240, is not with try/except - so any error will not be caught and will stop the execution
- I think there's another loop, in line 247 - looks like almost the same code. Is it duplicated? or, is this on purpose?
| @@ -25,7 +24,7 @@ | |||
|
|
|||
|
|
|||
| class BaseBashExecutorToolset(Toolset): | |||
| config: Optional[BashExecutorConfig] = None | |||
| typed_config: Optional[BashExecutorConfig] = None | |||
There was a problem hiding this comment.
Instead of reusing config, which might be confused with Toolset.config, I use typed_config for specific toolsets for a typed config.
FYI @arikalon1 We had discussions around this variable naming, but now we are on the same page to use typed_config to be specific.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py (1)
147-167: Duplicate environment variable logic and non-idempotent init_config.The environment variable fallback logic is duplicated between
prerequisites_callable(lines 148-155) andinit_config(lines 219-233), which could lead to inconsistent behavior. Additionally, theinit_configmethod is not idempotent as required by the retrieved learnings.Refactor to eliminate duplication and ensure idempotency:
def prerequisites_callable(self, config: dict[str, Any]) -> Tuple[bool, str]: - if not config or not config.get("clusters"): - # Attempt to load from environment variables as fallback - env_url = os.environ.get("RABBITMQ_MANAGEMENT_URL") - if not env_url: - return ( - False, - "RabbitMQ toolset is misconfigured. 'management_url' is required.", - ) - try: self.init_config(config) if not self.typed_config: return ( False, - "RabbitMQ toolset is misconfigured.", + "RabbitMQ toolset is misconfigured. 'management_url' is required.", ) except Exception as e: return (False, f"Failed to parse RabbitMQ configuration: {str(e)}") return self._check_clusters_config(self.typed_config)
♻️ Duplicate comments (2)
holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py (1)
7-7: Remove the type ignore comment as requested.Based on past review feedback, this type ignore comment should be removed.
-from requests import RequestException # type: ignore +from requests import RequestExceptionholmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py (1)
160-166: Remove unnecessary else block after return.The
elseblock is unnecessary since thereturnstatement already exits the function.- if self.typed_config.api_key: - return health_check( - domain=self.typed_config.domain, - api_key=self.typed_config.api_key, - ) - else: - return False, "Missing configuration field 'api_key'" + if not self.typed_config.api_key: + return False, "Missing configuration field 'api_key'" + return health_check( + domain=self.typed_config.domain, + api_key=self.typed_config.api_key, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
holmes/core/tools.py(6 hunks)holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py(5 hunks)holmes/plugins/toolsets/datadog.py(2 hunks)holmes/plugins/toolsets/git.py(3 hunks)holmes/plugins/toolsets/grafana/base_grafana_toolset.py(2 hunks)holmes/plugins/toolsets/opensearch/opensearch_logs.py(7 hunks)holmes/plugins/toolsets/opensearch/opensearch_traces.py(4 hunks)holmes/plugins/toolsets/opensearch/opensearch_utils.py(3 hunks)holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py(7 hunks)tests/utils/toolsets.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- holmes/plugins/toolsets/grafana/base_grafana_toolset.py
- holmes/plugins/toolsets/git.py
- holmes/plugins/toolsets/opensearch/opensearch_traces.py
- holmes/plugins/toolsets/opensearch/opensearch_logs.py
- holmes/plugins/toolsets/datadog.py
- holmes/core/tools.py
- holmes/plugins/toolsets/opensearch/opensearch_utils.py
- tests/utils/toolsets.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: nherment
PR: robusta-dev/holmesgpt#535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.531Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🪛 Pylint (3.3.7)
holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py
[refactor] 160-166: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py
[refactor] 37-45: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
holmes/core/tools.py(6 hunks)holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py(3 hunks)holmes/plugins/toolsets/git.py(3 hunks)holmes/plugins/toolsets/grafana/base_grafana_toolset.py(3 hunks)holmes/plugins/toolsets/grafana/toolset_grafana.py(4 hunks)holmes/plugins/toolsets/grafana/toolset_grafana_loki.py(3 hunks)holmes/plugins/toolsets/grafana/toolset_grafana_tempo.py(1 hunks)holmes/plugins/toolsets/kubernetes_logs.py(3 hunks)holmes/plugins/toolsets/logging_utils/logging_api.py(1 hunks)holmes/plugins/toolsets/opensearch/opensearch_logs.py(7 hunks)holmes/plugins/toolsets/opensearch/opensearch_traces.py(5 hunks)holmes/plugins/toolsets/opensearch/opensearch_utils.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- holmes/plugins/toolsets/logging_utils/logging_api.py
🚧 Files skipped from review as they are similar to previous changes (7)
- holmes/plugins/toolsets/git.py
- holmes/plugins/toolsets/grafana/base_grafana_toolset.py
- holmes/core/tools.py
- holmes/plugins/toolsets/opensearch/opensearch_logs.py
- holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py
- holmes/plugins/toolsets/opensearch/opensearch_traces.py
- holmes/plugins/toolsets/opensearch/opensearch_utils.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: nherment
PR: robusta-dev/holmesgpt#535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.531Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: llm_evals
🔇 Additional comments (8)
holmes/plugins/toolsets/kubernetes_logs.py (3)
3-4: LGTM: Import formatting improvements.The import statement reordering and additional blank line improve code formatting and readability without any functional impact.
133-137: LGTM: Improved readability with multiline formatting.The reformatting of the
StructuredToolResultconstruction to use multiline parentheses enhances code readability while maintaining identical functionality.
259-261: LGTM: Appropriate no-op implementation of init_config.The no-op implementation is consistent with the architectural pattern across toolsets and makes sense for this Kubernetes logs toolset, which already handles its initialization in the
__init__method through client setup and health checks. The method signature correctly matches the abstract interface requirements and is idempotent as required.holmes/plugins/toolsets/grafana/toolset_grafana_tempo.py (1)
58-58: Verify thattyped_configis properly initialized in the base class.The change from
self._grafana_configtoself.typed_configassumes thattyped_configis available and properly initialized. Since this class inherits fromBaseGrafanaToolset, ensure that the base class implements theinit_configmethod and properly initializestyped_config.#!/bin/bash # Description: Verify that BaseGrafanaToolset implements typed_config and init_config # Expected: Find typed_config attribute and init_config method in BaseGrafanaToolset echo "Checking BaseGrafanaToolset implementation..." rg -A 10 "class BaseGrafanaToolset" holmes/plugins/toolsets/grafana/base_grafana_toolset.py echo "Checking for typed_config attribute in BaseGrafanaToolset..." rg "typed_config" holmes/plugins/toolsets/grafana/base_grafana_toolset.py echo "Checking for init_config method in BaseGrafanaToolset..." rg -A 5 "def init_config" holmes/plugins/toolsets/grafana/base_grafana_toolset.pyholmes/plugins/toolsets/grafana/toolset_grafana.py (3)
48-53: Good error handling for missing configuration.The check for
typed_configexistence and returning a structured error result is well-implemented and follows the pattern established in the PR.
54-55: Consistent migration from_grafana_configtotyped_config.All references to the configuration have been consistently updated to use
typed_config. The changes maintain the same functionality while following the new standardized configuration pattern.Also applies to: 63-64, 77-77
94-98: Verify the impact of changing return type from string toStructuredToolResult.The method now returns
StructuredToolResultinstead of a plain string. This is a breaking change that may impact existing callers of this method.#!/bin/bash # Description: Find all usages of ListAndBuildGrafanaDashboardURLs._invoke to check for compatibility # Expected: All callers should handle StructuredToolResult return type echo "Searching for direct calls to _invoke method..." rg -A 3 "ListAndBuildGrafanaDashboardURLs.*_invoke" echo "Searching for instantiation and usage of ListAndBuildGrafanaDashboardURLs..." rg -A 5 "ListAndBuildGrafanaDashboardURLs" echo "Checking if this tool is used through the Tool framework which might handle StructuredToolResult..." rg -A 5 "list_and_build_grafana_dashboard_urls"Also applies to: 101-105
holmes/plugins/toolsets/grafana/toolset_grafana_loki.py (1)
37-37: Proper declaration oftyped_configattribute.The declaration of
typed_configasOptional[GrafanaLokiConfig]is correct and follows the established pattern for this refactoring.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
holmes/plugins/toolsets/prometheus/prometheus.py (1)
829-841: Make init_config method idempotent and address error handling regressionThe
init_configmethod needs to be idempotent to safely handle multiple calls. Additionally, there's a regression in error handling for auto-discovery that should be addressed.Apply this diff to make the method idempotent and improve error handling:
def init_config(self, config: Optional[dict[str, Any]]): + # Make idempotent - don't reinitialize if already set + if self.typed_config is not None: + return + if config: self.typed_config = PrometheusConfig(**config) else: prometheus_url = os.environ.get("PROMETHEUS_URL") if not prometheus_url: prometheus_url = self.auto_detect_prometheus_url() + if not prometheus_url: + raise ValueError("Unable to auto-detect prometheus. Define prometheus_url in the configuration for tool prometheus/metrics") self.typed_config = PrometheusConfig( prometheus_url=prometheus_url, headers=add_prometheus_auth(os.environ.get("PROMETHEUS_AUTH_HEADER")), ) - logging.info(f"Prometheus auto discovered at url {prometheus_url}") + if prometheus_url: + logging.info(f"Prometheus auto discovered at url {prometheus_url}")holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py (3)
7-7: Remove unnecessary type ignore comment-from requests import RequestException # type: ignore +from requests import RequestException
147-167: Fix error handling logic in prerequisites_callableThe current logic has issues with error handling when environment variables are missing. The method checks for missing environment variables but doesn't handle the case properly.
Apply this diff to fix the error handling:
def prerequisites_callable(self, config: dict[str, Any]) -> Tuple[bool, str]: - if not config or not config.get("clusters"): - # Attempt to load from environment variables as fallback - env_url = os.environ.get("RABBITMQ_MANAGEMENT_URL") - if not env_url: - return ( - False, - "RabbitMQ toolset is misconfigured. 'management_url' is required.", - ) - try: self.init_config(config) if not self.typed_config: return ( False, "RabbitMQ toolset is misconfigured.", ) except Exception as e: return (False, f"Failed to parse RabbitMQ configuration: {str(e)}") return self._check_clusters_config(self.typed_config)
218-236: Make init_config idempotent and handle missing environment variables properlyThe method should be idempotent and properly handle the case where required environment variables are missing.
Apply this diff to address the issues:
def init_config(self, config: Optional[dict[str, Any]]): + # Make idempotent - don't reinitialize if already set + if self.typed_config is not None: + return + if not config or not config.get("clusters"): # Attempt to load from environment variables as fallback env_url = os.environ.get("RABBITMQ_MANAGEMENT_URL") + if not env_url: + raise ValueError("RabbitMQ toolset is misconfigured. 'management_url' is required.") env_user = os.environ.get("RABBITMQ_USERNAME", "guest") env_pass = os.environ.get("RABBITMQ_PASSWORD", "guest") config = { "clusters": [ { "id": "rabbitmq", "management_url": env_url, "username": env_user, "password": env_pass, } ] } logging.info("Loaded RabbitMQ config from environment variables.") self.typed_config = RabbitMQConfig(**config)
🧹 Nitpick comments (1)
holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py (1)
34-53: Consider simplifying the conditional logicThe static analysis tool suggests removing unnecessary
elifstatements afterreturn.if not cluster_id and len(cluster_ids) == 1: # cluster id is optional if there is only one configured return self.toolset.typed_config.clusters[0] - elif not cluster_id and len(cluster_ids) > 0: + if not cluster_id and len(cluster_ids) > 0: raise ValueError( f"No cluster is configured. Possible cluster_id values are: {', '.join(cluster_ids)}" ) - elif not cluster_id: + if not cluster_id: raise ValueError("No cluster is configured")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
holmes/core/tools.py(6 hunks)holmes/plugins/toolsets/bash/bash_toolset.py(5 hunks)holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py(4 hunks)holmes/plugins/toolsets/datadog.py(2 hunks)holmes/plugins/toolsets/git.py(3 hunks)holmes/plugins/toolsets/grafana/base_grafana_toolset.py(3 hunks)holmes/plugins/toolsets/grafana/toolset_grafana_loki.py(3 hunks)holmes/plugins/toolsets/internet/internet.py(2 hunks)holmes/plugins/toolsets/kafka.py(1 hunks)holmes/plugins/toolsets/kubernetes_logs.py(3 hunks)holmes/plugins/toolsets/mcp/toolset_mcp.py(2 hunks)holmes/plugins/toolsets/newrelic.py(2 hunks)holmes/plugins/toolsets/opensearch/opensearch.py(2 hunks)holmes/plugins/toolsets/opensearch/opensearch_logs.py(7 hunks)holmes/plugins/toolsets/opensearch/opensearch_utils.py(3 hunks)holmes/plugins/toolsets/prometheus/prometheus.py(15 hunks)holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py(7 hunks)holmes/plugins/toolsets/robusta/robusta.py(2 hunks)holmes/plugins/toolsets/runbook/runbook_fetcher.py(2 hunks)tests/llm/utils/mock_toolset.py(2 hunks)tests/plugins/prompt/test_toolsets_instructions.py(2 hunks)tests/test_check_prerequisites.py(3 hunks)tests/test_holmes_sync_toolsets.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
- tests/llm/utils/mock_toolset.py
- tests/test_check_prerequisites.py
- tests/test_holmes_sync_toolsets.py
- tests/plugins/prompt/test_toolsets_instructions.py
- holmes/plugins/toolsets/mcp/toolset_mcp.py
- holmes/plugins/toolsets/runbook/runbook_fetcher.py
- holmes/plugins/toolsets/robusta/robusta.py
- holmes/plugins/toolsets/kubernetes_logs.py
- holmes/plugins/toolsets/git.py
- holmes/plugins/toolsets/grafana/base_grafana_toolset.py
- holmes/plugins/toolsets/internet/internet.py
- holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py
- holmes/plugins/toolsets/datadog.py
- holmes/plugins/toolsets/bash/bash_toolset.py
- holmes/plugins/toolsets/newrelic.py
- holmes/plugins/toolsets/opensearch/opensearch_utils.py
- holmes/plugins/toolsets/kafka.py
- holmes/core/tools.py
- holmes/plugins/toolsets/opensearch/opensearch_logs.py
- holmes/plugins/toolsets/opensearch/opensearch.py
- holmes/plugins/toolsets/grafana/toolset_grafana_loki.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: nherment
PR: robusta-dev/holmesgpt#535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.531Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🪛 Pylint (3.3.7)
holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py
[refactor] 37-45: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: llm_evals
🔇 Additional comments (4)
holmes/plugins/toolsets/prometheus/prometheus.py (3)
748-748: LGTM: Typed configuration attribute addedThe addition of the typed configuration attribute follows the established pattern for centralizing configuration handling.
292-295: Consistent configuration validation across toolsAll tools now properly validate the presence of
typed_configandprometheus_urlbefore proceeding. This ensures consistent error handling across the toolset.Also applies to: 386-389, 494-497, 633-636
776-776: Configuration initialization properly delegatedThe prerequisites callable now correctly delegates configuration initialization to the
init_configmethod, separating concerns appropriately.holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py (1)
123-123: LGTM: Typed configuration attribute addedThe addition of the typed configuration attribute follows the established pattern for centralizing configuration handling.
mainred
force-pushed
the
fix-bash-toolset
branch
from
fd882fd to
48f0f94
Compare
| return False, str(e) | ||
| return True, "" | ||
|
|
||
| def init_config(self, config: Optional[dict[str, Any]]): |
There was a problem hiding this comment.
Another concern is related to init_config.
prerequisites_callable calls init_config and can catch the exception when config is not well set. But in init_config, which can be also called when the status is cached and prerequisites_callable is not called https://github.com/robusta-dev/holmesgpt/pull/535/files#diff-bfd1b7e4ad5810c10197015d94b5d2aa9f3ad74a937f531f25954306858b08b0R246. The exception is not well handled, since we assume the prerequisites have been checked, but it is not also so considering some configurations are set by environment variables, and environment variables can be cleaned up after console restarts.
There was a problem hiding this comment.
@mainred with init_config, if the configuration is invalid, it should always raise an exception? And then the tool is not initialized?
| @@ -130,6 +131,12 @@ def __init__(self, toolset: "OpenSearchTracesToolset"): | |||
| def _invoke(self, params: Any) -> StructuredToolResult: | |||
| err_msg = "" | |||
| try: | |||
| if not self._toolset.typed_config: | |||
There was a problem hiding this comment.
@mainred Why do we need this check?
How can we get to _invoke, if no config was provided?
Wouldn't the toolset be disabled?
| def coralogix_config(self) -> Optional[CoralogixConfig]: | ||
| return self.config | ||
| def init_config(self, config: Optional[dict[str, Any]]): | ||
| if not config: |
There was a problem hiding this comment.
@mainred here, for example, init_config doesn't raise an exception when it's not initialized.
Will the Coralogix tool appear enabled or disabled?
| errors.append(message) | ||
|
|
||
| return len(self.clients) > 0, "\n".join(errors) | ||
| self.init_config(config) |
There was a problem hiding this comment.
@mainred here as well
If no config, init_config doesn't raise an exception. But we return True.
Is this correct?
We rely on check prerequisites to initialize the toolset configuration, but when the toolset is loaded from the cache, the prerequisites is not checked, and toolset configuration is not initialized.
Fix this by adding init_config method to initialize the toolset configuration when check prerequisites is not called.
fix: #532