Allow ignoring sinks init errors #1915
Conversation
Fix finding dates timezone issues
WalkthroughAdds a configurable option to continue startup when sink initialization fails, surfaces a sink-init-error flag on the registry and ActivityStats, wires the flag into cluster status updates, makes timestamps UTC-aware, and updates docs and tests. No public API removals. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CL as ConfigLoader
participant SR as SinksRegistry
participant REG as Registry
participant RS as RobustaSink
participant CS as ActivityStats
Note over CL,SR: Runtime startup — prepare runtime config
CL->>SR: construct_new_sinks(config, existing, registry, continue_on_sink_errors)
activate SR
loop for each sink config
SR->>SR: try initialize sink
alt init succeeds
SR-->>SR: add/update sink
else init fails
alt continue_on_sink_errors == true
SR-->>SR: skip sink, mark has_sink_errors = true
else
SR-->>CL: raise exception (startup abort)
end
end
end
SR-->>CL: (new_sinks, has_sink_errors)
deactivate SR
CL->>REG: set_sink_initialization_errors(has_sink_errors)
REG-->>CL: ack
Note over RS,CS: periodic cluster status update
RS->>REG: has_sink_initialization_errors()
REG-->>RS: bool
RS->>CS: Construct ActivityStats(..., sinksInitializationErrors=bool)
CS-->>RS: payload emitted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing Touches
🧪 Generate unit tests
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. Comment |
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/notification-routing/configuring-sinks.rst (1)
93-108: Tighten grammar and style; keep “sinks” lowercase; YAML booleans lowercaseMinor edits improve clarity and match existing style.
-By default, when Robusta fails to initialize any of the Sinks, it will not start. +By default, when Robusta fails to initialize any of the sinks, it will not start. -On some scenarios, you may want to ignore Sinks initialization errors. +In some scenarios, you may want to ignore sinks initialization errors. For example, if Robusta is not allowed to connect to Slack, but you still want to receive notifications on the Robusta UI. In order to enable that, add the below to ``globalConfig`` in your ``generated_values.yaml`` file: .. code-block:: yaml globalConfig: - continue_on_sink_errors: True + continue_on_sink_errors: truesrc/robusta/runner/config_loader.py (1)
290-300: Record and surface sink init errors in logs during reloadThe new return signature and registry flagging look good. Log when errors occurred so operators see it without checking the UI.
) -> tuple[SinksRegistry, PlaybooksRegistry]: existing_sinks = sinks_registry.get_all() if sinks_registry else {} new_sinks, has_sink_errors = SinksRegistry.construct_new_sinks( runner_config.sinks_config, existing_sinks, registry, runner_config.global_config.get("continue_on_sink_errors", False) ) sinks_registry = SinksRegistry(new_sinks) registry.set_sink_initialization_errors(has_sink_errors) + if has_sink_errors: + logging.warning("One or more sinks failed to initialize. Continuing due to continue_on_sink_errors.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/notification-routing/configuring-sinks.rst(1 hunks)src/robusta/core/model/cluster_status.py(1 hunks)src/robusta/core/reporting/base.py(2 hunks)src/robusta/core/sinks/robusta/dal/model_conversion.py(2 hunks)src/robusta/core/sinks/robusta/robusta_sink.py(1 hunks)src/robusta/model/config.py(6 hunks)src/robusta/runner/config_loader.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/robusta/core/sinks/robusta/robusta_sink.py (1)
src/robusta/model/config.py (1)
has_sink_initialization_errors(250-251)
src/robusta/core/sinks/robusta/dal/model_conversion.py (1)
src/robusta/utils/parsing.py (1)
datetime_to_db_str(52-56)
src/robusta/model/config.py (4)
src/robusta/core/sinks/sink_base.py (3)
SinkBase(51-185)is_global_config_changed(154-160)stop(162-163)src/robusta/core/sinks/sink_factory.py (2)
SinkFactory(37-70)create_sink(66-70)src/robusta/core/sinks/sink_config.py (2)
get_params(14-15)get_name(9-11)src/robusta/core/sinks/robusta/robusta_sink.py (1)
stop(218-219)
src/robusta/runner/config_loader.py (1)
src/robusta/model/config.py (4)
SinksRegistry(25-99)get_all(37-38)construct_new_sinks(44-99)set_sink_initialization_errors(247-248)
⏰ 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). (4)
- GitHub Check: run_tests
- GitHub Check: run_tests
- GitHub Check: Deploy docs
- GitHub Check: Deploy docs
🔇 Additional comments (10)
src/robusta/core/reporting/base.py (1)
7-7: UTC-aware timestamps import looks goodImporting timezone alongside datetime is appropriate for standardizing UTC usage.
src/robusta/core/sinks/robusta/dal/model_conversion.py (3)
5-5: Importing timezone for UTC-aware now() usageGood addition to support aware timestamps.
54-56: Use UTC-aware updated_atSwitching updated_at to datetime.now(timezone.utc) aligns with DB format expectations (%z).
121-121: Ensure all “createdAt” timestamps use UTC
I couldn’t locate any otherdatetime.now()calls in the repo; please manually verify that the instances at line 121 and line 158 insrc/robusta/core/sinks/robusta/dal/model_conversion.pyare updated todatetime.now(timezone.utc)for consistency.src/robusta/core/model/cluster_status.py (1)
26-26: New ActivityStats flag is sensible and backward-compatibleAdding sinksInitializationErrors: bool = False extends the schema safely with a default.
src/robusta/core/sinks/robusta/robusta_sink.py (1)
566-567: Propagating sinksInitializationErrors into ActivityStatsGood wiring from the registry to surface sink init health in cluster status.
src/robusta/model/config.py (4)
44-51: Extend construct_new_sinks signature to support continue-on-errorAccepting continue_on_sink_errors and returning (new_sinks, has_sink_errors) is the right API.
60-60: Aggregate sink error stateInitializing has_sink_errors is fine; see next comment for update-path robustness.
185-185: Registry: add sink init error stateDefaulting _sink_initialization_errors to False is fine.
247-251: Accessor methods for sink init error stateClear and minimal API; works with RobustaSink usage.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/config/test_sink_registry.py (3)
35-38: Unpack construct_new_sinks return; you’re treating a tuple as a dict.construct_new_sinks now returns (new_sinks: Dict, has_sink_errors: bool). This causes AttributeError on .keys().
- new_sinks = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry) + new_sinks, _ = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry)
41-46: Same tuple unpacking bug here.- new_sinks = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry) + new_sinks, _ = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry)
48-54: And here as well; tuple indexing TypeError on line 52.- new_sinks = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry) + new_sinks, _ = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry)
🧹 Nitpick comments (2)
tests/config/test_sink_registry.py (2)
27-27: Fix flake8 E111: reindent to 8 spaces.Line 27 is indented with a non-multiple of 4. Align with the surrounding block.
- new_sinks, _ = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, {}, self.registry) + new_sinks, _ = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, {}, self.registry)
23-29: Make sink initialization resilient by passingcontinue_on_sink_errors=Trueand unpacking the returned errors
- Update every call to
SinksRegistry.construct_new_sinksto:
- Add the
continue_on_sink_errors=Truekeyword argument- Unpack both return values as
(new_sinks, has_sink_errors)Call sites to update:
- tests/config/test_sink_registry.py at lines 27, 37, 44, 51
- src/robusta/runner/config_loader.py at line 292
Example diff (test at line 27):
- new_sinks, _ = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, {}, self.registry) + new_sinks, _ = SinksRegistry.construct_new_sinks( + self.runner_config.sinks_config, {}, self.registry, continue_on_sink_errors=True + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/robusta/core/reporting/base.py(2 hunks)src/robusta/core/sinks/robusta/dal/model_conversion.py(2 hunks)src/robusta/utils/time_utils.py(1 hunks)tests/config/test_sink_registry.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/robusta/core/reporting/base.py
- src/robusta/core/sinks/robusta/dal/model_conversion.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/config/test_sink_registry.py (1)
src/robusta/model/config.py (2)
SinksRegistry(25-99)construct_new_sinks(44-99)
🪛 Flake8 (7.2.0)
tests/config/test_sink_registry.py
[error] 27-27: indentation is not a multiple of 4
(E111)
🪛 GitHub Actions: Test robusta with pytest
tests/config/test_sink_registry.py
[error] 38-38: AttributeError: 'tuple' object has no attribute 'keys' while constructing new sinks; SinksRegistry.construct_new_sinks returned a tuple instead of a dict.
[error] 45-45: AttributeError: 'tuple' object has no attribute 'keys'.
[error] 52-52: TypeError: tuple indices must be integers or slices, not str when indexing new_sinks by key; construct_new_sinks returns a tuple instead of a dict.
🔇 Additional comments (1)
src/robusta/utils/time_utils.py (1)
4-5: LGTM: returns tz-aware UTC datetime.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/config/test_sink_registry.py (1)
21-54: Fix PEP8 indentation (E111) to unblock CIFlake8 flags multiple E111 errors. Re-indent class body with 4 spaces.
class TestSinkFactory: - def setup_method(self, method): - self.config_data = yaml.safe_load(CONFIG) - self.runner_config = RunnerConfig(**self.config_data) - self.registry = Registry() - new_sinks, _ = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, {}, self.registry) - self.sinks_registry = SinksRegistry(new_sinks) + def setup_method(self, method): + self.config_data = yaml.safe_load(CONFIG) + self.runner_config = RunnerConfig(**self.config_data) + self.registry = Registry() + new_sinks, _ = SinksRegistry.construct_new_sinks( + self.runner_config.sinks_config, {}, self.registry + ) + self.sinks_registry = SinksRegistry(new_sinks) - def test_create_sinks(self): - assert list(self.sinks_registry.sinks.keys()) == ["my_sink1", "my_sink2"] + def test_create_sinks(self): + assert list(self.sinks_registry.sinks.keys()) == ["my_sink1", "my_sink2"] - def test_delete_sinks(self): - del self.runner_config.sinks_config[0] - new_sinks, _ = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry) - assert list(new_sinks.keys()) == ["my_sink2"] + def test_delete_sinks(self): + del self.runner_config.sinks_config[0] + new_sinks, _ = SinksRegistry.construct_new_sinks( + self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry + ) + assert list(new_sinks.keys()) == ["my_sink2"] - def test_add_new_sinks(self): - self.config_data["sinks_config"].insert(1, {'mail_sink': {'name': 'my_sink3', 'mailto': 'mailtos://user:password@example.com?from=a@x&to=b@y'}}) - self.runner_config = RunnerConfig(**self.config_data) - new_sinks, _ = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry) - assert list(new_sinks.keys()) == ["my_sink1", "my_sink3", "my_sink2"] + def test_add_new_sinks(self): + self.config_data["sinks_config"].insert( + 1, + { + "mail_sink": { + "name": "my_sink3", + "mailto": "mailtos://user:password@example.com?from=a@x&to=b@y", + } + }, + ) + self.runner_config = RunnerConfig(**self.config_data) + new_sinks, _ = SinksRegistry.construct_new_sinks( + self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry + ) + assert list(new_sinks.keys()) == ["my_sink1", "my_sink3", "my_sink2"] - def test_sink_config_changed(self): - new_email = "dev@robusta.dev" - self.runner_config.sinks_config[0].mail_sink.mailto = new_email - new_sinks, _ = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry) - assert new_sinks["my_sink1"].params.mailto == new_email - assert list(new_sinks.keys()) == ["my_sink1", "my_sink2"] + def test_sink_config_changed(self): + new_email = "dev@robusta.dev" + self.runner_config.sinks_config[0].mail_sink.mailto = new_email + new_sinks, _ = SinksRegistry.construct_new_sinks( + self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry + ) + assert new_sinks["my_sink1"].params.mailto == new_email + assert list(new_sinks.keys()) == ["my_sink1", "my_sink2"]
🧹 Nitpick comments (5)
tests/config/test_sink_registry.py (5)
27-28: Don’t silently discard has_sink_errors; assert it’s FalseCapture and assert the flag to catch unexpected init failures.
- new_sinks, _ = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, {}, self.registry) + new_sinks, has_errors = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, {}, self.registry) + assert not has_errors
37-38: Also validate has_sink_errors hereKeeps tests honest when a sink starts failing.
- new_sinks, _ = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry) + new_sinks, has_errors = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry) + assert not has_errors
44-45: Validate has_sink_errors on add flowPrevents false positives when a new sink fails to init.
- new_sinks, _ = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry) + new_sinks, has_errors = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry) + assert not has_errors
51-53: Validate has_sink_errors on change flowEnsures updates don’t hide init errors.
- new_sinks, _ = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry) + new_sinks, has_errors = SinksRegistry.construct_new_sinks(self.runner_config.sinks_config, self.sinks_registry.sinks, self.registry) + assert not has_errors
31-31: Add a targeted test for continue_on_sink_errors=TrueOptional but valuable: simulate one failing sink and verify it’s skipped and has_sink_errors=True. I can open a follow-up PR if helpful.
# Add alongside the class (pytest style) or as a method with `self, monkeypatch`. def test_construct_new_sinks_skips_failed_sink_when_continue_true(monkeypatch): from robusta.model import config as cfg # Keep one valid sink and one that will fail config_data = yaml.safe_load(CONFIG) config_data["sinks_config"].insert(0, {"mail_sink": {"name": "bad_sink", "mailto": "mailtos://bad"}}) runner_config = RunnerConfig(**config_data) registry = Registry() original = cfg.SinkFactory.create_sink def fake_create_sink(sink_config, registry_): if sink_config.get_name() == "bad_sink": raise RuntimeError("boom") return original(sink_config, registry_) monkeypatch.setattr(cfg.SinkFactory, "create_sink", fake_create_sink) new_sinks, has_errors = cfg.SinksRegistry.construct_new_sinks( runner_config.sinks_config, {}, registry, continue_on_sink_errors=True ) assert has_errors is True assert "bad_sink" not in new_sinks assert "my_sink1" in new_sinks and "my_sink2" in new_sinks
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/config/test_sink_registry.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/config/test_sink_registry.py (1)
src/robusta/model/config.py (2)
SinksRegistry(25-99)construct_new_sinks(44-99)
🪛 Flake8 (7.2.0)
tests/config/test_sink_registry.py
[error] 27-27: indentation is not a multiple of 4
(E111)
[error] 37-37: indentation is not a multiple of 4
(E111)
[error] 38-38: indentation is not a multiple of 4
(E111)
[error] 41-41: indentation is not a multiple of 4
(E111)
[error] 42-42: indentation is not a multiple of 4
(E111)
[error] 43-43: indentation is not a multiple of 4
(E111)
[error] 44-44: indentation is not a multiple of 4
(E111)
[error] 45-45: indentation is not a multiple of 4
(E111)
[error] 48-48: indentation is not a multiple of 4
(E111)
[error] 49-49: indentation is not a multiple of 4
(E111)
[error] 50-50: indentation is not a multiple of 4
(E111)
[error] 51-51: indentation is not a multiple of 4
(E111)
⏰ 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: run_tests
- GitHub Check: run_tests
Fix finding dates timezone issues