Cluster health check and metrics retrieval #23
Conversation
…base cluster, including their bucket settings.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Eyal-CB, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly expands the capabilities of the Couchbase MCP server by introducing new tools for cluster health monitoring, metrics retrieval, and query optimization through index advising. A key architectural change involves shifting from a single, globally configured bucket to allowing bucket specification per operation, enhancing the server's flexibility and multi-bucket support.
Highlights
- New Cluster Management Tools: I've added new tools to perform cluster health checks (ping reports) which provide node and service IPs, and to fetch Prometheus-formatted metrics from individual Couchbase nodes. This enhances observability and diagnostic capabilities.
- SQL++ Index Advisor Integration: A new tool has been introduced to leverage the Couchbase SQL++ Index Advisor. This allows users to get recommendations for optimizing their N1QL queries, improving performance.
- Per-Operation Bucket Specification: I've refactored the server to remove the global
CB_BUCKET_NAMEconfiguration. Most Couchbase operations now explicitly require abucket_nameparameter, enabling more flexible interaction with multiple buckets within the same server instance. - Bucket Listing Capability: A new tool has been added to retrieve a comprehensive list of all configured buckets within the Couchbase cluster, including their detailed settings.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces new features for cluster health checks and metrics retrieval. It also refactors the server to support operations on multiple buckets by removing the single-bucket configuration. The changes are generally good, but there are some important issues to address: a critical security vulnerability by disabling SSL verification, some high-severity design issues regarding error handling and configuration retrieval, and several medium-severity issues related to code duplication, incorrect type hints, and code complexity.
| try: | ||
| bucket = cluster.bucket(bucket_name) | ||
| except Exception as e: | ||
| logger.error(f"Error accessing bucket: {e}") | ||
| raise ValueError("Tool does not have access to bucket, or bucket does not exist.") |
There was a problem hiding this comment.
The logic for retrieving a bucket object from the cluster and handling potential errors is duplicated across multiple tool functions (e.g., get_scopes_and_collections_in_bucket, get_document_by_id, upsert_document_by_id, etc.). To improve maintainability and reduce code duplication, consider extracting this logic into a private helper function.
| if result and (advice := result[0].get("advice")): | ||
| if (advice is not None): | ||
| advise_info = advice.get("adviseinfo") | ||
| if ( advise_info is not None): | ||
| response["current_indexes"] = advise_info.get("current_indexes", "No current indexes") | ||
| response["recommended_indexes"] = advise_info.get("recommended_indexes","No index recommendations available") | ||
| response["query"]=result[0].get("query","Query statement unavailable") | ||
| return response |
There was a problem hiding this comment.
The nested if checks here are redundant and make the code harder to read. The walrus operator := in the outer if condition already ensures that advice is a truthy value (not None) within the if block. You can simplify this logic by removing the redundant checks and flattening the structure.
if result and (advice := result[0].get("advice")) and (advise_info := advice.get("adviseinfo")):
response["current_indexes"] = advise_info.get("current_indexes", "No current indexes")
response["recommended_indexes"] = advise_info.get("recommended_indexes","No index recommendations available")
response["query"]=result[0].get("query","Query statement unavailable")
return response…icate path can now be included on initialization or env params. Refactor includes generic funciton for creating and sending API calls to couchbase with username/password and cert support. Use of tls is decide based on couchbase/couchbases protocol in connection string. If no certificate is used, verify certificates will be set to false.
|
Refactored the approach for API calls to include hostname retrieval and a generic method to generate the API url based on protocol and port. Certificates are now supported. |
src/mcp_server.py
Outdated
|
|
||
| username = ctx.request_context.lifespan_context.username | ||
| password = ctx.request_context.lifespan_context.password | ||
| ca_cert_path = ctx.request_context.lifespan_context.ca_cert_path or False |
There was a problem hiding this comment.
Using or False here creates a security risk by disabling SSL certificate verification when a CA path isn't provided. This is because verify=False is passed to requests.get, which instructs it to ignore certificate validation. It's much safer to default to using the system's trusted CAs. To fix this, remove or False from this line and then modify line 246 to handle the None case for ca_cert_path, for example: verify=ca_cert_path if ca_cert_path else True.
ca_cert_path = ctx.request_context.lifespan_context.ca_cert_pathThere was a problem hiding this comment.
Changed false-like values to True
src/mcp_server.py
Outdated
| if (response.json()): | ||
| return response.json()[0].get("targets") |
There was a problem hiding this comment.
Calling response.json() multiple times is problematic. The first call consumes the response body, so the second call will fail or return an empty result. Additionally, response.json()[0] will raise an IndexError if the JSON response is an empty list. To fix this, call response.json() once, store its result in a variable, and then perform the necessary checks and operations on that variable.
json_data = response.json()
if json_data and isinstance(json_data, list):
return json_data[0].get("targets")| hostname = strip_protocol(hostname) | ||
| parts = hostname.split(":",1) | ||
| hostname = parts[0] |
There was a problem hiding this comment.
This logic incorrectly discards the port from the hostname input. The code splits the hostname and port but then only uses the hostname part, effectively ignoring any port passed in. The build_api_url function then adds a default port. This contradicts the docstring, which states that the function accepts a hostname and port. The implementation should be corrected to respect the port if it's provided in the hostname argument.
There was a problem hiding this comment.
Fixed documentation, port is ignored
| cluster = ctx.request_context.lifespan_context.cluster | ||
| try: | ||
| bucket = cluster.bucket(bucket_name) | ||
| except Exception as e: | ||
| logger.error(f"Error accessing bucket: {e}") | ||
| raise ValueError("Tool does not have access to bucket, or bucket does not exist.") |
There was a problem hiding this comment.
This block of code for retrieving a bucket and handling errors is repeated in several functions (e.g., get_document_by_id, upsert_document_by_id, delete_document_by_id, run_sql_plus_plus_query, get_scopes_and_collections_in_bucket). To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider extracting this logic into a shared helper function.
| if result and (advice := result[0].get("advice")): | ||
| if (advice is not None): | ||
| advise_info = advice.get("adviseinfo") | ||
| if ( advise_info is not None): | ||
| response["current_indexes"] = advise_info.get("current_indexes", "No current indexes") | ||
| response["recommended_indexes"] = advise_info.get("recommended_indexes","No index recommendations available") | ||
| response["query"]=result[0].get("query","Query statement unavailable") |
There was a problem hiding this comment.
These nested if statements make the code difficult to read and can be simplified. The walrus operator := in the first if already ensures that advice is not None. The subsequent checks are redundant. You can flatten this structure for better readability and conciseness.
if result and (advice := result[0].get("advice")) and (advise_info := advice.get("adviseinfo")):
response["current_indexes"] = advise_info.get("current_indexes", "No current indexes")
response["recommended_indexes"] = advise_info.get("recommended_indexes","No index recommendations available")
response["query"]=result[0].get("query","Query statement unavailable")…internally. Fixed potential errors in get_cluster_prometheus_metrics_endpoints when parsing json response.
…ired now for call_api function
1 participant
Added the ability to call for a cursory heath status (ping report) on the couchbase cluster, getting IPs of nodes/services in the process.
Added the ability to call the /metrics endpoint of a node, given it's IP.