Skip to content

Conversation

aantn
Copy link
Contributor

@aantn aantn commented Sep 25, 2025

I've tested in the case where SaaS is not available (i.e. no token) - please also test on happy path where it is available and verify everything still works.

@aantn aantn requested a review from arikalon1 September 25, 2025 06:19
Copy link
Contributor

coderabbitai bot commented Sep 25, 2025

Walkthrough

Conditionalize the Robusta sync at server startup to run only when dal.enabled and config.cluster_name are set; otherwise log a debug skip. Adjust Sentry tag initialization so account_id_tag uses dal.account_id only when dal.enabled is true; otherwise None.

Changes

Cohort / File(s) Summary
Server startup sync gating
server.py
sync_before_server_start now calls Robusta sync only if dal.enabled and config.cluster_name are truthy; otherwise logs a debug message and skips.
Sentry tag initialization
server.py
account_id_tag is set to dal.account_id when dal.enabled is true; otherwise set to None before sending to Sentry.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Server
    participant DAL
    participant RobustaSync as Robusta Sync
    participant Sentry

    rect rgba(200,230,255,0.3)
    note over Server: Startup
    Server->>DAL: Check dal.enabled
    Server->>Server: Read config.cluster_name
    alt dal.enabled && cluster_name
        Server->>RobustaSync: Run sync()
        RobustaSync-->>Server: Result/complete
    else not enabled or missing cluster_name
        Server-->>Server: Log debug "skip sync"
    end
    end

    rect rgba(220,255,220,0.3)
    note over Server,Sentry: Initialize Sentry tags
    Server->>DAL: Get account_id (if enabled)
    alt dal.enabled
        Server->>Sentry: setTag(account_id = dal.account_id)
    else
        Server->>Sentry: setTag(account_id = None)
    end
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly captures the primary change of reducing server errors when the service is not connected to SaaS by conditionally skipping synchronization steps, clearly reflecting the pull request’s main effect.
Description Check ✅ Passed The description directly addresses testing both the absence and presence of SaaS connectivity, which aligns with the changeset’s goal of handling missing tokens and ensuring functionality remains intact.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch server-errors-no-saas

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
server.py (2)

84-84: Add a return type hint to comply with typing guidelines.

Recommend annotating the function return type.

-def sync_before_server_start():
+def sync_before_server_start() -> None:

114-121: Avoid sending None-valued tags to Sentry; set account_id only when enabled.

set_tags expects string values; passing None can result in an unintended "None"/null tag or type issues. Build the tag map conditionally.

-        sentry_sdk.set_tags(
-            {
-                "account_id": dal.account_id if dal.enabled else None,
-                "cluster_name": config.cluster_name,
-                "version": get_version(),
-                "environment": environment,
-            }
-        )
+        tags = {
+            "cluster_name": config.cluster_name,
+            "version": get_version(),
+            "environment": environment,
+        }
+        if dal.enabled and getattr(dal, "account_id", None):
+            tags["account_id"] = str(dal.account_id)
+        sentry_sdk.set_tags(tags)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 85ce5b3 and a059933.

📒 Files selected for processing (1)
  • server.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)

Files:

  • server.py
🧬 Code graph analysis (1)
server.py (3)
holmes/config.py (1)
  • dal (120-123)
holmes/utils/holmes_status.py (1)
  • update_holmes_status_in_db (8-23)
holmes/utils/holmes_sync_toolsets.py (1)
  • holmes_sync_toolsets_status (24-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: llm_evals
  • GitHub Check: Pre-commit checks
🔇 Additional comments (1)
server.py (1)

85-98: SaaS-gated startup sync looks good; reduces failures when no token/cluster name.

This should eliminate the startup exceptions when SaaS isn’t configured. Please verify that constructing config.dal itself does not perform network I/O or raise when the token is missing; otherwise consider deferring DAL construction until inside the gated block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant