Add orchestrator jobs data to alerts #1997
Conversation
quickcoffee
had a problem deploying
to
elementary_test_env
GitHub Actions
Error
WalkthroughAdds optional job/orchestrator fields to alert payloads, exposes them via AlertModel.orchestrator_info, adds Icon.LINK and its Unicode, introduces 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 (dict 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 Files/areas to pay extra attention to:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
| @property | ||
| def orchestrator_info(self) -> Optional[Dict[str, str]]: | ||
| """Returns structured orchestrator metadata if available.""" | ||
| if not any([self.job_name, self.job_run_id, self.orchestrator]): | ||
| return None | ||
|
|
||
| info = {} | ||
| if self.job_name: | ||
| info["job_name"] = self.job_name | ||
| if self.job_run_id: | ||
| info["run_id"] = self.job_run_id | ||
| if self.orchestrator: | ||
| info["orchestrator"] = self.orchestrator | ||
| if self.job_url: | ||
| info["job_url"] = self.job_url | ||
| if self.job_run_url: | ||
| info["run_url"] = self.job_run_url | ||
|
|
||
| return info |
There was a problem hiding this comment.
orchestrator_info incorrectly returns None when only URLs are present.
This blocks link rendering when run_url/job_url exist without name/run_id/orchestrator.
Apply:
@property
def orchestrator_info(self) -> Optional[Dict[str, str]]:
"""Returns structured orchestrator metadata if available."""
- if not any([self.job_name, self.job_run_id, self.orchestrator]):
+ if not any([self.job_name, self.job_run_id, self.orchestrator, self.job_url, self.job_run_url]):
return None
info = {}
+ if self.job_id:
+ info["job_id"] = self.job_id
if self.job_name:
info["job_name"] = self.job_name
if self.job_run_id:
info["run_id"] = self.job_run_id
if self.orchestrator:
info["orchestrator"] = self.orchestrator
if self.job_url:
info["job_url"] = self.job_url
if self.job_run_url:
info["run_url"] = self.job_run_url📝 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.
| @property | |
| def orchestrator_info(self) -> Optional[Dict[str, str]]: | |
| """Returns structured orchestrator metadata if available.""" | |
| if not any([self.job_name, self.job_run_id, self.orchestrator]): | |
| return None | |
| info = {} | |
| if self.job_name: | |
| info["job_name"] = self.job_name | |
| if self.job_run_id: | |
| info["run_id"] = self.job_run_id | |
| if self.orchestrator: | |
| info["orchestrator"] = self.orchestrator | |
| if self.job_url: | |
| info["job_url"] = self.job_url | |
| if self.job_run_url: | |
| info["run_url"] = self.job_run_url | |
| return info | |
| @property | |
| def orchestrator_info(self) -> Optional[Dict[str, str]]: | |
| """Returns structured orchestrator metadata if available.""" | |
| if not any([self.job_name, self.job_run_id, self.orchestrator, self.job_url, self.job_run_url]): | |
| return None | |
| info = {} | |
| if self.job_id: | |
| info["job_id"] = self.job_id | |
| if self.job_name: | |
| info["job_name"] = self.job_name | |
| if self.job_run_id: | |
| info["run_id"] = self.job_run_id | |
| if self.orchestrator: | |
| info["orchestrator"] = self.orchestrator | |
| if self.job_url: | |
| info["job_url"] = self.job_url | |
| if self.job_run_url: | |
| info["run_url"] = self.job_run_url | |
| return info |
🤖 Prompt for AI Agents
In elementary/monitor/alerts/alert.py around lines 100 to 118, the
orchestrator_info property returns None when only job_url and/or job_run_url are
set because the initial any(...) check only tests job_name, job_run_id, and
orchestrator; change that guard to include job_url and job_run_url so the method
builds and returns a dict whenever any of the five fields are present, leaving
the rest of the logic that conditionally adds keys intact.
| assert link is not None | ||
| assert link.text == "View in Airflow" | ||
| assert link.url == "https://airflow.example.com/run/12345" | ||
| assert link.orchestrator == "airflow" | ||
| assert link.icon == Icon.INFO | ||
|
|
There was a problem hiding this comment.
Fix failing expectations: icon should be LINK, not INFO.
create_orchestrator_link returns Icon.LINK (per implementation), but the test expects Icon.INFO.
Apply this diff:
- assert link.icon == Icon.INFO
+ assert link.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.
| assert link is not None | |
| assert link.text == "View in Airflow" | |
| assert link.url == "https://airflow.example.com/run/12345" | |
| assert link.orchestrator == "airflow" | |
| assert link.icon == Icon.INFO | |
| assert link is not None | |
| assert link.text == "View in Airflow" | |
| assert link.url == "https://airflow.example.com/run/12345" | |
| assert link.orchestrator == "airflow" | |
| assert link.icon == Icon.LINK |
🤖 Prompt for AI Agents
In tests/unit/alerts/test_orchestrator_integration.py around lines 27 to 32, the
test asserts the wrong icon constant (Icon.INFO) while the implementation
returns Icon.LINK; update the assertion to expect Icon.LINK by replacing the
Icon.INFO check with Icon.LINK so the test matches create_orchestrator_link's
behavior.
| 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>"}.
| # Should raise validation error if required fields are missing | ||
| with pytest.raises((TypeError, ValueError)): | ||
| OrchestratorLinkData( | ||
| # Missing required fields | ||
| icon=Icon.LINK | ||
| ) |
There was a problem hiding this comment.
Catch the correct exception type from pydantic.
OrchestratorLinkData (pydantic BaseModel) raises ValidationError on missing required fields, not TypeError/ValueError.
- with pytest.raises((TypeError, ValueError)):
+ with pytest.raises(ValidationError):
OrchestratorLinkData(
# Missing required fields
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.
| # Should raise validation error if required fields are missing | |
| with pytest.raises((TypeError, ValueError)): | |
| OrchestratorLinkData( | |
| # Missing required fields | |
| icon=Icon.LINK | |
| ) | |
| # Should raise validation error if required fields are missing | |
| with pytest.raises(ValidationError): | |
| OrchestratorLinkData( | |
| # Missing required fields | |
| icon=Icon.LINK | |
| ) |
🤖 Prompt for AI Agents
In tests/unit/alerts/test_orchestrator_integration.py around lines 293 to 298,
the test is catching TypeError/ValueError but pydantic BaseModel raises
pydantic.error_wrappers.ValidationError for missing required fields; update the
pytest.raises to expect ValidationError and add an import for ValidationError
from pydantic at the top of the test file (or import it in the test scope) so
the test asserts the correct exception type.
|
@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
| orchestrator_link = create_orchestrator_link(orchestrator_info) | ||
| if orchestrator_link: | ||
| # Create inline blocks for job info + link | ||
| job_inlines: List[InlineBlock] = [ |
There was a problem hiding this comment.
Minor: Use consistent type annotation style.
Line 135 uses List[InlineBlock] (uppercase) while line 113 uses list[ReportLinkData] (lowercase). For consistency and modern Python convention (3.9+), prefer lowercase list throughout.
Apply this diff:
- job_inlines: List[InlineBlock] = [
+ job_inlines: list[InlineBlock] = [📝 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.
| job_inlines: List[InlineBlock] = [ | |
| job_inlines: list[InlineBlock] = [ |
🤖 Prompt for AI Agents
In elementary/monitor/alerts/alert_messages/builder.py around line 135, the type
annotation uses the legacy uppercase typing (List[InlineBlock]) which is
inconsistent with the modern lowercase style used elsewhere (e.g., line 113 uses
list[ReportLinkData]); change List[InlineBlock] to list[InlineBlock] and remove
any unnecessary import of List from typing if it becomes unused to keep
annotations consistent with Python 3.9+ conventions.
2 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