[ROB-1286] Updated holmes slack format#1826
Conversation
Summary by CodeRabbit
WalkthroughA new documentation section was added to guide users in enabling Holmes AI in Slack via the Robusta platform. The Slack integration now includes logic to detect and display Holmes AI prompts based on platform and Slackbot connection status, utilizing new methods for checking Holmes Slackbot connectivity and managing message construction accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Slack
participant SlackSender
participant RobustaSink
participant HolmesSlackbotAPI
User->>Slack: Mentions alert / triggers notification
Slack->>SlackSender: Send alert message
SlackSender->>RobustaSink: is_holmes_slackbot_connected()
RobustaSink->>HolmesSlackbotAPI: POST /check_slackbot (with session token, account id)
HolmesSlackbotAPI-->>RobustaSink: Response (connected/not connected)
RobustaSink-->>SlackSender: Return connection status
SlackSender->>Slack: Post message with Holmes AI prompt (if enabled)
User->>Slack: Interact with Holmes AI as instructed
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ 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: 0
🧹 Nitpick comments (4)
src/robusta/core/sinks/robusta/dal/supabase_dal.py (4)
757-759: Consider using self.account_id by default.The method takes
account_idas a parameter, but most other methods in this class useself.account_iddirectly. Unless there's a specific need to check Holmes Slackbot status for different accounts, consider using the instance's account ID by default.- def holmes_slackbot_enabled( - self, account_id: str - ) -> bool: + def holmes_slackbot_enabled(self) -> bool:And update the RPC call to use
self.account_id:res = self.client.rpc( "holmes_slackbot_enabled", - {"_account_id": account_id}, + {"_account_id": self.account_id}, ).execute()
757-759: Add method documentation.The method lacks a docstring explaining its purpose, parameters, and return value. This would improve code maintainability and help other developers understand the method's purpose.
def holmes_slackbot_enabled( self, account_id: str ) -> bool: + """ + Check if Holmes Slackbot is enabled for the given account. + + Args: + account_id: The account ID to check Holmes Slackbot status for + + Returns: + bool: True if Holmes Slackbot is enabled, False otherwise + + Raises: + Exception: If the RPC call fails + """
770-774: Improve error message clarity.The error message phrasing is slightly awkward and could be more descriptive.
logging.error( - f"Unexpected error rpc holmes_slackbot_enabled." - f"account_id: {account_id}", + f"Unexpected error in RPC call 'holmes_slackbot_enabled' " + f"for account_id: {account_id}", exc_info=True, )
760-775: Consider adding input validation.The method doesn't validate the
account_idparameter. Adding basic validation would improve robustness.def holmes_slackbot_enabled( self, account_id: str ) -> bool: + if not account_id or not account_id.strip(): + raise ValueError("account_id cannot be empty") + try:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/robusta/core/sinks/robusta/dal/supabase_dal.py(1 hunks)src/robusta/integrations/slack/sender.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/robusta/integrations/slack/sender.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (1)
src/robusta/core/sinks/robusta/dal/supabase_dal.py (1)
757-775: Implementation looks good overall.The method correctly implements the Holmes Slackbot status check following established patterns in the codebase. The RPC call structure, error handling, and boolean conversion are all appropriate.
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 (4)
docs/configuration/holmesgpt/index.rst(1 hunks)src/robusta/core/sinks/robusta/robusta_sink.py(2 hunks)src/robusta/core/sinks/slack/slack_sink.py(1 hunks)src/robusta/integrations/slack/sender.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/configuration/holmesgpt/index.rst
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/robusta/integrations/slack/sender.py (3)
src/robusta/model/config.py (3)
Registry(160-233)get_sinks(196-197)get_robusta_sinks(40-41)src/robusta/core/reporting/blocks.py (1)
MarkdownBlock(42-61)src/robusta/core/sinks/robusta/robusta_sink.py (1)
is_holmes_slackbot_connected(706-729)
🪛 Ruff (0.11.9)
src/robusta/integrations/slack/sender.py
676-676: SyntaxError: unindent does not match any outer indentation level
🪛 Pylint (3.3.7)
src/robusta/integrations/slack/sender.py
[error] 676-676: Parsing failed: 'unindent does not match any outer indentation level (src.robusta.integrations.slack.sender, line 676)'
(E0001)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: Deploy docs
🔇 Additional comments (8)
src/robusta/core/sinks/slack/slack_sink.py (1)
19-19: LGTM! Clean dependency injection.The registry parameter addition enables Holmes Slackbot integration by providing SlackSender access to other sinks for connectivity checks.
src/robusta/core/sinks/robusta/robusta_sink.py (1)
45-52: LGTM! Good caching strategy with configurable TTL.The cache setup with TTLCache and environment variable configuration is well-designed.
src/robusta/integrations/slack/sender.py (6)
51-51: LGTM! Proper import for dependency injection.The Registry import is necessary for the Holmes integration functionality.
65-65: LGTM! Constructor updated to accept registry parameter.The registry parameter addition enables access to other sinks for Holmes connectivity checks.
83-83: LGTM! Registry stored as instance attribute.Proper storage of the registry for later use in Holmes connectivity checks.
642-649: LGTM! Well-designed conditional messaging logic.The method provides appropriate guidance messages based on platform and Slackbot connectivity status. The logic covers all combinations correctly.
744-748: LGTM! Proper integration of Holmes block in message flow.The conditional logic correctly adds the Holmes guidance block when appropriate, maintaining clean message structure.
652-652:⚠️ Potential issueCRITICAL: Fix syntax error - missing function definition keyword.
The line appears to be missing the
defkeyword for the function definition.Apply this diff to fix the syntax error:
-def send_finding_to_slack( + def send_finding_to_slack(Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/robusta/integrations/slack/sender.py (1)
675-682:⚠️ Potential issueAddress the indentation issue and improve robustness.
This method has the same indentation and robustness issues flagged in the previous review that remain unresolved.
The method has indentation issues and lacks proper error handling. Apply this fix:
- def __is_holmes_slackbot_enabled(self) -> bool: - robusta_sinks = self.registry.get_sinks().get_robusta_sinks() - if not robusta_sinks: - logging.debug("No robusta sinks found, holmes not connected to slackbot") - return False - - robusta_sink = robusta_sinks[0] - return robusta_sink.is_holmes_slackbot_connected() + def __is_holmes_slackbot_enabled(self) -> bool: + try: + robusta_sinks = self.registry.get_sinks().get_robusta_sinks() + if not robusta_sinks: + logging.debug("No robusta sinks found, holmes not connected to slackbot") + return False + + robusta_sink = robusta_sinks[0] + return robusta_sink.is_holmes_slackbot_connected() + except Exception as e: + logging.debug(f"Error checking Holmes slackbot status: {e}") + return False
🧹 Nitpick comments (2)
src/robusta/integrations/slack/sender.py (2)
64-64: Consider refactoring the constructor to reduce parameter count.The constructor now accepts 8 parameters, which exceeds the recommended limit and affects readability. Consider using a configuration object or builder pattern to group related parameters.
- def __init__(self, slack_token: str, account_id: str, cluster_name: str, signing_key: str, slack_channel: str, registry, is_preview: bool = False): + def __init__(self, config: SlackSenderConfig):Where
SlackSenderConfigwould encapsulate the parameters:@dataclass class SlackSenderConfig: slack_token: str account_id: str cluster_name: str signing_key: str slack_channel: str registry: Any is_preview: bool = FalseAlso applies to: 82-82
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 64-64: Too many arguments (8/5)
(R0913)
[refactor] 64-64: Too many positional arguments (8/5)
(R0917)
641-649: Simplify control flow by removing unnecessary elif statements.The method contains unnecessary
elifstatements afterreturnstatements, which can be simplified for better readability.- def get_holmes_block(self, platform_enabled: bool, slackbot_enabled) -> Optional[MarkdownBlock]: - if not platform_enabled and not slackbot_enabled: - return MarkdownBlock("_Ask AI questions about this alert, by connecting <https://platform.robusta.dev/create-account|Robusta SaaS> and tagging @holmes._") - elif platform_enabled and not slackbot_enabled: - return MarkdownBlock("_Ask AI questions about this alert, by adding @holmes to your <https://docs.robusta.dev/master/configuration/holmesgpt/index.html#enable-holmes-in-slack-in-the-platform|Slack>._") - elif platform_enabled and slackbot_enabled: - return MarkdownBlock("_Ask AI questions about this alert, by tagging @holmes in a threaded reply_") - return None + def get_holmes_block(self, platform_enabled: bool, slackbot_enabled) -> Optional[MarkdownBlock]: + if not platform_enabled and not slackbot_enabled: + return MarkdownBlock("_Ask AI questions about this alert, by connecting <https://platform.robusta.dev/create-account|Robusta SaaS> and tagging @holmes._") + if platform_enabled and not slackbot_enabled: + return MarkdownBlock("_Ask AI questions about this alert, by adding @holmes to your <https://docs.robusta.dev/master/configuration/holmesgpt/index.html#enable-holmes-in-slack-in-the-platform|Slack>._") + if platform_enabled and slackbot_enabled: + return MarkdownBlock("_Ask AI questions about this alert, by tagging @holmes in a threaded reply_") + return None🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 642-647: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/robusta/core/sinks/robusta/robusta_sink.py(2 hunks)src/robusta/integrations/slack/sender.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/robusta/core/sinks/robusta/robusta_sink.py
🧰 Additional context used
🪛 Pylint (3.3.7)
src/robusta/integrations/slack/sender.py
[refactor] 64-64: Too many arguments (8/5)
(R0913)
[refactor] 64-64: Too many positional arguments (8/5)
(R0917)
[refactor] 642-647: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (1)
src/robusta/integrations/slack/sender.py (1)
743-747: LGTM! Clean integration of Holmes block logic.The conditional logic for adding the Holmes block is well-implemented and follows the existing pattern of conditionally appending blocks based on feature availability.
Avi-Robusta
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/robusta/integrations/slack/sender.py (1)
677-684: 🛠️ Refactor suggestionAdd error handling for robustness as suggested in past review.
The method logic is correct and indentation issues appear to be resolved. However, the past review comment about adding error handling for robustness should be addressed to prevent potential exceptions.
Apply this diff to add error handling as suggested in the past review:
def __is_holmes_slackbot_enabled(self) -> bool: + try: robusta_sinks = self.registry.get_sinks().get_robusta_sinks() if not robusta_sinks: logging.debug("No robusta sinks found, holmes not connected to slackbot") return False robusta_sink = robusta_sinks[0] return robusta_sink.is_holmes_slackbot_connected() + except Exception as e: + logging.debug(f"Error checking Holmes slackbot status: {e}") + return False
🧹 Nitpick comments (2)
src/robusta/integrations/slack/sender.py (2)
64-64: Constructor parameter addition is reasonable but could be improved.The addition of the
registryparameter is appropriate for the Holmes integration functionality. However, consider using keyword-only arguments for better maintainability given the growing parameter count.Consider refactoring to use keyword-only arguments:
- def __init__(self, slack_token: str, account_id: str, cluster_name: str, signing_key: str, slack_channel: str, registry, is_preview: bool = False): + def __init__(self, slack_token: str, account_id: str, cluster_name: str, signing_key: str, slack_channel: str, *, registry, is_preview: bool = False):🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 64-64: Too many arguments (8/5)
(R0913)
[refactor] 64-64: Too many positional arguments (8/5)
(R0917)
643-651: Simplify the conditional logic in get_holmes_block.The method logic is sound, but the elif chain can be simplified as suggested by static analysis tools. The current logic correctly handles different combinations of platform and slackbot enablement states.
Apply this diff to simplify the conditional logic:
def get_holmes_block(self, platform_enabled: bool, slackbot_enabled) -> Optional[MarkdownBlock]: - if not platform_enabled and not slackbot_enabled: + if not platform_enabled: return MarkdownBlock("_Ask AI questions about this alert, by connecting <https://platform.robusta.dev/create-account|Robusta SaaS> and tagging @holmes._") - elif platform_enabled and not slackbot_enabled: + if not slackbot_enabled: return MarkdownBlock("_Ask AI questions about this alert, by adding @holmes to your <https://docs.robusta.dev/master/configuration/holmesgpt/index.html#enable-holmes-in-slack-in-the-platform|Slack>._") - elif platform_enabled and slackbot_enabled: - return MarkdownBlock("_Ask AI questions about this alert, by tagging @holmes in a threaded reply_") - return None + return MarkdownBlock("_Ask AI questions about this alert, by tagging @holmes in a threaded reply_")🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 644-649: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/robusta/integrations/slack/sender.py(5 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
src/robusta/integrations/slack/sender.py
[refactor] 64-64: Too many arguments (8/5)
(R0913)
[refactor] 64-64: Too many positional arguments (8/5)
(R0917)
[refactor] 644-649: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run_tests
- GitHub Check: run_tests
- GitHub Check: Deploy docs
🔇 Additional comments (2)
src/robusta/integrations/slack/sender.py (2)
82-82: Registry storage implementation is correct.The registry instance variable is properly initialized and will be used by the Holmes integration methods.
745-749: Holmes block integration is well implemented.The integration of Holmes block generation into the main message flow is clean and follows good practices. The conditional check ensures the block is only added when appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/robusta/integrations/slack/sender.py (1)
801-804:__create_holmes_callbackis gone but still referenced – runtimeAttributeError
__send_finding_to_slack_previewcalls an instance method that no longer exists, which will explode the first time a preview message is rendered.- if HOLMES_ENABLED and HOLMES_ASK_SLACK_BUTTON_ENABLED: - blocks.append(self.__create_holmes_callback(finding)) + holmes_block = self.get_holmes_block( + platform_enabled, self.__is_holmes_slackbot_enabled() + ) + if holmes_block: + blocks.append(holmes_block)Removes the stale dependency and re-uses the new unified helper.
♻️ Duplicate comments (2)
tests/test_slack_preview.py (2)
32-36: Duplicate of the previous suggestion – same reasoning applies here.
84-88: Duplicate of the previous suggestion – same reasoning applies here.
🧹 Nitpick comments (2)
src/robusta/integrations/slack/sender.py (1)
643-651: Minor: simplify conditional chainYou can drop the
elifladders after eachreturnfor clearer flow:- if not platform_enabled and not slackbot_enabled: + if not platform_enabled and not slackbot_enabled: return MarkdownBlock("_Ask AI questions about this alert, by connecting <https://platform.robusta.dev/create-account|Robusta SaaS> and tagging @holmes._") - elif platform_enabled and not slackbot_enabled: + if platform_enabled and not slackbot_enabled: return MarkdownBlock("_Ask AI questions about this alert, by adding @holmes to your <https://docs.robusta.dev/master/configuration/holmesgpt/index.html#enable-holmes-in-slack-in-the-platform|Slack>._") - elif platform_enabled and slackbot_enabled: + if platform_enabled and slackbot_enabled: return MarkdownBlock("_Ask AI questions about this alert, by tagging @holmes in a threaded reply_") return NonePure style, no functional change.
tests/test_blocks.py (1)
32-36: Prefer keyword args for long positional listsThe sixth positional arg is easy to mis-order. Using keywords for all non-obvious params improves readability and shields you from future signature tweaks:
- slack_sender = SlackSender( - CONFIG.PYTEST_IN_CLUSTER_SLACK_TOKEN, TEST_ACCOUNT, TEST_CLUSTER, TEST_KEY, slack_channel.channel_name, registry=None - ) + slack_sender = SlackSender( + slack_token=CONFIG.PYTEST_IN_CLUSTER_SLACK_TOKEN, + account_id=TEST_ACCOUNT, + cluster_name=TEST_CLUSTER, + signing_key=TEST_KEY, + slack_channel=slack_channel.channel_name, + registry=None, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/robusta/integrations/slack/sender.py(5 hunks)tests/manual_tests/test_slack_integration_manual.py(2 hunks)tests/test_blocks.py(2 hunks)tests/test_slack.py(6 hunks)tests/test_slack_preview.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/test_slack.py
🧰 Additional context used
🪛 Pylint (3.3.7)
src/robusta/integrations/slack/sender.py
[refactor] 64-64: Too many arguments (8/5)
(R0913)
[refactor] 64-64: Too many positional arguments (8/5)
(R0917)
[refactor] 644-649: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (1)
tests/manual_tests/test_slack_integration_manual.py (1)
439-457: Instantiation updated correctly
SlackSendernow receivesregistry=None, matching the new ctor signature. No further action needed.
2 participants
removed ask holmes button
link to docs