-
Notifications
You must be signed in to change notification settings - Fork 220
feat: add --slack-full-width for full-width Slack alerts with markdown test result tables #2107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 9 commits
9b23738
fa34bca
153f530
117f19f
7d2a4ff
a611b9f
5ec85bb
3e54e29
5446b6b
1eb6672
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -26,16 +26,36 @@ class SlackAlertMessageSchema(BaseModel): | |||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| class SlackAlertMessageBuilder(SlackMessageBuilder): | ||||||||||||||||||||||||||||||
| def __init__(self) -> None: | ||||||||||||||||||||||||||||||
| def __init__(self, full_width: bool = False) -> None: | ||||||||||||||||||||||||||||||
| super().__init__() | ||||||||||||||||||||||||||||||
| self.full_width = full_width | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def get_slack_message( | ||||||||||||||||||||||||||||||
| self, | ||||||||||||||||||||||||||||||
| alert_schema: SlackAlertMessageSchema, | ||||||||||||||||||||||||||||||
| ) -> SlackMessageSchema: | ||||||||||||||||||||||||||||||
| if self.full_width: | ||||||||||||||||||||||||||||||
| # A rich_text block at the start forces Slack to use full message width | ||||||||||||||||||||||||||||||
| # for following blocks instead of the narrower attachment-style layout. | ||||||||||||||||||||||||||||||
| # The elements array must be non-empty per Slack Block Kit API. | ||||||||||||||||||||||||||||||
| self._add_always_displayed_blocks( | ||||||||||||||||||||||||||||||
| [ | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| "type": "rich_text", | ||||||||||||||||||||||||||||||
| "elements": [ | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| "type": "rich_text_section", | ||||||||||||||||||||||||||||||
| "elements": [{"type": "text", "text": " "}], | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| self.add_title_to_slack_alert(alert_schema.title) | ||||||||||||||||||||||||||||||
| self.add_preview_to_slack_alert(alert_schema.preview) | ||||||||||||||||||||||||||||||
| self.add_details_to_slack_alert(alert_schema.details) | ||||||||||||||||||||||||||||||
| if self.full_width: | ||||||||||||||||||||||||||||||
| self.slack_message["attachments"] = [] | ||||||||||||||||||||||||||||||
| return super().get_slack_message() | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def add_title_to_slack_alert(self, title_blocks: Optional[SlackBlocksType] = None): | ||||||||||||||||||||||||||||||
|
|
@@ -46,15 +66,23 @@ def add_title_to_slack_alert(self, title_blocks: Optional[SlackBlocksType] = Non | |||||||||||||||||||||||||||||
| def add_preview_to_slack_alert( | ||||||||||||||||||||||||||||||
| self, preview_blocks: Optional[SlackBlocksType] = None | ||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||
| if preview_blocks: | ||||||||||||||||||||||||||||||
| validated_preview_blocks = self._validate_preview_blocks(preview_blocks) | ||||||||||||||||||||||||||||||
| if not preview_blocks: | ||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||
| validated_preview_blocks = self._validate_preview_blocks(preview_blocks) | ||||||||||||||||||||||||||||||
| if self.full_width: | ||||||||||||||||||||||||||||||
| self._add_always_displayed_blocks(validated_preview_blocks) | ||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||
| self._add_blocks_as_attachments(validated_preview_blocks) | ||||||||||||||||||||||||||||||
|
Comment on lines
+69
to
75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Skip attachment-era preview validation in Line 71 still runs Suggested fix if not preview_blocks:
return
- validated_preview_blocks = self._validate_preview_blocks(preview_blocks)
if self.full_width:
- self._add_always_displayed_blocks(validated_preview_blocks)
+ self._add_always_displayed_blocks(preview_blocks)
else:
+ validated_preview_blocks = self._validate_preview_blocks(preview_blocks)
self._add_blocks_as_attachments(validated_preview_blocks)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def add_details_to_slack_alert( | ||||||||||||||||||||||||||||||
| self, | ||||||||||||||||||||||||||||||
| detail_blocks: Optional[SlackBlocksType] = None, | ||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||
| if detail_blocks: | ||||||||||||||||||||||||||||||
| if not detail_blocks: | ||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||
| if self.full_width: | ||||||||||||||||||||||||||||||
| self._add_always_displayed_blocks(detail_blocks) | ||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||
| self._add_blocks_as_attachments(detail_blocks) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,8 @@ | ||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||
| import math | ||||||||||||||||||||||||||
| from typing import Any, List, Optional, Union | ||||||||||||||||||||||||||
| from typing import Any, Dict, List, Optional, Union | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| from tabulate import tabulate | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| def try_load_json(value: Optional[Union[str, dict, list]]): | ||||||||||||||||||||||||||
|
|
@@ -94,3 +96,70 @@ def inf_and_nan_to_str(obj) -> Any: | |||||||||||||||||||||||||
| return [inf_and_nan_to_str(i) for i in obj] | ||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||
| return obj | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| def _format_value(value: Any) -> str: | ||||||||||||||||||||||||||
| """Format a value for table display, avoiding scientific notation for floats.""" | ||||||||||||||||||||||||||
| if value is None: | ||||||||||||||||||||||||||
| return "" | ||||||||||||||||||||||||||
| if isinstance(value, float): | ||||||||||||||||||||||||||
| if math.isinf(value) or math.isnan(value): | ||||||||||||||||||||||||||
| return str(value) | ||||||||||||||||||||||||||
| # Format floats without scientific notation | ||||||||||||||||||||||||||
| if value == int(value) and abs(value) < 1e15: | ||||||||||||||||||||||||||
| return str(int(value)) | ||||||||||||||||||||||||||
| return f"{value:.10f}".rstrip("0").rstrip(".") | ||||||||||||||||||||||||||
|
Comment on lines
+105
to
+111
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
python - <<'PY'
for value in [1e-11, 1e-12, -1e-12]:
rendered = f"{value:.10f}".rstrip("0").rstrip(".")
print(f"{value!r} -> {rendered!r}")
PYRepository: elementary-data/elementary Length of output: 110 🏁 Script executed: # First, check if the file exists and inspect the relevant lines
find . -name "json_utils.py" -type fRepository: elementary-data/elementary Length of output: 102 🏁 Script executed: # Check the file size to understand scope
wc -l elementary/utils/json_utils.py 2>/dev/null || echo "File not found in expected location"Repository: elementary-data/elementary Length of output: 104 🏁 Script executed: # Search for the _format_value function to understand context
rg "_format_value" --type py -B 3 -A 15Repository: elementary-data/elementary Length of output: 2498 🏁 Script executed: # Search for uses of list_of_dicts_to_markdown_table to understand impact
rg "list_of_dicts_to_markdown_table|_format_value" --type pyRepository: elementary-data/elementary Length of output: 2001 🏁 Script executed: # Search for test files related to json_utils
fd -e "test*.py" -o -name "*test.py" | xargs grep -l "json_utils\|_format_value\|markdown_table" 2>/dev/null || echo "No matching tests found"Repository: elementary-data/elementary Length of output: 341 🏁 Script executed: # Check if there are any references to this in Slack alert or table rendering code
rg "Slack|alert|markdown|table" elementary/utils/json_utils.py -A 2 -B 2Repository: elementary-data/elementary Length of output: 1823 🏁 Script executed: # Look at the actual test file to see what float cases are tested
cat -n tests/unit/utils/test_json_utils.pyRepository: elementary-data/elementary Length of output: 2574 🏁 Script executed: # Run the existing tests to see if they pass
grep -A 20 "test_list_of_dicts_to_markdown_table_float" tests/unit/utils/test_json_utils.pyRepository: elementary-data/elementary Length of output: 964 🏁 Script executed: # Verify the impact by checking where list_of_dicts_to_markdown_table is used in the Slack integration
rg "test_rows_sample_table" elementary/monitor/data_monitoring/alerts/integrations/slack/slack.py -B 5 -A 5Repository: elementary-data/elementary Length of output: 857 🏁 Script executed: # Check if there are any recent fixes or discussions about small float handling
rg "1e-1[0-2]|scientific|notation" elementary/utils/json_utils.pyRepository: elementary-data/elementary Length of output: 206 Small non-zero floats collapse to Line 111 uses The existing test suite does not cover this edge case. Add a regression test for small float values (e.g., Possible fix direction+from decimal import Decimal
...
if isinstance(value, float):
if math.isinf(value) or math.isnan(value):
return str(value)
- # Format floats without scientific notation
- if value == int(value) and abs(value) < 1e15:
- return str(int(value))
- return f"{value:.10f}".rstrip("0").rstrip(".")
+ normalized = format(Decimal(str(value)), "f").rstrip("0").rstrip(".")
+ return "0" if normalized in {"", "-0"} else normalized📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| return str(value) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| def list_of_dicts_to_markdown_table( | ||||||||||||||||||||||||||
| data: List[Dict[str, Any]], max_length: Optional[int] = None | ||||||||||||||||||||||||||
| ) -> str: | ||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||
| Convert a list of dictionaries with consistent keys to a markdown table string. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||
| data: List of dictionaries | ||||||||||||||||||||||||||
| max_length: Optional maximum character length for the output. If the full | ||||||||||||||||||||||||||
| table exceeds this limit, rows are removed from the end and a | ||||||||||||||||||||||||||
| "(truncated)" note is appended to avoid cutting mid-row. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||
| A markdown-formatted table string using GitHub table format | ||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||
| if not data: | ||||||||||||||||||||||||||
| return "" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| processed_data = [{k: _format_value(v) for k, v in row.items()} for row in data] | ||||||||||||||||||||||||||
| full_table = tabulate( | ||||||||||||||||||||||||||
| processed_data, headers="keys", tablefmt="github", disable_numparse=True | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if max_length is None or len(full_table) <= max_length: | ||||||||||||||||||||||||||
| return full_table | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if max_length <= 0: | ||||||||||||||||||||||||||
| return "" | ||||||||||||||||||||||||||
| truncation_note = "\n(truncated)" | ||||||||||||||||||||||||||
| if max_length <= len(truncation_note): | ||||||||||||||||||||||||||
| return "(truncated)"[:max_length] | ||||||||||||||||||||||||||
| effective_max = max_length - len(truncation_note) | ||||||||||||||||||||||||||
| for row_count in range(len(processed_data) - 1, 0, -1): | ||||||||||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||||||||||||||||||||||
| table = tabulate( | ||||||||||||||||||||||||||
| processed_data[:row_count], | ||||||||||||||||||||||||||
| headers="keys", | ||||||||||||||||||||||||||
| tablefmt="github", | ||||||||||||||||||||||||||
| disable_numparse=True, | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| if len(table) <= effective_max: | ||||||||||||||||||||||||||
| return table + truncation_note | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| single_row_table = tabulate( | ||||||||||||||||||||||||||
| processed_data[:1], | ||||||||||||||||||||||||||
| headers="keys", | ||||||||||||||||||||||||||
| tablefmt="github", | ||||||||||||||||||||||||||
| disable_numparse=True, | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| if len(single_row_table) <= effective_max: | ||||||||||||||||||||||||||
| return single_row_table + truncation_note | ||||||||||||||||||||||||||
| return single_row_table[:effective_max].rstrip() + truncation_note | ||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.