Add orchestrator jobs data to alerts #1997
Conversation
quickcoffee
had a problem deploying
to
elementary_test_env
GitHub Actions
Error
📝 WalkthroughWalkthroughAdds optional job/orchestrator fields to alert payloads, exposes them via AlertModel.orchestrator_info, adds Icon.LINK and its Unicode mapping, provides orchestrator link utilities, renders job/orchestrator links in alert subtitles, updates dbt macros/models to populate invocation fields, and adds unit tests. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant DBT as DBT (macros/models)
participant Fetcher as Fetcher (alert_data)
participant Alert as AlertModel
participant Builder as AlertMessageBuilder
participant Utils as OrchestratorLinkUtils
participant Renderer as Message Renderer
DBT->>Fetcher: emit rows including invocation fields
Fetcher->>Alert: construct alert with job/orchestrator fields
Alert-->>Builder: orchestrator_info (OrchestratorInfo or None)
alt orchestrator_info present
Builder->>Utils: create_job_link(orchestrator_info)
Builder->>Utils: create_orchestrator_link(orchestrator_info)
Utils-->>Builder: OrchestratorLinkData (optional)
Builder->>Renderer: LinesBlock (inline Job line + links)
else no orchestrator_info
Builder->>Renderer: LinesBlock (summary + existing links)
end
Renderer-->>Builder: formatted message (e.g., Block Kit)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (5)
🔇 Additional comments (5)
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 |
|
👋 @quickcoffee |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
elementary/monitor/dbt_project/macros/get_source_freshness_results.sql (1)
43-53: Field mapping bugs: owners and filter keys are mismatched.
- owners: SELECT aliases
sources.owner as ownersbut later readsget('owner').- filter fallback: SELECT aliases
source_freshness_filterbut later readsget('freshness_filter').Both cause data loss in the returned dict.
- sources.owner as owners, + sources.owner as owners, @@ - 'owners': source_freshness_result_dict.get('owner'), + 'owners': source_freshness_result_dict.get('owners'), @@ - 'filter': filter if filter is not none else source_freshness_result_dict.get('freshness_filter'), + 'filter': filter if filter is not none else source_freshness_result_dict.get('source_freshness_filter'),Also applies to: 95-95, 99-99
🧹 Nitpick comments (12)
elementary/monitor/dbt_project/macros/utils/normalized_source_freshness_status.sql (1)
1-7: Make normalization robust to case/whitespace.Current comparisons are case-sensitive. Trim/lower the input to avoid surprises from upstream variations.
-{% macro normalized_source_freshness_status(status_column='status') %} +{% macro normalized_source_freshness_status(status_column='status') %} case - when {{ status_column }} = 'error' then 'fail' - when {{ status_column }} = 'runtime error' then 'error' - else {{ status_column }} + when trim(lower({{ status_column }})) = 'error' then 'fail' + when trim(lower({{ status_column }})) = 'runtime error' then 'error' + else {{ status_column }} end as normalized_status {% endmacro %}elementary/monitor/alerts/source_freshness_alert.py (1)
49-55: Forwarding orchestrator fields — good; include them in.datafor parity.Fields are accepted and forwarded to the base. Expose them in
datato keep serialization consistent across alert types.return dict( id=self.id, alert_class_id=self.alert_class_id, model_unique_id=self.model_unique_id, detected_at=self.detected_at_str, database_name=self.database_name, schema_name=self.schema_name, owners=self.owners, tags=self.tags, subscribers=self.subscribers, status=self.status, suppression_interval=self.suppression_interval, source_name=self.source_name, identifier=self.identifier, original_status=self.original_status, error_after=self.error_after, warn_after=self.warn_after, path=self.path, error=self.error, snapshotted_at=self.snapshotted_at_str, max_loaded_at=self.max_loaded_at, max_loaded_at_time_ago_in_s=self.max_loaded_at_time_ago_in_s, filter=self.filter, freshness_description=self.freshness_description, + orchestrator=getattr(self, "orchestrator", None), + job_id=getattr(self, "job_id", None), + job_name=getattr(self, "job_name", None), + job_run_id=getattr(self, "job_run_id", None), + job_url=getattr(self, "job_url", None), + job_run_url=getattr(self, "job_run_url", None), )If downstream renderers rely on
alert.orchestrator_infoinstead, confirm they don’t expect it insidedata.Also applies to: 75-81
elementary/monitor/dbt_project/macros/alerts/population/source_freshness_alerts.sql (1)
132-133: Consider filtering by normalized status for consistency.
Using rawresults.statusmay miss adapter-specific variants; prefer the normalized form if available.Possible change:
- where lower(results.status) != 'pass' + where lower({{ elementary_cli.normalized_source_freshness_status('results.status') }}) != 'pass'elementary/monitor/dbt_project/macros/alerts/population/test_alerts.sql (2)
123-124: Explicit status filter is correct; consider normalization.
Optional: normalize status to be adapter-agnostic.
162-169: Join may duplicate alerts if multiple run results per execution — verify and dedupe if needed.
If duplication occurs, select the latest invocation pertest_execution_id(e.g., window/QUALIFY).Also applies to: 172-173
elementary/monitor/dbt_project/macros/alerts/population/model_alerts.sql (1)
145-151: Join/run invocation wiring — verify 1:1 mapping and dedupe if necessary.
If multiple run_result rows exist peralert_id, consider windowing to latest invocation before the join.Also applies to: 154-155
elementary/monitor/data_monitoring/alerts/integrations/utils/orchestrator_link.py (2)
14-29: Use display-friendly orchestrator names consistently.
create_orchestrator_linkshould mirror the title-casing used increate_job_link.- orchestrator = orchestrator_info.get("orchestrator", "orchestrator") + orchestrator = orchestrator_info.get("orchestrator", "orchestrator") + display_name = orchestrator.replace("_", " ").title() ... - text=f"View in {orchestrator}", + text=f"View in {display_name}",
31-50: Fallback to job_id when job_name is missing; minor typing polish.
Improves robustness for orchestrators that don’t populate names.- job_name = orchestrator_info.get("job_name", "Job") + job_name = orchestrator_info.get("job_name") or orchestrator_info.get("job_id") or "Job"Optional: change parameter type to
Mapping[str, str]for flexibility.tests/unit/alerts/alert_messages/test_orchestrator_message_simple.py (1)
91-98: Consider asserting structure rather than string repr.Matching on
str(slack_message)is brittle. Prefer asserting presence of aLinkBlockwith the expected text/URL.Example:
- slack_message = format_block_kit(message_body) - slack_json = str(slack_message) - assert "View in Dbt Cloud" in slack_json + slack_message = format_block_kit(message_body) + # walk blocks to find any LinkBlock with expected text + assert any( + getattr(inline, "text", None) == "View in Dbt Cloud" + for block in slack_message.blocks + for line in getattr(block, "lines", []) + for inline in getattr(line, "inlines", []) + )elementary/monitor/alerts/alert_messages/builder.py (1)
104-114: Avoid mutable default for links and rename param to avoid shadowing built-in.Use
links: Optional[list[ReportLinkData]] = Noneand set inside. Alsotypeshadows the built-in; preferasset_type.Apply:
- def _get_run_alert_subtitle_block( - self, - type: Literal["test", "snapshot", "model", "source"], + def _get_run_alert_subtitle_block( + self, + asset_type: Literal["test", "snapshot", "model", "source"], name: str, status: Optional[str] = None, detected_at_str: Optional[str] = None, suppression_interval: Optional[int] = None, env: Optional[str] = None, - links: list[ReportLinkData] = [], + links: Optional[list[ReportLinkData]] = None, orchestrator_info: Optional[Dict[str, str]] = None, ) -> LinesBlock: + links = links or [] summary = [] - summary.append((type.capitalize() + ":", name)) + summary.append((asset_type.capitalize() + ":", name))And update the call site below:
- type=asset_type, + asset_type=asset_type,elementary/monitor/alerts/model_alert.py (2)
66-73: Forwarding to base with explicit keywords — LGTM.Keeps signature parity and future-proofs against ordering drift in
AlertModel.If you want to reduce positional coupling further, consider switching the earlier positional args to keyword args in the
super().__init__call for clarity and resilience.
81-101: Confirm whether orchestrator fields must be surfaced indata.
dataomitsorchestrator,job_id,job_run_id,job_url,job_run_url. If downstream serialization/templates rely solely onAlertModel.data, consider adding them; if renderers read fromorchestrator_infoon the instance, current behavior is fine. Please verify consumers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
elementary/messages/blocks.py(1 hunks)elementary/messages/formats/unicode.py(1 hunks)elementary/monitor/alerts/alert.py(3 hunks)elementary/monitor/alerts/alert_messages/builder.py(6 hunks)elementary/monitor/alerts/model_alert.py(2 hunks)elementary/monitor/alerts/source_freshness_alert.py(2 hunks)elementary/monitor/alerts/test_alert.py(2 hunks)elementary/monitor/data_monitoring/alerts/integrations/utils/orchestrator_link.py(1 hunks)elementary/monitor/dbt_project/macros/alerts/population/model_alerts.sql(4 hunks)elementary/monitor/dbt_project/macros/alerts/population/source_freshness_alerts.sql(4 hunks)elementary/monitor/dbt_project/macros/alerts/population/test_alerts.sql(4 hunks)elementary/monitor/dbt_project/macros/get_source_freshness_results.sql(1 hunks)elementary/monitor/dbt_project/macros/utils/normalized_source_freshness_status.sql(1 hunks)elementary/monitor/dbt_project/models/alerts/alerts_v2.sql(1 hunks)elementary/monitor/fetchers/alerts/schema/alert_data.py(4 hunks)tests/unit/alerts/alert_messages/test_orchestrator_message_simple.py(1 hunks)tests/unit/alerts/test_orchestrator_integration.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
elementary/monitor/data_monitoring/alerts/integrations/utils/orchestrator_link.py (2)
elementary/messages/blocks.py (1)
Icon(8-23)elementary/monitor/alerts/alert.py (1)
orchestrator_info(101-118)
elementary/messages/formats/unicode.py (1)
elementary/messages/blocks.py (1)
Icon(8-23)
tests/unit/alerts/test_orchestrator_integration.py (3)
elementary/messages/blocks.py (1)
Icon(8-23)elementary/monitor/data_monitoring/alerts/integrations/utils/orchestrator_link.py (3)
OrchestratorLinkData(7-11)create_job_link(31-49)create_orchestrator_link(14-28)elementary/monitor/alerts/alert.py (1)
orchestrator_info(101-118)
tests/unit/alerts/alert_messages/test_orchestrator_message_simple.py (5)
elementary/messages/formats/block_kit.py (1)
format_block_kit(360-364)elementary/monitor/alerts/model_alert.py (2)
ModelAlertModel(14-116)get_report_link(115-116)elementary/monitor/alerts/test_alert.py (2)
TestAlertModel(13-201)get_report_link(200-201)tests/unit/alerts/alert_messages/test_alert_utils.py (1)
get_alert_message_body(305-316)elementary/monitor/alerts/alert.py (2)
get_report_link(97-98)orchestrator_info(101-118)
elementary/monitor/alerts/alert_messages/builder.py (4)
elementary/messages/block_builders.py (4)
BoldTextBlock(63-64)LinkInlineBlocks(85-92)SummaryLineBlock(109-122)LinksLineBlock(99-106)elementary/monitor/data_monitoring/alerts/integrations/utils/orchestrator_link.py (1)
create_orchestrator_link(14-28)elementary/monitor/alerts/alert.py (2)
orchestrator_info(101-118)summary(94-95)elementary/messages/blocks.py (2)
TextBlock(40-43)LineBlock(67-70)
🔇 Additional comments (29)
elementary/monitor/dbt_project/models/alerts/alerts_v2.sql (1)
27-29: Confirm upstream model existence and DAG ordering
The-- depends_onrefs in alerts_v2.sql correctly declare dependencies on the two Elementary package models. Sincedbt_invocationsanddbt_run_resultslive in the externalelementarypackage, verify that your pinned package version includes them and that they appear upstream ofalerts_v2by running:dbt ls --select +elementary.alerts_v2 --resource-type model --output name | sed -n '1,50p'Ensure both
elementary.dbt_invocationsandelementary.dbt_run_resultsare listed beforealerts_v2.elementary/messages/blocks.py (1)
23-23: LINK icon mapping verified across all outputs
Icon.LINKis present inICON_TO_UNICODE, and the text, markdown, adaptive_cards, and block_kit formatters all delegate to it. There is no HTML-specific formatter to update.elementary/monitor/dbt_project/macros/get_source_freshness_results.sql (1)
13-13: Alignment with new macro signature — OK.Passing 'status' explicitly matches the updated macro and keeps intent clear.
elementary/messages/formats/unicode.py (1)
18-19: LGTM — mapping added and coverage check retained.The 🔗 mapping prevents runtime errors from the coverage guard below.
elementary/monitor/dbt_project/macros/alerts/population/source_freshness_alerts.sql (3)
36-43: Good addition: job/orchestrator fields are plumbed into the payload.
Matches the new AlertModel fields; no issues spotted.
95-95: Ensure the normalized status has a stable alias.
populate_source_freshness_alertslater readsnormalized_status. If the macro doesn’t alias, add one here.Apply if needed:
- {{ elementary_cli.normalized_source_freshness_status('results.status')}}, + {{ elementary_cli.normalized_source_freshness_status('results.status') }} as normalized_status,
130-131: Confirm join cardinality on model_execution_id
TheLEFT JOIN dbt_run_results ON results.source_freshness_execution_id = dbt_run_results.model_execution_id(source_freshness_alerts.sql lines 130–131) assumes eachmodel_execution_idappears only once. If duplicates exist indbt_run_results, alerts will fan out. Verify the underlyingdbt_run_resultsmodel enforces a one-row-per-execution_id constraint or add a deduplication step.elementary/monitor/dbt_project/macros/alerts/population/test_alerts.sql (2)
42-49: Payload enriched with job/orchestrator context — looks good.
Keys align with AlertModel/orchestrator_info.
83-90: Confirm availability of invocation/run results models across environments
Thedbt_invocationsanddbt_run_resultsmodels are defined in the package underelementary/monitor/dbt_project/models(e.g.models/dbt_invocations.sqlandmodels/dbt_run_results.sql), and are referenced by macros in alerts and tests. Ensure these files are indeed present in all adapter-specific subfolders (if any) and included in every deployed environment.elementary/monitor/alerts/alert.py (1)
35-40: Constructor now carries orchestrator/job fields — OK.
Field names match downstream usage and schema.Also applies to: 74-80
elementary/monitor/dbt_project/macros/alerts/population/model_alerts.sql (3)
24-31: Model alert payload gains orchestrator/job fields — good alignment.
No structural issues.
120-125: Filter scoping fixed toall_run_results— good.
Time window and status predicates look correct.
132-132: detected_at sourced from the right alias — good fix.
Prevents timezone/alias confusion.tests/unit/alerts/test_orchestrator_integration.py (5)
67-81: LGTM: job link expectations match implementation.Text, URL, and
Icon.GEARalign withcreate_job_link.
122-141: LGTM: minimal orchestrator info handled correctly.
orchestrator_infoincludes onlyjob_name, as intended.
203-247: LGTM: edge cases are well covered.Empty strings, None filtering, and single-field scenarios match
AlertModel.orchestrator_info.
290-301: LGTM: model validation happy path.Creation with optional icon validated correctly.
313-320: LGTM: required fields validation.Exception expectations for missing fields are appropriate.
tests/unit/alerts/alert_messages/test_orchestrator_message_simple.py (3)
45-72: Relies on link text formatting — ensure upstream function formats display name.This assertion depends on "View in Dbt Cloud" text. The upstream fix to
create_orchestrator_link(title-casing) is required for this to pass consistently.
121-125: LGTM: covers GitHub Actions rendering path.Validates both job info and link presence.
165-168: LGTM: full integration exercise.End-to-end check that link URL is present in rendered output.
elementary/monitor/fetchers/alerts/schema/alert_data.py (4)
37-43: LGTM: orchestrator fields added to schema.Optional fields with clear naming; safe defaults.
238-245: LGTM: TestAlertModel receives orchestrator fields.Fields are forwarded correctly.
294-301: LGTM: ModelAlertModel receives orchestrator fields.Consistent with TestAlert and SourceFreshness alert flows.
371-377: LGTM: SourceFreshnessAlertModel receives orchestrator fields.End-to-end propagation covered.
elementary/monitor/alerts/test_alert.py (1)
51-84: AlertModel.init already defines job_id, job_name, job_run_id, job_url, job_run_url, and orchestrator; all super().init calls match this signature.elementary/monitor/alerts/alert_messages/builder.py (2)
126-147: LGTM: inline “Job:” line with link is clear and compact.Good use of
BoldTextBlock+LinkInlineBlocks.
159-176: LGTM: avoids duplicate link when job line already embeds it.Conditional merge into
LinksLineBlockis correct.elementary/monitor/alerts/model_alert.py (1)
40-46: Approve orchestrator/job context additionsAll ModelAlertModel instantiations pass job_id, job_name, job_run_id, job_url, job_run_url, and orchestrator via keyword arguments; no positional usage detected.
| def test_create_orchestrator_link_formats_orchestrator_names(self): | ||
| test_cases = [ | ||
| ("airflow", "View in Airflow"), | ||
| ("dbt_cloud", "View in Dbt Cloud"), | ||
| ("github_actions", "View in Github Actions"), | ||
| ("custom_orchestrator", "View in Custom Orchestrator"), | ||
| ] | ||
|
|
||
| for orchestrator, expected_text in test_cases: | ||
| orchestrator_info = { | ||
| "orchestrator": orchestrator, | ||
| "run_url": "https://example.com/run/123", # noqa: E231 | ||
| } | ||
|
|
||
| link = create_orchestrator_link(orchestrator_info) | ||
|
|
||
| assert link is not None | ||
| assert link.text == expected_text | ||
| assert link.orchestrator == orchestrator | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align display text with function behavior (title-casing orchestrator).
The tests expect "View in Airflow/Dbt Cloud/Github Actions", but create_orchestrator_link currently uses the raw orchestrator string. Update the function to format display names (replace _ with space and title-case) to match these expectations.
Apply this change outside this file (in elementary/monitor/data_monitoring/alerts/integrations/utils/orchestrator_link.py):
def create_orchestrator_link(
orchestrator_info: Dict[str, str]
) -> Optional[OrchestratorLinkData]:
"""Create an orchestrator link from orchestrator info if URL is available."""
if not orchestrator_info or not orchestrator_info.get("run_url"):
return None
- orchestrator = orchestrator_info.get("orchestrator", "orchestrator")
+ orchestrator = orchestrator_info.get("orchestrator", "orchestrator")
+ display_name = orchestrator.replace("_", " ").title()
return OrchestratorLinkData(
url=orchestrator_info["run_url"],
- text=f"View in {orchestrator}",
- orchestrator=orchestrator,
+ text=f"View in {display_name}",
+ orchestrator=orchestrator,
icon=Icon.LINK,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_create_orchestrator_link_formats_orchestrator_names(self): | |
| test_cases = [ | |
| ("airflow", "View in Airflow"), | |
| ("dbt_cloud", "View in Dbt Cloud"), | |
| ("github_actions", "View in Github Actions"), | |
| ("custom_orchestrator", "View in Custom Orchestrator"), | |
| ] | |
| for orchestrator, expected_text in test_cases: | |
| orchestrator_info = { | |
| "orchestrator": orchestrator, | |
| "run_url": "https://example.com/run/123", # noqa: E231 | |
| } | |
| link = create_orchestrator_link(orchestrator_info) | |
| assert link is not None | |
| assert link.text == expected_text | |
| assert link.orchestrator == orchestrator | |
| def create_orchestrator_link( | |
| orchestrator_info: Dict[str, str] | |
| ) -> Optional[OrchestratorLinkData]: | |
| """Create an orchestrator link from orchestrator info if URL is available.""" | |
| if not orchestrator_info or not orchestrator_info.get("run_url"): | |
| return None | |
| orchestrator = orchestrator_info.get("orchestrator", "orchestrator") | |
| display_name = orchestrator.replace("_", " ").title() | |
| return OrchestratorLinkData( | |
| url=orchestrator_info["run_url"], | |
| text=f"View in {display_name}", | |
| orchestrator=orchestrator, | |
| icon=Icon.LINK, | |
| ) |
🤖 Prompt for AI Agents
In tests/unit/alerts/test_orchestrator_integration.py around lines 33 to 52 the
tests expect orchestrator display names like "View in Airflow" (title-cased,
with underscores replaced by spaces); update the implementation in
elementary/monitor/data_monitoring/alerts/integrations/utils/orchestrator_link.py
to format the orchestrator name by replacing "_" with " " and applying
title-case before composing the link text (e.g., text = f"View in
{formatted_orchestrator}"), while preserving the original orchestrator value in
the link.orchestrator and keeping run_url handling unchanged.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/unit/alerts/test_orchestrator_integration.py (1)
27-31: Resolved: icon expectation matches implementation (LINK).The earlier comment about expecting INFO is now fixed; asserting Icon.LINK is correct.
🧹 Nitpick comments (2)
tests/unit/alerts/test_orchestrator_integration.py (2)
17-31: Lock in display text expectations for create_orchestrator_link (casing).Current implementation uses raw orchestrator in text ("View in airflow"), while job links title-case ("Airflow"). Decide on consistent UX and assert it in tests.
- If adopting title-case for run links too, update implementation (outside this file) and add this assertion:
link = create_orchestrator_link(orchestrator_info) assert link is not None assert link.url == "https://airflow.example.com/run/12345" assert link.orchestrator == "airflow" assert link.icon == Icon.LINK + assert link.text == "View in Airflow"
- Otherwise, if keeping raw casing, add:
+ assert link.text == "View in airflow"
46-60: Broaden coverage via parametrization for multiple orchestrators.Parametrize this test over ["airflow", "dbt_cloud", "github_actions"] and assert the display text formatting (underscores → spaces, title-case) and icon GEAR consistently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
elementary/monitor/alerts/alert.py(3 hunks)tests/unit/alerts/alert_messages/test_orchestrator_message_simple.py(1 hunks)tests/unit/alerts/test_orchestrator_integration.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- elementary/monitor/alerts/alert.py
- tests/unit/alerts/alert_messages/test_orchestrator_message_simple.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/alerts/test_orchestrator_integration.py (4)
elementary/messages/blocks.py (1)
Icon(8-23)elementary/monitor/alerts/test_alert.py (1)
TestAlertModel(13-201)elementary/monitor/data_monitoring/alerts/integrations/utils/orchestrator_link.py (3)
OrchestratorLinkData(7-11)create_job_link(31-49)create_orchestrator_link(14-28)elementary/monitor/alerts/alert.py (1)
orchestrator_info(101-128)
🔇 Additional comments (2)
tests/unit/alerts/test_orchestrator_integration.py (2)
283-288: Syntax error: missing comma after keyword arg.Add a trailing comma after orchestrator to make the call valid.
- orchestrator="airflow" + orchestrator="airflow",Likely an incorrect or invalid review comment.
112-114: Syntax error: missing comma after keyword arg.Add a trailing comma after job_name to make the call valid.
- job_name="test_job" + job_name="test_job",Likely an incorrect or invalid review comment.
| import pytest | ||
|
|
||
| from elementary.messages.blocks import Icon | ||
| from elementary.monitor.alerts.model_alert import ModelAlertModel |
There was a problem hiding this comment.
Import ValidationError for pydantic model assertions.
Tests rely on pydantic’s ValidationError; import it explicitly.
import pytest
+
+from pydantic import ValidationError📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import pytest | |
| from elementary.messages.blocks import Icon | |
| from elementary.monitor.alerts.model_alert import ModelAlertModel | |
| import pytest | |
| from pydantic import ValidationError | |
| from elementary.messages.blocks import Icon | |
| from elementary.monitor.alerts.model_alert import ModelAlertModel |
🤖 Prompt for AI Agents
In tests/unit/alerts/test_orchestrator_integration.py around lines 1 to 4, the
tests assert pydantic model validation failures but do not import
ValidationError; add "from pydantic import ValidationError" to the imports at
the top of the file so tests can reference ValidationError in their assertions.
| def test_model_alert_orchestrator_info(self): | ||
| alert = ModelAlertModel( | ||
| id="model_alert_id", | ||
| alias="test_model", | ||
| path="/models/test_model.sql", | ||
| original_path="models/test_model.sql", | ||
| materialization="table", | ||
| full_refresh=False, | ||
| alert_class_id="model_alert_class", | ||
| job_name="nightly_build", | ||
| job_run_id="67890", | ||
| orchestrator="dbt_cloud", | ||
| job_run_url="https://cloud.getdbt.com/run/67890", | ||
| ) | ||
|
|
||
| info = alert.orchestrator_info | ||
| assert info is not None | ||
| assert info["job_name"] == "nightly_build" | ||
| assert info["run_id"] == "67890" | ||
| assert info["orchestrator"] == "dbt_cloud" | ||
| assert info["run_url"] == "https://cloud.getdbt.com/run/67890" | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Edge case not covered: only job_id present returns None due to gating.
AlertModel.orchestrator_info currently gates on job_name, job_run_id, orchestrator, job_url, job_run_url but not job_id. If only job_id is set, info incorrectly returns None. Consider including job_id in the gate or compute info first and return None if empty. I can open a PR to adjust base logic and add a unit test here.
Proposed fix outside this file (elementary/monitor/alerts/alert.py):
- if not any(
- [
- self.job_name,
- self.job_run_id,
- self.orchestrator,
- self.job_url,
- self.job_run_url,
- ]
- ):
+ if not any(
+ [
+ self.job_id,
+ self.job_name,
+ self.job_run_id,
+ self.orchestrator,
+ self.job_url,
+ self.job_run_url,
+ ]
+ ):
return None🏁 Script executed:
#!/usr/bin/env bash
set -e
# Locate the orchestrator_info method
rg -n "def orchestrator_info" -t py -C5
# Locate the ModelAlertModel class
rg -n "class ModelAlertModel" -t py -C3Length of output: 1278
🏁 Script executed:
#!/usr/bin/env bash
set -e
# Display orchestrator_info implementation
sed -n '90,150p' elementary/monitor/alerts/alert.pyLength of output: 1151
Add job_id to orchestrator_info gate and cover it with a test
- In
AlertModel.orchestrator_info, includeself.job_idin the leadingany([...])check so that when onlyjob_idis set, the method returns{"job_id": …}instead ofNone. - Add a unit test in
tests/unit/alerts/test_orchestrator_integration.pyverifying that an alert with onlyjob_idyieldsorchestrator_info == {"job_id": "<value>"}.
🤖 Prompt for AI Agents
In tests/unit/alerts/test_orchestrator_integration.py around lines 138-159, the
review requests adding job_id to the orchestrator_info gate and covering it with
a unit test; update the AlertModel.orchestrator_info implementation to include
self.job_id in the leading any([...]) check so that when only job_id is present
the method returns {"job_id": self.job_id}; then add a new unit test in this
file that constructs a ModelAlertModel with only job_id set and asserts
alert.orchestrator_info == {"job_id": "<value>"}.
|
@quickcoffee can you enable "Allow edits by maintainers" on the PR? |
@arbiv done, though was already ticked as far as I can see 🤔 |
arbiv
had a problem deploying
to
elementary_test_env
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
elementary/monitor/alerts/alert_messages/builder.py (1)
123-179: Orchestrator link rendering logic is correct and handles edge cases well.The implementation correctly handles multiple scenarios:
- Inline job link when
job_nameandrun_urlare present- Fallback to summary text when
job_nameexists butrun_urlis missing- Separate links section when only
run_urlis available (nojob_name)- Graceful handling when neither is present
The logic is sound and mutually exclusive branches prevent duplication.
Optional refactor: Extract job line construction for clarity.
The nested conditionals (lines 127-150) could be extracted into a helper method to improve readability. For example:
def _create_job_line(self, orchestrator_info: Dict[str, str]) -> Optional[LineBlock]: """Create inline job line with orchestrator link if available.""" orchestrator_name = orchestrator_info.get("orchestrator", "orchestrator") job_info_text = f"{orchestrator_info['job_name']} (via {orchestrator_name})" orchestrator_link = create_orchestrator_link(orchestrator_info) if not orchestrator_link: return None job_inlines: list[InlineBlock] = [ BoldTextBlock(text="Job:"), TextBlock(text=job_info_text + " | "), ] job_inlines.extend( LinkInlineBlocks( text=orchestrator_link.text, url=orchestrator_link.url, icon=orchestrator_link.icon, ) ) return LineBlock(inlines=job_inlines)Then simplify the main logic:
if orchestrator_info and orchestrator_info.get("job_name"): job_line = self._create_job_line(orchestrator_info) if job_line: subtitle_lines.append(job_line) else: job_info_text = f"{orchestrator_info['job_name']} (via {orchestrator_info.get('orchestrator', 'orchestrator')})" summary.append(("Job:", job_info_text))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
elementary/monitor/alerts/alert_messages/builder.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
elementary/monitor/alerts/alert_messages/builder.py (4)
elementary/messages/block_builders.py (4)
BoldTextBlock(63-64)LinkInlineBlocks(85-92)SummaryLineBlock(109-122)LinksLineBlock(99-106)elementary/monitor/data_monitoring/alerts/integrations/utils/orchestrator_link.py (1)
create_orchestrator_link(14-28)elementary/monitor/alerts/alert.py (2)
orchestrator_info(101-128)summary(94-95)elementary/messages/blocks.py (2)
TextBlock(40-43)LineBlock(67-70)
🔇 Additional comments (3)
elementary/monitor/alerts/alert_messages/builder.py (3)
7-7: LGTM! Import additions support orchestrator link rendering.The new imports (
BoldTextBlock,LinkInlineBlocks, andcreate_orchestrator_link) are all used in the subtitle rendering logic and come from appropriate modules.Also applies to: 13-13, 47-49
114-114: LGTM! Parameter addition is backward compatible.The
orchestrator_infoparameter is properly typed, has a sensible default, and maintains backward compatibility with existing callers.
206-206: LGTM! Clean orchestrator info propagation.The orchestrator info is correctly retrieved from the alert's property (line 206) and cleanly passed through to the subtitle rendering (line 216). The types align properly throughout the call chain.
Also applies to: 216-216
arbiv
had a problem deploying
to
elementary_test_env
GitHub Actions
Failure
| if detected_at_str: | ||
| summary.append(("Time:", detected_at_str)) | ||
|
|
||
| # Initialize subtitle lines with summary |
There was a problem hiding this comment.
please remove all of the inline comments you added, as it goes agains the conventions of the project (only complicated/unintuitive code should be commented)
Relevant to the entire PR
There was a problem hiding this comment.
I think I removed all of them now :)
elementary/monitor/alerts/alert.py
Outdated
| raise NotImplementedError | ||
|
|
||
| @property | ||
| def orchestrator_info(self) -> Optional[Dict[str, str]]: |
There was a problem hiding this comment.
dont return a dict to represent structured data, create a dedicated OrchestratorInfo class for it
There was a problem hiding this comment.
Added OrchestratorInfo instead
|
Hey @quickcoffee sorry for the delay on the review, it slipped a bit through the cracks Overall looks awesome 🤘 |
quickcoffee
had a problem deploying
to
elementary_test_env
GitHub Actions
Error
|
@ofek1weiss thanks for the review! I made the changes you suggested :) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
elementary/monitor/data_monitoring/alerts/integrations/utils/orchestrator_link.py (1)
32-49: Consider extracting the display_name transformation to reduce duplication.Line 42 duplicates logic from
elementary/monitor/alerts/test_alert.py(lines 126-127), where adisplay_namefunction performs the same transformation. Consider extracting this to a shared utility module to maintain DRY principles.♻️ Suggested refactor
Create a shared utility function (e.g., in
elementary/utils/strings.py):def format_display_name(value: str) -> str: """Convert snake_case to Title Case for display.""" return value.replace("_", " ").title()Then import and use it here:
+from elementary.utils.strings import format_display_name + def create_job_link( orchestrator_info: OrchestratorInfo, ) -> Optional[OrchestratorLinkData]: """Create a job-level orchestrator link if job URL is available.""" if not orchestrator_info or not orchestrator_info.job_url: return None orchestrator = orchestrator_info.orchestrator or "orchestrator" job_name = orchestrator_info.job_name or "Job" - display_name = orchestrator.replace("_", " ").title() + display_name = format_display_name(orchestrator) return OrchestratorLinkData( url=orchestrator_info.job_url, text=f"{job_name} in {display_name}", orchestrator=orchestrator, icon=Icon.GEAR, )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
elementary/monitor/alerts/alert.pyelementary/monitor/alerts/alert_messages/builder.pyelementary/monitor/data_monitoring/alerts/integrations/utils/orchestrator_link.pytests/unit/alerts/alert_messages/test_orchestrator_message_simple.pytests/unit/alerts/test_orchestrator_integration.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/unit/alerts/alert_messages/test_orchestrator_message_simple.py (4)
elementary/messages/formats/block_kit.py (1)
format_block_kit(360-364)elementary/monitor/alerts/model_alert.py (2)
ModelAlertModel(14-116)get_report_link(115-116)tests/unit/alerts/alert_messages/test_alert_utils.py (1)
get_alert_message_body(305-316)elementary/monitor/alerts/alert.py (2)
get_report_link(109-110)orchestrator_info(113-133)
tests/unit/alerts/test_orchestrator_integration.py (2)
elementary/monitor/alerts/alert.py (2)
OrchestratorInfo(16-24)orchestrator_info(113-133)elementary/monitor/data_monitoring/alerts/integrations/utils/orchestrator_link.py (3)
OrchestratorLinkData(8-12)create_job_link(32-49)create_orchestrator_link(15-29)
elementary/monitor/data_monitoring/alerts/integrations/utils/orchestrator_link.py (3)
elementary/messages/blocks.py (1)
Icon(8-23)elementary/monitor/alerts/alert.py (2)
OrchestratorInfo(16-24)orchestrator_info(113-133)elementary/monitor/alerts/test_alert.py (1)
display_name(127-128)
🔇 Additional comments (5)
elementary/monitor/alerts/alert.py (2)
16-24: LGTM! Clean data model for orchestrator metadata.The
OrchestratorInfomodel is well-structured with all fields appropriately optional, enabling flexible orchestrator integration across different alert types.
112-133: LGTM! Orchestrator info property logic is correct.The gating logic correctly returns
Nonewhen no meaningful orchestrator data is present. The exclusion ofjob_idfrom the gating check (lines 115-123) is appropriate, as job_id alone is insufficient context. Theor Nonenormalization (line 127) properly handles empty strings.tests/unit/alerts/alert_messages/test_orchestrator_message_simple.py (1)
1-165: LGTM! Comprehensive test coverage for orchestrator message rendering.The tests effectively cover:
- Orchestrator data inclusion in messages
- Link rendering with URLs
- Backward compatibility (alerts without orchestrator data)
- Multiple alert types (TestAlertModel, ModelAlertModel)
- Property validation and integration
The use of
str()conversion for text search (line 39) is pragmatic for integration-style testing.tests/unit/alerts/test_orchestrator_integration.py (1)
1-302: LGTM! Excellent comprehensive test coverage.The test suite thoroughly validates:
- Link creation with various data scenarios (valid, missing URL, empty info)
orchestrator_infoproperty behavior across all alert model types- Edge cases including empty strings, None values, and partial data
- Model validation for required fields
The structured approach with dedicated test classes makes the coverage easy to verify and maintain.
elementary/monitor/alerts/alert_messages/builder.py (1)
106-172: LGTM! The orchestrator rendering logic is well-designed.The conditional rendering strategy is sound:
- When
job_nameis present: renders job info inline with optional orchestrator link- When
job_nameis absent: adds orchestrator link to the links sectionThe fallback values (line 128:
orchestrator_name = orchestrator_info.orchestrator or "orchestrator") ensure graceful degradation, and the link aggregation logic (lines 153-170) properly handles both existing and new orchestrator links.
ofek1weiss
had a problem deploying
to
elementary_test_env
GitHub Actions
Failure
|
@quickcoffee approved your changes, but looks like some of the tests are failing If you can, run the tests with the following env variable: I can help with doing that if this does not work for you 👍 |
@ofek1weiss could you point me into the right direction for this? I tried |
quickcoffee
requested a deployment
to
elementary_test_env
GitHub Actions
Waiting
3 participants
This is a first attempt for #1944.
I tested it locally with a failed model and a failed test:
I'm not quite sure if the tests that Claude Code created make that much sense - looking forward to some feedback here.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.