From bdd71c600096273512f2185ee8843721cfb15ece Mon Sep 17 00:00:00 2001 From: alonelish Date: Wed, 15 Jul 2026 14:46:26 +0300 Subject: [PATCH 1/8] ROB-3946 Truncate over-wide summary table cells instead of wrapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Slack "Alerts Summary" digest rendered a corrupted ASCII table: long values in text columns (e.g. Java class names in `label:site`) were passed to tabulate via `maxcolwidths`, which *wraps* cells onto extra physical lines rather than truncating them, mangling every following row. TableBlock.to_table_string now truncates over-wide cells itself (with a single-char "…" ellipsis) so each row stays on one line, and no longer passes maxcolwidths to tabulate. Dotted, space-free qualified names are trimmed from the left to keep the distinctive class-name suffix (e.g. `…settler.AbstractBetSettler`); other text is trimmed from the right. __calc_max_width now water-fills the reduction across the widest text columns (fair distribution, with a minimum width floor) and never shrinks numeric columns, so the Fired/Resolved counters are always shown in full. Adds regression tests proving rows stay single-line and are truncated (not wrapped). Co-Authored-By: Claude Opus 4.8 --- src/robusta/core/reporting/blocks.py | 97 +++++++++++++++++++++++----- tests/test_blocks.py | 82 +++++++++++++++++++++++ 2 files changed, 163 insertions(+), 16 deletions(-) diff --git a/src/robusta/core/reporting/blocks.py b/src/robusta/core/reporting/blocks.py index 9e6c7cf6a..3de9784e5 100644 --- a/src/robusta/core/reporting/blocks.py +++ b/src/robusta/core/reporting/blocks.py @@ -38,6 +38,13 @@ def tabulate(*args, **kwargs): BLOCK_SIZE_LIMIT = 2997 # due to slack block size limit of 3000 +# A single-character ellipsis is used (rather than "...") for table-cell truncation +# so the width accounting stays exact - it consumes exactly one column of the cell. +TABLE_TRUNCATION_ELLIPSIS = "…" +# Never shrink a text column below this many characters, otherwise the value +# becomes unreadable (essentially just the ellipsis). +TABLE_MIN_COLUMN_WIDTH = 4 + class MarkdownBlock(BaseBlock): """ @@ -364,28 +371,78 @@ def __init__( if table_format: self.metadata["format"] = TableBlockFormat.vertical.value + @classmethod + def __is_number(cls, value) -> bool: + try: + float(str(value)) + return True + except (TypeError, ValueError): + return False + + @classmethod + def __numeric_column_indices(cls, rendered_rows, num_columns: int) -> set: + # A column is considered numeric when every non-empty cell parses as a + # number (e.g. the "Fired"/"Resolved" counters). These are kept at full + # width so the counts always stay readable. + numeric_indices = set() + for idx in range(num_columns): + values = [str(row[idx]) for row in rendered_rows if idx < len(row)] + non_empty = [v for v in values if v.strip() != ""] + if non_empty and all(cls.__is_number(v) for v in non_empty): + numeric_indices.add(idx) + return numeric_indices + @classmethod def __calc_max_width(cls, headers, rendered_rows, table_max_width: int) -> List[int]: - # We need to make sure the total table width, doesn't exceed the max width, - # otherwise, the table is printed corrupted - columns_max_widths = [len(header) for header in headers] + # We need to make sure the total table width doesn't exceed the max width, + # otherwise the table is printed corrupted. + num_columns = len(headers) + columns_max_widths = [len(str(header)) for header in headers] for row in rendered_rows: for idx, val in enumerate(row): columns_max_widths[idx] = max(len(str(val)), columns_max_widths[idx]) - if sum(columns_max_widths) > table_max_width: # We want to limit the widest column - largest_width = max(columns_max_widths) - widest_column_idx = columns_max_widths.index(largest_width) - diff = sum(columns_max_widths) - table_max_width - columns_max_widths[widest_column_idx] = largest_width - diff - if columns_max_widths[widest_column_idx] < 0: # in case the diff is bigger than the largest column - # just divide equally - columns_max_widths = [ - int(table_max_width / len(columns_max_widths)) for i in range(0, len(columns_max_widths)) - ] + if sum(columns_max_widths) <= table_max_width: + return columns_max_widths + + # Only shrink text columns; numeric columns (counters) are left intact so + # the numbers are never the ones that get truncated. + numeric_indices = cls.__numeric_column_indices(rendered_rows, num_columns) + shrinkable = [idx for idx in range(num_columns) if idx not in numeric_indices] + if not shrinkable: # nothing but numeric columns - fall back to shrinking everything + shrinkable = list(range(num_columns)) + + # Water-fill: repeatedly trim the currently-widest shrinkable column by one + # character. This distributes the reduction fairly (widest first) instead of + # gutting a single column, and never drops a column below TABLE_MIN_COLUMN_WIDTH. + while sum(columns_max_widths) > table_max_width: + candidates = [idx for idx in shrinkable if columns_max_widths[idx] > TABLE_MIN_COLUMN_WIDTH] + if not candidates: + break # can't shrink any further without making columns unreadable + widest = max(candidates, key=lambda idx: columns_max_widths[idx]) + columns_max_widths[widest] -= 1 return columns_max_widths + @classmethod + def __truncate_cell(cls, value: str, max_width: int) -> str: + # Truncate an over-wide cell to a single line with an ellipsis, instead of + # letting tabulate word-wrap it onto extra physical lines (which mangles + # every following row in the rendered table). + if max_width <= 0 or len(value) <= max_width: + return value + ellipsis = TABLE_TRUNCATION_ELLIPSIS + if max_width <= len(ellipsis): + return value[:max_width] + keep = max_width - len(ellipsis) + # For dotted, space-free qualified names (e.g. Java class paths like + # "ats.betting...settler.AbstractBetSettler") the distinctive part is the + # suffix (the class name), so trim from the left and keep the tail. For any + # other text a trailing cut reads more naturally. + if "." in value and " " not in value: + return ellipsis + value[-keep:] + return value[:keep] + ellipsis + @classmethod def __trim_rows(cls, contents: str, max_chars: int): # We need to make sure that the total character count doesn't exceed max_chars, @@ -430,11 +487,19 @@ def to_markdown(self, max_chars=None, add_table_header: bool = True) -> Markdown def to_table_string(self, table_max_width: int = PRINTED_TABLE_MAX_WIDTH, table_fmt: str = "presto") -> str: rendered_rows = self.__to_strings_rows(self.render_rows()) col_max_width = self.__calc_max_width(self.headers, rendered_rows, table_max_width) + # Truncate over-wide cells ourselves rather than passing maxcolwidths to + # tabulate. tabulate would *wrap* long cells onto extra physical lines, + # which corrupts the whole table; truncating keeps every row on one line. + truncated_headers = [ + self.__truncate_cell(str(header), col_max_width[idx]) for idx, header in enumerate(self.headers) + ] + truncated_rows = [ + [self.__truncate_cell(val, col_max_width[idx]) for idx, val in enumerate(row)] for row in rendered_rows + ] return tabulate( - rendered_rows, - headers=self.headers, + truncated_rows, + headers=truncated_headers, tablefmt=table_fmt, - maxcolwidths=col_max_width, ) def render_rows(self) -> List[List]: diff --git a/tests/test_blocks.py b/tests/test_blocks.py index 86fdc4517..1ff5f6870 100644 --- a/tests/test_blocks.py +++ b/tests/test_blocks.py @@ -134,3 +134,85 @@ def test_all_block_types(slack_channel: SlackChannel): result = slack_sender.send_finding_to_slack(finding, slack_params, False) # result = slack_sender.send_finding_to_slack(finding, slack_params, True) print(result) + + +# Regression tests for FRO-211 / ROB-3946: the "Alerts Summary" digest table +# rendered a corrupted ASCII table because long cell values (Java class names in +# the label:site column) were word-wrapped onto extra physical lines instead of +# being truncated. These tests pin down that rows stay single-line and over-wide +# cells are truncated with an ellipsis. +LONG_CLASS_NAME = "ats.betting.betcatcher.settlement.settler.AbstractBetSettler" + + +def test_to_table_string_truncates_wide_column_without_wrapping(): + rows = [ + [LONG_CLASS_NAME, "103", "0"], + ["orders.checkout.impl.OrderServiceImpl", "16", "4"], + ] + table_block = TableBlock(rows=rows, headers=["label:site", "Fired", "Resolved"]) + + output = table_block.to_table_string(table_max_width=40) + lines = output.splitlines() + + # presto renders: header line + separator line + exactly one line per row. + # If any cell had wrapped, there would be extra physical lines here. + assert len(lines) == 2 + len(rows) + # Every physical line stays bounded: the content budget (table_max_width) plus + # the fixed per-column separator/padding overhead tabulate adds. Without the + # fix, wrapped cells would have blown past this and split rows across lines. + assert all(len(line) <= 40 + 6 * len(table_block.headers) for line in lines) + + # The over-wide class name is truncated, not present in full, and the "wrap + # spillover" fragments from the bug report never appear on their own line. + assert LONG_CLASS_NAME not in output + assert "…" in output + assert not any(line.strip() in ("ServiceImpl", "erviceImpl") for line in lines) + + +def test_to_table_string_keeps_distinctive_suffix_of_dotted_names(): + table_block = TableBlock(rows=[[LONG_CLASS_NAME, "103", "0"]], headers=["label:site", "Fired", "Resolved"]) + + output = table_block.to_table_string(table_max_width=40) + + # Dotted, space-free qualified names are trimmed from the left so the class + # name (the distinctive suffix) survives. + assert "AbstractBetSettler" in output + assert "ats.betting.betcatcher" not in output + + +def test_to_table_string_never_truncates_numeric_columns(): + table_block = TableBlock( + rows=[[LONG_CLASS_NAME, "103", "9999"]], + headers=["label:site", "Fired", "Resolved"], + ) + + output = table_block.to_table_string(table_max_width=40) + + # The numeric counters are always shown in full - only the wide text column shrinks. + assert "103" in output + assert "9999" in output + assert "…" not in output.split("103")[1] # nothing after the counters got ellipsized + + +def test_to_table_string_trailing_truncation_for_plain_text(): + long_sentence = "this is a fairly long free text value that should be cut at the end" + table_block = TableBlock(rows=[[long_sentence, "1", "0"]], headers=["message", "Fired", "Resolved"]) + + output = table_block.to_table_string(table_max_width=30) + + # Plain text (contains spaces) is truncated from the right, ending in an ellipsis. + assert "this is a fairly" in output + assert "…" in output + assert long_sentence not in output + + +def test_to_markdown_wide_table_stays_single_line_per_row(): + rows = [[LONG_CLASS_NAME, "103", "0"], ["orders.checkout.impl.OrderServiceImpl", "16", "4"]] + table_block = TableBlock(rows=rows, headers=["label:site", "Fired", "Resolved"]) + + markdown = table_block.to_markdown().text + + # Strip the ``` code fences, then assert the table body is header + separator + one line per row. + inner = markdown.strip().strip("`").strip("\n") + body_lines = [line for line in inner.splitlines() if line.strip()] + assert len(body_lines) == 2 + len(rows) From aeac17374161adf97df281f3b5e4d7e4bc93925f Mon Sep 17 00:00:00 2001 From: alonelish Date: Wed, 15 Jul 2026 15:01:07 +0300 Subject: [PATCH 2/8] ROB-3946 Address review: headerless/ragged rows and all-numeric tables - __calc_max_width now sizes columns to the widest row, not just the headers, so headerless or ragged tables no longer raise IndexError. - An all-numeric over-width table is left at full width instead of ellipsizing the numbers. - Shorten code/test comments; add regression tests for both cases. Co-Authored-By: Claude Opus 4.8 --- src/robusta/core/reporting/blocks.py | 52 +++++++++++----------------- tests/test_blocks.py | 42 +++++++++++++++------- 2 files changed, 51 insertions(+), 43 deletions(-) diff --git a/src/robusta/core/reporting/blocks.py b/src/robusta/core/reporting/blocks.py index 3de9784e5..638b9874b 100644 --- a/src/robusta/core/reporting/blocks.py +++ b/src/robusta/core/reporting/blocks.py @@ -38,11 +38,9 @@ def tabulate(*args, **kwargs): BLOCK_SIZE_LIMIT = 2997 # due to slack block size limit of 3000 -# A single-character ellipsis is used (rather than "...") for table-cell truncation -# so the width accounting stays exact - it consumes exactly one column of the cell. +# Single-char ellipsis so truncated-cell width accounting stays exact (one column). TABLE_TRUNCATION_ELLIPSIS = "…" -# Never shrink a text column below this many characters, otherwise the value -# becomes unreadable (essentially just the ellipsis). +# Don't shrink a text column below this, or it becomes just the ellipsis. TABLE_MIN_COLUMN_WIDTH = 4 @@ -381,23 +379,21 @@ def __is_number(cls, value) -> bool: @classmethod def __numeric_column_indices(cls, rendered_rows, num_columns: int) -> set: - # A column is considered numeric when every non-empty cell parses as a - # number (e.g. the "Fired"/"Resolved" counters). These are kept at full - # width so the counts always stay readable. + # Numeric columns (e.g. the Fired/Resolved counters) are kept at full width. numeric_indices = set() for idx in range(num_columns): - values = [str(row[idx]) for row in rendered_rows if idx < len(row)] - non_empty = [v for v in values if v.strip() != ""] + non_empty = [str(row[idx]) for row in rendered_rows if idx < len(row) and str(row[idx]).strip()] if non_empty and all(cls.__is_number(v) for v in non_empty): numeric_indices.add(idx) return numeric_indices @classmethod def __calc_max_width(cls, headers, rendered_rows, table_max_width: int) -> List[int]: - # We need to make sure the total table width doesn't exceed the max width, - # otherwise the table is printed corrupted. - num_columns = len(headers) - columns_max_widths = [len(str(header)) for header in headers] + # Keep the total table width within the max, otherwise it renders corrupted. + num_columns = max(len(headers), max((len(row) for row in rendered_rows), default=0)) + columns_max_widths = [0] * num_columns + for idx, header in enumerate(headers): + columns_max_widths[idx] = len(str(header)) for row in rendered_rows: for idx, val in enumerate(row): columns_max_widths[idx] = max(len(str(val)), columns_max_widths[idx]) @@ -405,20 +401,19 @@ def __calc_max_width(cls, headers, rendered_rows, table_max_width: int) -> List[ if sum(columns_max_widths) <= table_max_width: return columns_max_widths - # Only shrink text columns; numeric columns (counters) are left intact so - # the numbers are never the ones that get truncated. + # Only shrink text columns, so numeric columns are never truncated. If every + # column is numeric, leave the widths as-is rather than ellipsizing numbers. numeric_indices = cls.__numeric_column_indices(rendered_rows, num_columns) shrinkable = [idx for idx in range(num_columns) if idx not in numeric_indices] - if not shrinkable: # nothing but numeric columns - fall back to shrinking everything - shrinkable = list(range(num_columns)) + if not shrinkable: + return columns_max_widths - # Water-fill: repeatedly trim the currently-widest shrinkable column by one - # character. This distributes the reduction fairly (widest first) instead of - # gutting a single column, and never drops a column below TABLE_MIN_COLUMN_WIDTH. + # Water-fill: trim the widest shrinkable column by one char at a time (fair + # distribution), never below TABLE_MIN_COLUMN_WIDTH. while sum(columns_max_widths) > table_max_width: candidates = [idx for idx in shrinkable if columns_max_widths[idx] > TABLE_MIN_COLUMN_WIDTH] if not candidates: - break # can't shrink any further without making columns unreadable + break widest = max(candidates, key=lambda idx: columns_max_widths[idx]) columns_max_widths[widest] -= 1 @@ -426,19 +421,15 @@ def __calc_max_width(cls, headers, rendered_rows, table_max_width: int) -> List[ @classmethod def __truncate_cell(cls, value: str, max_width: int) -> str: - # Truncate an over-wide cell to a single line with an ellipsis, instead of - # letting tabulate word-wrap it onto extra physical lines (which mangles - # every following row in the rendered table). + # Truncate over-wide cells to one line, instead of letting tabulate wrap them. if max_width <= 0 or len(value) <= max_width: return value ellipsis = TABLE_TRUNCATION_ELLIPSIS if max_width <= len(ellipsis): return value[:max_width] keep = max_width - len(ellipsis) - # For dotted, space-free qualified names (e.g. Java class paths like - # "ats.betting...settler.AbstractBetSettler") the distinctive part is the - # suffix (the class name), so trim from the left and keep the tail. For any - # other text a trailing cut reads more naturally. + # Dotted, space-free names (e.g. Java class paths) keep their distinctive + # suffix via a left trim; other text is trimmed on the right. if "." in value and " " not in value: return ellipsis + value[-keep:] return value[:keep] + ellipsis @@ -487,9 +478,8 @@ def to_markdown(self, max_chars=None, add_table_header: bool = True) -> Markdown def to_table_string(self, table_max_width: int = PRINTED_TABLE_MAX_WIDTH, table_fmt: str = "presto") -> str: rendered_rows = self.__to_strings_rows(self.render_rows()) col_max_width = self.__calc_max_width(self.headers, rendered_rows, table_max_width) - # Truncate over-wide cells ourselves rather than passing maxcolwidths to - # tabulate. tabulate would *wrap* long cells onto extra physical lines, - # which corrupts the whole table; truncating keeps every row on one line. + # Truncate over-wide cells ourselves; tabulate's maxcolwidths would wrap them + # onto extra lines and corrupt the table. truncated_headers = [ self.__truncate_cell(str(header), col_max_width[idx]) for idx, header in enumerate(self.headers) ] diff --git a/tests/test_blocks.py b/tests/test_blocks.py index 1ff5f6870..e92af01a1 100644 --- a/tests/test_blocks.py +++ b/tests/test_blocks.py @@ -136,11 +136,8 @@ def test_all_block_types(slack_channel: SlackChannel): print(result) -# Regression tests for FRO-211 / ROB-3946: the "Alerts Summary" digest table -# rendered a corrupted ASCII table because long cell values (Java class names in -# the label:site column) were word-wrapped onto extra physical lines instead of -# being truncated. These tests pin down that rows stay single-line and over-wide -# cells are truncated with an ellipsis. +# Regression tests for FRO-211 / ROB-3946: over-wide cells must be truncated to a +# single line, not word-wrapped onto extra lines (which corrupted the digest table). LONG_CLASS_NAME = "ats.betting.betcatcher.settlement.settler.AbstractBetSettler" @@ -154,16 +151,12 @@ def test_to_table_string_truncates_wide_column_without_wrapping(): output = table_block.to_table_string(table_max_width=40) lines = output.splitlines() - # presto renders: header line + separator line + exactly one line per row. - # If any cell had wrapped, there would be extra physical lines here. + # presto renders header + separator + one line per row; wrapping would add lines. assert len(lines) == 2 + len(rows) - # Every physical line stays bounded: the content budget (table_max_width) plus - # the fixed per-column separator/padding overhead tabulate adds. Without the - # fix, wrapped cells would have blown past this and split rows across lines. + # Bounded width: content budget + tabulate's per-column separator/padding overhead. assert all(len(line) <= 40 + 6 * len(table_block.headers) for line in lines) - # The over-wide class name is truncated, not present in full, and the "wrap - # spillover" fragments from the bug report never appear on their own line. + # The class name is truncated, not present in full, and no wrap-spillover fragment. assert LONG_CLASS_NAME not in output assert "…" in output assert not any(line.strip() in ("ServiceImpl", "erviceImpl") for line in lines) @@ -216,3 +209,28 @@ def test_to_markdown_wide_table_stays_single_line_per_row(): inner = markdown.strip().strip("`").strip("\n") body_lines = [line for line in inner.splitlines() if line.strip()] assert len(body_lines) == 2 + len(rows) + + +def test_to_table_string_headerless_and_ragged_rows(): + # No headers, and rows wider than the (empty) header list must not IndexError. + table_block = TableBlock(rows=[[LONG_CLASS_NAME, "extra", "cols"]], headers=[]) + + output = table_block.to_table_string(table_max_width=30) + + assert output # rendered without raising + assert LONG_CLASS_NAME not in output # still truncated + assert "…" in output + + +def test_to_table_string_all_numeric_table_is_not_truncated(): + # Every column numeric and over budget: leave widths intact rather than ellipsize numbers. + table_block = TableBlock( + rows=[["123456789012345", "678901234567890", "112233445566778"]], + headers=["a", "b", "c"], + ) + + output = table_block.to_table_string(table_max_width=10) + + assert "…" not in output + for value in ("123456789012345", "678901234567890", "112233445566778"): + assert value in output From 82fb6a2b17cd8dba7c3ef6f56f1bbe7ba60dd125 Mon Sep 17 00:00:00 2001 From: alonelish Date: Wed, 15 Jul 2026 15:19:23 +0300 Subject: [PATCH 3/8] ROB-3946 Genericize numeric-column comment in shared blocks module Drop the Slack-digest-specific "Fired/Resolved" example, since blocks.py is core code shared across all sinks. Co-Authored-By: Claude Opus 4.8 --- src/robusta/core/reporting/blocks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/robusta/core/reporting/blocks.py b/src/robusta/core/reporting/blocks.py index 638b9874b..e615f85a1 100644 --- a/src/robusta/core/reporting/blocks.py +++ b/src/robusta/core/reporting/blocks.py @@ -379,7 +379,7 @@ def __is_number(cls, value) -> bool: @classmethod def __numeric_column_indices(cls, rendered_rows, num_columns: int) -> set: - # Numeric columns (e.g. the Fired/Resolved counters) are kept at full width. + # Numeric columns (e.g. counters) are kept at full width. numeric_indices = set() for idx in range(num_columns): non_empty = [str(row[idx]) for row in rendered_rows if idx < len(row) and str(row[idx]).strip()] From e24c43d8aaa5906ee327397adb784045c5fb0b2f Mon Sep 17 00:00:00 2001 From: alonelish Date: Sun, 26 Jul 2026 14:25:43 +0300 Subject: [PATCH 4/8] ROB-3946 Keep full values (wrap) and drop whole rows when over the limit Reviewer feedback: the full alert path must stay visible, so revert the cell-level ellipsis truncation and let long values wrap as before. Instead of cutting values, guard the real breakage: - to_markdown now renders within a char budget and drops WHOLE rows, appending "... N more rows not shown". The previous line-based trim cut mid-wrapped-row, leaving a value's continuation behind and presenting a partial name as if it were complete. - The budget always stays under BLOCK_SIZE_LIMIT, so MarkdownBlock can no longer blind-cut the text and lose the closing ``` fence - which is what made the large digest render as unformatted, non-monospace text. - Column widths are computed once from all rows so dropping rows doesn't reflow the table. to_table_string keeps its previous wrapping behaviour; only column width allocation changed (numeric columns are never shrunk, so counters don't wrap, plus the headerless/ragged-row fix). Co-Authored-By: Claude Opus 4.8 --- src/robusta/core/reporting/blocks.py | 84 ++++++++++------- tests/test_blocks.py | 130 +++++++++++++-------------- 2 files changed, 112 insertions(+), 102 deletions(-) diff --git a/src/robusta/core/reporting/blocks.py b/src/robusta/core/reporting/blocks.py index e615f85a1..e69699a7d 100644 --- a/src/robusta/core/reporting/blocks.py +++ b/src/robusta/core/reporting/blocks.py @@ -38,9 +38,7 @@ def tabulate(*args, **kwargs): BLOCK_SIZE_LIMIT = 2997 # due to slack block size limit of 3000 -# Single-char ellipsis so truncated-cell width accounting stays exact (one column). -TABLE_TRUNCATION_ELLIPSIS = "…" -# Don't shrink a text column below this, or it becomes just the ellipsis. +# Don't shrink a text column below this, or its values become unreadable. TABLE_MIN_COLUMN_WIDTH = 4 @@ -401,8 +399,8 @@ def __calc_max_width(cls, headers, rendered_rows, table_max_width: int) -> List[ if sum(columns_max_widths) <= table_max_width: return columns_max_widths - # Only shrink text columns, so numeric columns are never truncated. If every - # column is numeric, leave the widths as-is rather than ellipsizing numbers. + # Only shrink text columns, so numeric columns never get wrapped. If every + # column is numeric, leave the widths as-is rather than wrapping numbers. numeric_indices = cls.__numeric_column_indices(rendered_rows, num_columns) shrinkable = [idx for idx in range(num_columns) if idx not in numeric_indices] if not shrinkable: @@ -419,21 +417,6 @@ def __calc_max_width(cls, headers, rendered_rows, table_max_width: int) -> List[ return columns_max_widths - @classmethod - def __truncate_cell(cls, value: str, max_width: int) -> str: - # Truncate over-wide cells to one line, instead of letting tabulate wrap them. - if max_width <= 0 or len(value) <= max_width: - return value - ellipsis = TABLE_TRUNCATION_ELLIPSIS - if max_width <= len(ellipsis): - return value[:max_width] - keep = max_width - len(ellipsis) - # Dotted, space-free names (e.g. Java class paths) keep their distinctive - # suffix via a left trim; other text is trimmed on the right. - if "." in value and " " not in value: - return ellipsis + value[-keep:] - return value[:keep] + ellipsis - @classmethod def __trim_rows(cls, contents: str, max_chars: int): # We need to make sure that the total character count doesn't exceed max_chars, @@ -458,6 +441,43 @@ def __trim_rows(cls, contents: str, max_chars: int): return "\n".join(lines[:lines_to_include]) + truncator + def __render_within_budget(self, max_chars: int, table_max_width: int, table_fmt: str) -> str: + """Render the table, dropping whole rows to fit max_chars. + + Values themselves are never cut - long ones wrap onto extra lines - so rows must be + dropped as complete units. Cutting by physical line would leave a wrapped value's + continuation behind and show a partial value as if it were the whole thing. + """ + rows = self.__to_strings_rows(self.render_rows()) + # Column widths are computed once from all rows, so dropping rows never reflows + # the columns and the rendered size shrinks monotonically with the row count. + col_max_width = self.__calc_max_width(self.headers, rows, table_max_width) + + def render(subset: List[List[str]]) -> str: + return tabulate(subset, headers=self.headers, tablefmt=table_fmt, maxcolwidths=col_max_width) + + full = render(rows) + if len(full) <= max_chars: + return full + + # Largest number of whole rows that fits alongside the "N more rows" note. + best = None + low, high = 0, len(rows) + while low <= high: + mid = (low + high) // 2 + candidate = render(rows[:mid]) + omitted = len(rows) - mid + if omitted: + candidate = f"{candidate}\n... {omitted} more rows not shown" + if len(candidate) <= max_chars: + best = candidate + low = mid + 1 + else: + high = mid - 1 + + # Fall back to the line-based trim if not even a header-only table fits. + return best if best is not None else self.__trim_rows(full, max_chars) + @classmethod def __to_strings_rows(cls, rows): # This is just to assert all row column values are strings. Tabulate might fail on other types @@ -468,28 +488,24 @@ def to_markdown(self, max_chars=None, add_table_header: bool = True) -> Markdown table_header = "" if not add_table_header else table_header prefix = f"{table_header}```\n" suffix = "\n```" - table_contents = self.to_table_string() - if max_chars is not None: - max_chars = max_chars - len(prefix) - len(suffix) - table_contents = self.__trim_rows(table_contents, max_chars) + # Stay under BLOCK_SIZE_LIMIT even when no caller-supplied limit: MarkdownBlock + # would otherwise blindly cut the text and lose the closing ``` fence, which + # makes the whole table render as unformatted (non-monospace) text. + budget = BLOCK_SIZE_LIMIT - 1 if max_chars is None else min(max_chars, BLOCK_SIZE_LIMIT - 1) + table_contents = self.__render_within_budget( + budget - len(prefix) - len(suffix), PRINTED_TABLE_MAX_WIDTH, "presto" + ) return MarkdownBlock(f"{prefix}{table_contents}{suffix}") def to_table_string(self, table_max_width: int = PRINTED_TABLE_MAX_WIDTH, table_fmt: str = "presto") -> str: rendered_rows = self.__to_strings_rows(self.render_rows()) col_max_width = self.__calc_max_width(self.headers, rendered_rows, table_max_width) - # Truncate over-wide cells ourselves; tabulate's maxcolwidths would wrap them - # onto extra lines and corrupt the table. - truncated_headers = [ - self.__truncate_cell(str(header), col_max_width[idx]) for idx, header in enumerate(self.headers) - ] - truncated_rows = [ - [self.__truncate_cell(val, col_max_width[idx]) for idx, val in enumerate(row)] for row in rendered_rows - ] return tabulate( - truncated_rows, - headers=truncated_headers, + rendered_rows, + headers=self.headers, tablefmt=table_fmt, + maxcolwidths=col_max_width, ) def render_rows(self) -> List[List]: diff --git a/tests/test_blocks.py b/tests/test_blocks.py index e92af01a1..662feea25 100644 --- a/tests/test_blocks.py +++ b/tests/test_blocks.py @@ -1,4 +1,5 @@ import json +import re from hikaru.model.rel_1_26 import HikaruDocumentBase, ObjectMeta, Pod @@ -19,6 +20,7 @@ TableBlock, action, ) +from robusta.core.reporting.blocks import BLOCK_SIZE_LIMIT from robusta.core.reporting.consts import ScanType from robusta.core.sinks.slack.slack_sink_params import SlackSinkParams from tests.config import CONFIG @@ -136,101 +138,93 @@ def test_all_block_types(slack_channel: SlackChannel): print(result) -# Regression tests for FRO-211 / ROB-3946: over-wide cells must be truncated to a -# single line, not word-wrapped onto extra lines (which corrupted the digest table). +# Regression tests for FRO-211 / ROB-3946: long values must stay complete (wrapped +# onto extra lines, never cut mid-value), and an over-long table must drop whole rows +# with a note rather than blow past Slack's block limit and lose its closing ``` fence. LONG_CLASS_NAME = "ats.betting.betcatcher.settlement.settler.AbstractBetSettler" -def test_to_table_string_truncates_wide_column_without_wrapping(): - rows = [ - [LONG_CLASS_NAME, "103", "0"], - ["orders.checkout.impl.OrderServiceImpl", "16", "4"], - ] - table_block = TableBlock(rows=rows, headers=["label:site", "Fired", "Resolved"]) - - output = table_block.to_table_string(table_max_width=40) - lines = output.splitlines() - - # presto renders header + separator + one line per row; wrapping would add lines. - assert len(lines) == 2 + len(rows) - # Bounded width: content budget + tabulate's per-column separator/padding overhead. - assert all(len(line) <= 40 + 6 * len(table_block.headers) for line in lines) +def _column_values(output, column_index=0): + """Rebuild each logical row's column value from a wrapped presto table.""" + values, lines = [], output.splitlines() + for line in lines[2:]: # skip header + separator + if "|" not in line: + continue + cell = line.split("|")[column_index].strip() + starts_new_row = all(part.strip() for part in line.split("|")[1:]) + if starts_new_row: + values.append(cell) + elif values: + values[-1] += cell # continuation of the previous row's wrapped value + return values - # The class name is truncated, not present in full, and no wrap-spillover fragment. - assert LONG_CLASS_NAME not in output - assert "…" in output - assert not any(line.strip() in ("ServiceImpl", "erviceImpl") for line in lines) - -def test_to_table_string_keeps_distinctive_suffix_of_dotted_names(): - table_block = TableBlock(rows=[[LONG_CLASS_NAME, "103", "0"]], headers=["label:site", "Fired", "Resolved"]) +def test_long_values_are_wrapped_not_cut(): + rows = [[LONG_CLASS_NAME, "103", "0"], ["orders.checkout.impl.OrderServiceImpl", "16", "4"]] + table_block = TableBlock(rows=rows, headers=["label:site", "Fired", "Resolved"]) output = table_block.to_table_string(table_max_width=40) - # Dotted, space-free qualified names are trimmed from the left so the class - # name (the distinctive suffix) survives. - assert "AbstractBetSettler" in output - assert "ats.betting.betcatcher" not in output + # The full path survives, reassembled across the wrapped lines - nothing is elided. + assert _column_values(output) == [LONG_CLASS_NAME, "orders.checkout.impl.OrderServiceImpl"] + assert "…" not in output -def test_to_table_string_never_truncates_numeric_columns(): +def test_numeric_columns_are_not_shrunk(): table_block = TableBlock( - rows=[[LONG_CLASS_NAME, "103", "9999"]], - headers=["label:site", "Fired", "Resolved"], + rows=[[LONG_CLASS_NAME, "103", "9999"]], headers=["label:site", "Fired", "Resolved"] ) output = table_block.to_table_string(table_max_width=40) - # The numeric counters are always shown in full - only the wide text column shrinks. - assert "103" in output - assert "9999" in output - assert "…" not in output.split("103")[1] # nothing after the counters got ellipsized - + # Counters stay on one line - only the wide text column absorbs the reduction. + assert "103" in output and "9999" in output + assert all(line.count("|") == 2 for line in output.splitlines() if "|" in line) -def test_to_table_string_trailing_truncation_for_plain_text(): - long_sentence = "this is a fairly long free text value that should be cut at the end" - table_block = TableBlock(rows=[[long_sentence, "1", "0"]], headers=["message", "Fired", "Resolved"]) - output = table_block.to_table_string(table_max_width=30) +def test_all_numeric_table_is_not_shrunk(): + values = ["123456789012345", "678901234567890", "112233445566778"] + table_block = TableBlock(rows=[values], headers=["a", "b", "c"]) - # Plain text (contains spaces) is truncated from the right, ending in an ellipsis. - assert "this is a fairly" in output - assert "…" in output - assert long_sentence not in output + output = table_block.to_table_string(table_max_width=10) + for value in values: + assert value in output -def test_to_markdown_wide_table_stays_single_line_per_row(): - rows = [[LONG_CLASS_NAME, "103", "0"], ["orders.checkout.impl.OrderServiceImpl", "16", "4"]] - table_block = TableBlock(rows=rows, headers=["label:site", "Fired", "Resolved"]) - markdown = table_block.to_markdown().text +def test_headerless_and_ragged_rows(): + # No headers, and rows wider than the (empty) header list must not IndexError. + table_block = TableBlock(rows=[[LONG_CLASS_NAME, "extra", "cols"]], headers=[]) - # Strip the ``` code fences, then assert the table body is header + separator + one line per row. - inner = markdown.strip().strip("`").strip("\n") - body_lines = [line for line in inner.splitlines() if line.strip()] - assert len(body_lines) == 2 + len(rows) + assert table_block.to_table_string(table_max_width=30) -def test_to_table_string_headerless_and_ragged_rows(): - # No headers, and rows wider than the (empty) header list must not IndexError. - table_block = TableBlock(rows=[[LONG_CLASS_NAME, "extra", "cols"]], headers=[]) +def test_to_markdown_small_table_has_no_omission_note(): + table_block = TableBlock(rows=[[LONG_CLASS_NAME, "1", "0"]], headers=["label:site", "Fired", "Resolved"]) - output = table_block.to_table_string(table_max_width=30) + markdown = table_block.to_markdown().text - assert output # rendered without raising - assert LONG_CLASS_NAME not in output # still truncated - assert "…" in output + assert "more rows not shown" not in markdown + assert markdown.startswith("```") and markdown.endswith("```") -def test_to_table_string_all_numeric_table_is_not_truncated(): - # Every column numeric and over budget: leave widths intact rather than ellipsize numbers. - table_block = TableBlock( - rows=[["123456789012345", "678901234567890", "112233445566778"]], - headers=["a", "b", "c"], - ) +def test_to_markdown_drops_whole_rows_and_keeps_code_fence(): + # The 4-column shape of the "Alerts Summary" digest that triggered the bug. + rows = [[f"ats.betting.betcatcher.validation.impl.Validator{i:03d}Foo", "nj", "1", "0"] for i in range(200)] + table_block = TableBlock(rows=rows, headers=["label:site", "label:component", "Fired", "Resolved"]) - output = table_block.to_table_string(table_max_width=10) + markdown = table_block.to_markdown().text - assert "…" not in output - for value in ("123456789012345", "678901234567890", "112233445566778"): - assert value in output + # Small enough that MarkdownBlock never blind-cuts it, so the fence is intact. + assert len(markdown) < BLOCK_SIZE_LIMIT + assert markdown.startswith("```") and markdown.endswith("```") + # The reader is told what was left out. + assert re.search(r"\.\.\. \d+ more rows not shown", markdown) + + # Every displayed value is a complete class name - rows are dropped as whole units, + # so no row is left showing only the first half of a wrapped value. + inner = markdown.split("```")[1] + displayed = _column_values(inner) + assert displayed # some rows did survive + for value in displayed: + assert re.fullmatch(r"ats\.betting\.betcatcher\.validation\.impl\.Validator\d{3}Foo", value), value From d41907cf623fb475f86a0a259b7d660c2c9a70f0 Mon Sep 17 00:00:00 2001 From: alonelish Date: Mon, 27 Jul 2026 08:25:06 +0300 Subject: [PATCH 5/8] ROB-3946 Sort summary rows by significance and link to the full timeline When the digest is too long to fit in a Slack message, rows are dropped. Previously the rows were sorted alphabetically, so whichever groups happened to sort last were the ones discarded - unrelated to how noisy they were. Sort by notification count instead (key as a stable secondary criterion), so the dropped rows are the least significant ones. Also: - The omission note now carries the residual totals ("... 44 more groups (89 fired, 39 resolved) not shown"), so the table still reconciles with the notification count in the message header. TableBlock takes an optional omission_note callback for this, keeping the digest-specific wording out of the shared reporting module. - Add a "View all N groups" link below the table, since rows may be hidden. It has to sit outside the code block - Slack won't render a link inside one. - get_timeline_uri now passes "clusters" as a JSON list, which is what the timeline actually reads; the previous singular "cluster" param was ignored, so the existing Investigate link was never cluster-filtered. Co-Authored-By: Claude Opus 4.8 --- src/robusta/core/reporting/blocks.py | 20 ++++++++---- src/robusta/core/sinks/slack/slack_sink.py | 10 +++++- src/robusta/integrations/slack/sender.py | 36 ++++++++++++++++++++-- tests/test_blocks.py | 17 ++++++++++ 4 files changed, 74 insertions(+), 9 deletions(-) diff --git a/src/robusta/core/reporting/blocks.py b/src/robusta/core/reporting/blocks.py index e69699a7d..7bc66ac44 100644 --- a/src/robusta/core/reporting/blocks.py +++ b/src/robusta/core/reporting/blocks.py @@ -441,7 +441,11 @@ def __trim_rows(cls, contents: str, max_chars: int): return "\n".join(lines[:lines_to_include]) + truncator - def __render_within_budget(self, max_chars: int, table_max_width: int, table_fmt: str) -> str: + @staticmethod + def default_omission_note(omitted_rows: List[List[str]]) -> str: + return f"... {len(omitted_rows)} more rows not shown" + + def __render_within_budget(self, max_chars: int, table_max_width: int, table_fmt: str, omission_note) -> str: """Render the table, dropping whole rows to fit max_chars. Values themselves are never cut - long ones wrap onto extra lines - so rows must be @@ -466,9 +470,8 @@ def render(subset: List[List[str]]) -> str: while low <= high: mid = (low + high) // 2 candidate = render(rows[:mid]) - omitted = len(rows) - mid - if omitted: - candidate = f"{candidate}\n... {omitted} more rows not shown" + if mid < len(rows): + candidate = f"{candidate}\n{omission_note(rows[mid:])}" if len(candidate) <= max_chars: best = candidate low = mid + 1 @@ -483,7 +486,9 @@ def __to_strings_rows(cls, rows): # This is just to assert all row column values are strings. Tabulate might fail on other types return [list(map(lambda column_value: str(column_value), row)) for row in rows] - def to_markdown(self, max_chars=None, add_table_header: bool = True) -> MarkdownBlock: + def to_markdown(self, max_chars=None, add_table_header: bool = True, omission_note=None) -> MarkdownBlock: + """:param omission_note: builds the note shown when rows are dropped to fit the size limit. + Receives the dropped rows, so callers can summarise what was left out (e.g. residual counts).""" table_header = f"{self.table_name}\n" if self.table_name else "" table_header = "" if not add_table_header else table_header prefix = f"{table_header}```\n" @@ -493,7 +498,10 @@ def to_markdown(self, max_chars=None, add_table_header: bool = True) -> Markdown # makes the whole table render as unformatted (non-monospace) text. budget = BLOCK_SIZE_LIMIT - 1 if max_chars is None else min(max_chars, BLOCK_SIZE_LIMIT - 1) table_contents = self.__render_within_budget( - budget - len(prefix) - len(suffix), PRINTED_TABLE_MAX_WIDTH, "presto" + budget - len(prefix) - len(suffix), + PRINTED_TABLE_MAX_WIDTH, + "presto", + omission_note or self.default_omission_note, ) return MarkdownBlock(f"{prefix}{table_contents}{suffix}") diff --git a/src/robusta/core/sinks/slack/slack_sink.py b/src/robusta/core/sinks/slack/slack_sink.py index afd2d9ab9..ffb083899 100644 --- a/src/robusta/core/sinks/slack/slack_sink.py +++ b/src/robusta/core/sinks/slack/slack_sink.py @@ -1,4 +1,6 @@ +import json import logging +from urllib.parse import urlencode from robusta.core.model.env_vars import ROBUSTA_UI_DOMAIN from robusta.core.reporting.base import Finding, FindingStatus @@ -104,7 +106,13 @@ def handle_notification_grouping(self, finding: Finding, platform_enabled: bool) ) def get_timeline_uri(self, account_id: str, cluster_name: str) -> str: - return f"{ROBUSTA_UI_DOMAIN}/graphs?account_id={account_id}&cluster={cluster_name}" + # The timeline reads its filters as JSON-encoded query params, and expects "clusters" + # (a list) rather than a single "cluster" - passing the latter left the link unfiltered. + params = { + "account_id": account_id, + "clusters": json.dumps([cluster_name]), + } + return f"{ROBUSTA_UI_DOMAIN}/graphs?{urlencode(params)}" def __replace_callback_with_string(self, slack_message, block_id, message_string): """ diff --git a/src/robusta/integrations/slack/sender.py b/src/robusta/integrations/slack/sender.py index 682fe7263..c386dcc8e 100644 --- a/src/robusta/integrations/slack/sender.py +++ b/src/robusta/integrations/slack/sender.py @@ -860,6 +860,28 @@ def __send_finding_to_slack_preview( output_blocks=slack_blocks, ) + @staticmethod + def __summary_table_block(table_block: TableBlock) -> BaseBlock: + """Render the summary table, noting the totals of any rows dropped to fit the size limit. + + Without the residual counts the numbers in the table wouldn't add up to the notification + count in the message header, making the summary look wrong. + """ + if len(table_block.headers) <= 2: + return table_block # rendered as a bullet list rather than a table, see __to_slack_table + + def omission_note(omitted_rows: List[List[str]]) -> str: + fired = resolved = 0 + for row in omitted_rows: + try: + fired += int(row[-2]) + resolved += int(row[-1]) + except (IndexError, ValueError): # not the expected numeric columns - just count rows + return TableBlock.default_omission_note(omitted_rows) + return f"... {len(omitted_rows)} more groups ({fired} fired, {resolved} resolved) not shown" + + return table_block.to_markdown(omission_note=omission_note) + def send_or_update_summary_message( self, group_by_classification_header: List[str], @@ -878,7 +900,11 @@ def send_or_update_summary_message( fired/resolved and a header describing the event group that this information concerns.""" rows = [] n_total_alerts = 0 - for key, value in sorted(summary_table.items()): + # Sort by notification count (descending), so that when the table is too long to fit + # in a message the rows that get dropped are the least significant ones rather than + # whichever groups happen to sort last alphabetically. The key is a secondary sort + # criterion, to keep the order stable between updates for groups with equal counts. + for key, value in sorted(summary_table.items(), key=lambda item: (-sum(item[1]), item[0])): # key is a tuple of attribute names; value is a 2-element list with # the number of firing and resolved notifications. row = list(str(e) for e in chain(key, value)) @@ -928,10 +954,16 @@ def send_or_update_summary_message( [ MarkdownBlock(f"*Matching criteria*: {group_by_criteria_str}"), MarkdownBlock(text=time_text), - table_block, + self.__summary_table_block(table_block), ] ) + if platform_enabled and sink_params.investigate_link and investigate_uri: + # Rows may have been dropped to fit the message size limit, so always offer a way + # to see every group. Must be outside the table's code block - Slack doesn't + # render links inside one. + blocks.append(MarkdownBlock(text=f"<{investigate_uri}|View all {len(rows)} groups →>")) + if threaded: blocks.append(MarkdownBlock(text="See thread for individual alerts")) diff --git a/tests/test_blocks.py b/tests/test_blocks.py index 662feea25..93fc093b3 100644 --- a/tests/test_blocks.py +++ b/tests/test_blocks.py @@ -228,3 +228,20 @@ def test_to_markdown_drops_whole_rows_and_keeps_code_fence(): assert displayed # some rows did survive for value in displayed: assert re.fullmatch(r"ats\.betting\.betcatcher\.validation\.impl\.Validator\d{3}Foo", value), value + + +def test_to_markdown_custom_omission_note_receives_dropped_rows(): + rows = [[f"ats.betting.betcatcher.validation.impl.Validator{i:03d}Foo", "nj", "1", "2"] for i in range(200)] + table_block = TableBlock(rows=rows, headers=["label:site", "label:component", "Fired", "Resolved"]) + + def omission_note(omitted): + return f"... {len(omitted)} more groups ({sum(int(r[-2]) for r in omitted)} fired) not shown" + + markdown = table_block.to_markdown(omission_note=omission_note).text + + # The note reports the residual totals, so the table still reconciles with the header count. + match = re.search(r"\.\.\. (\d+) more groups \((\d+) fired\) not shown", markdown) + assert match, markdown[-200:] + omitted_count, omitted_fired = int(match.group(1)), int(match.group(2)) + assert omitted_fired == omitted_count # one "fired" per dropped row + assert len(markdown) < BLOCK_SIZE_LIMIT From 4a85cf534133f6046704331583fad4768db145d4 Mon Sep 17 00:00:00 2001 From: alonelish Date: Mon, 27 Jul 2026 08:51:04 +0300 Subject: [PATCH 6/8] ROB-3946 Sort by fired/resolved counts and fix empty-table rendering - Sort summary rows by firing count, then resolved count, rather than by the total. The group key is now only a final tie-break, so the Fired column reads in order instead of interleaving groups with equal totals. - Fix an IndexError when rendering a table with no rows: tabulate rejects maxcolwidths when there are no rows. This was pre-existing in to_table_string, and the new row-dropping path could also hit it when not even one row fits the size limit. - Add regression tests: an empty table renders, and to_markdown always closes its code fence and stays under BLOCK_SIZE_LIMIT across a sweep of row counts and value widths. Co-Authored-By: Claude Opus 4.8 --- src/robusta/core/reporting/blocks.py | 4 ++++ src/robusta/integrations/slack/sender.py | 10 +++++----- tests/test_blocks.py | 22 ++++++++++++++++++++++ 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/robusta/core/reporting/blocks.py b/src/robusta/core/reporting/blocks.py index 7bc66ac44..edbd2fbf9 100644 --- a/src/robusta/core/reporting/blocks.py +++ b/src/robusta/core/reporting/blocks.py @@ -458,6 +458,8 @@ def __render_within_budget(self, max_chars: int, table_max_width: int, table_fmt col_max_width = self.__calc_max_width(self.headers, rows, table_max_width) def render(subset: List[List[str]]) -> str: + if not subset: # tabulate raises IndexError on maxcolwidths with no rows + return tabulate(subset, headers=self.headers, tablefmt=table_fmt) return tabulate(subset, headers=self.headers, tablefmt=table_fmt, maxcolwidths=col_max_width) full = render(rows) @@ -508,6 +510,8 @@ def to_markdown(self, max_chars=None, add_table_header: bool = True, omission_no def to_table_string(self, table_max_width: int = PRINTED_TABLE_MAX_WIDTH, table_fmt: str = "presto") -> str: rendered_rows = self.__to_strings_rows(self.render_rows()) + if not rendered_rows: # tabulate raises IndexError on maxcolwidths with no rows + return tabulate(rendered_rows, headers=self.headers, tablefmt=table_fmt) col_max_width = self.__calc_max_width(self.headers, rendered_rows, table_max_width) return tabulate( rendered_rows, diff --git a/src/robusta/integrations/slack/sender.py b/src/robusta/integrations/slack/sender.py index c386dcc8e..09e701abf 100644 --- a/src/robusta/integrations/slack/sender.py +++ b/src/robusta/integrations/slack/sender.py @@ -900,11 +900,11 @@ def send_or_update_summary_message( fired/resolved and a header describing the event group that this information concerns.""" rows = [] n_total_alerts = 0 - # Sort by notification count (descending), so that when the table is too long to fit - # in a message the rows that get dropped are the least significant ones rather than - # whichever groups happen to sort last alphabetically. The key is a secondary sort - # criterion, to keep the order stable between updates for groups with equal counts. - for key, value in sorted(summary_table.items(), key=lambda item: (-sum(item[1]), item[0])): + # Sort by firing count, then resolved count (both descending), so that when the table + # is too long to fit in a message the rows that get dropped are the least significant + # ones rather than whichever groups happen to sort last alphabetically. The key is only + # a final tie-break, to keep the order stable between updates of the same message. + for key, value in sorted(summary_table.items(), key=lambda item: (-item[1][0], -item[1][1], item[0])): # key is a tuple of attribute names; value is a 2-element list with # the number of firing and resolved notifications. row = list(str(e) for e in chain(key, value)) diff --git a/tests/test_blocks.py b/tests/test_blocks.py index 93fc093b3..106cc8562 100644 --- a/tests/test_blocks.py +++ b/tests/test_blocks.py @@ -245,3 +245,25 @@ def omission_note(omitted): omitted_count, omitted_fired = int(match.group(1)), int(match.group(2)) assert omitted_fired == omitted_count # one "fired" per dropped row assert len(markdown) < BLOCK_SIZE_LIMIT + + +def test_empty_table_does_not_raise(): + # tabulate raises IndexError if maxcolwidths is passed with no rows, which also hit the + # row-dropping path when not even one row fits the size limit. + table_block = TableBlock(rows=[], headers=["label:site", "Fired", "Resolved"]) + + assert table_block.to_table_string() + markdown = table_block.to_markdown().text + assert markdown.startswith("```") and markdown.endswith("```") + + +def test_to_markdown_always_closes_code_fence(): + # Sweep sizes/widths that previously produced an unterminated fence (which made Slack + # render the whole table as non-monospace text) or a blind cut past the block limit. + for rows_n in (0, 1, 18, 200): + for width in (5, 80, 600): + rows = [[("v" * width) + str(i), "1", "0"] for i in range(rows_n)] + markdown = TableBlock(rows=rows, headers=["label:site", "Fired", "Resolved"]).to_markdown().text + assert markdown.count("```") % 2 == 0, (rows_n, width) + assert markdown.rstrip().endswith("```"), (rows_n, width) + assert len(markdown) < BLOCK_SIZE_LIMIT, (rows_n, width, len(markdown)) From 173436377625796d83350c3581d209d2f3f6d732 Mon Sep 17 00:00:00 2001 From: alonelish Date: Mon, 27 Jul 2026 09:09:34 +0300 Subject: [PATCH 7/8] ROB-3946 Attach the full summary table as a file instead of linking the UI The "View all N groups" platform link was misleading: the timeline's grouping is a fixed enum (ALERT_NAME/APP_NAME/CLUSTER/NAMESPACE/POD_NAME) and can't group by an arbitrary label, so the link could never show the same breakdown as the digest. Use the approach already taken for wide tables on regular findings (Transformer.tableblock_to_fileblocks) instead: when rows don't fit in the message, attach the complete table as a file rendered at width 250, where nothing wraps and every group is present. Slack files are immutable while the summary message is rewritten on every notification, so the file is managed rather than edited: - uploaded only when rows were actually dropped - refreshed at most once a minute, not once per notification - the superseded file is deleted whenever a new one is uploaded - the file id outlives the interval reset (the link does not), so the last file of an interval is still cleaned up by the next upload - an upload failure (e.g. no files:write scope) leaves the digest intact, just without the attachment The channel is now resolved before the upload, so an update that can't proceed no longer leaves an orphaned file behind. Co-Authored-By: Claude Opus 4.8 --- src/robusta/core/sinks/sink_base.py | 10 ++ src/robusta/core/sinks/slack/slack_sink.py | 1 + src/robusta/integrations/slack/sender.py | 106 +++++++++++++--- tests/test_slack_summary_attachment.py | 138 +++++++++++++++++++++ 4 files changed, 236 insertions(+), 19 deletions(-) create mode 100644 tests/test_slack_summary_attachment.py diff --git a/src/robusta/core/sinks/sink_base.py b/src/robusta/core/sinks/sink_base.py index 80fb6de66..c13d99e96 100644 --- a/src/robusta/core/sinks/sink_base.py +++ b/src/robusta/core/sinks/sink_base.py @@ -36,6 +36,11 @@ class NotificationSummary(BaseModel): start_ts: float = Field(default_factory=lambda: time.time()) # Timestamp of the first notification # Keys for the table are determined by grouping.notification_mode.summary.by summary_table: DefaultDict[KeyT, List[int]] = None + # The full summary table is attached as a file when it doesn't fit in the message. Files + # can't be edited in place, so the previous one is deleted whenever a new one is uploaded. + attachment_file_id: Optional[str] = None + attachment_permalink: Optional[str] = None + attachment_ts: float = 0 # when the attachment was last refreshed def register_notification(self, summary_key: KeyT, resolved: bool, interval: int): now_ts = time.time() @@ -45,6 +50,11 @@ def register_notification(self, summary_key: KeyT, resolved: bool, interval: int self.summary_table = defaultdict(lambda: [0, 0]) self.start_ts = now_ts self.message_id = None + # Drop the link so the new summary doesn't point at the previous interval's file, + # but keep the id so that whoever uploads next can still delete that file - the + # sink itself has no way to reach Slack. + self.attachment_permalink = None + self.attachment_ts = 0 self.summary_table[summary_key][idx] += 1 diff --git a/src/robusta/core/sinks/slack/slack_sink.py b/src/robusta/core/sinks/slack/slack_sink.py index ffb083899..1411d2b76 100644 --- a/src/robusta/core/sinks/slack/slack_sink.py +++ b/src/robusta/core/sinks/slack/slack_sink.py @@ -91,6 +91,7 @@ def handle_notification_grouping(self, finding: Finding, platform_enabled: bool) investigate_uri=investigate_uri, grouping_interval=self.params.grouping.interval, channel=resolved_channel, + summary_state=notification_summary, ) notification_summary.message_id = slack_thread_ts should_send_notification = self.params.grouping.notification_mode.summary.threaded diff --git a/src/robusta/integrations/slack/sender.py b/src/robusta/integrations/slack/sender.py index 09e701abf..db61bb34a 100644 --- a/src/robusta/integrations/slack/sender.py +++ b/src/robusta/integrations/slack/sender.py @@ -1,11 +1,12 @@ import copy import logging +import time import ssl import tempfile import re from datetime import datetime, timedelta from itertools import chain -from typing import Any, Dict, List, Optional, Set, Union +from typing import Any, Dict, List, Optional, Set, Tuple, Union import certifi import humanize from dateutil import tz @@ -56,6 +57,11 @@ ACTION_LINK = "link" SlackBlock = Dict[str, Any] MAX_BLOCK_CHARS = 3000 +# A file can hold a far wider table than a message can. +SUMMARY_ATTACHMENT_TABLE_WIDTH = 250 +# The summary message is rewritten on every notification; don't re-upload the attachment +# that often. +SUMMARY_ATTACHMENT_REFRESH_SECONDS = 60 MENTION_PATTERN = re.compile(r"<[^>]+>") @@ -861,16 +867,22 @@ def __send_finding_to_slack_preview( ) @staticmethod - def __summary_table_block(table_block: TableBlock) -> BaseBlock: + def __summary_table_block(table_block: TableBlock) -> Tuple[BaseBlock, int]: """Render the summary table, noting the totals of any rows dropped to fit the size limit. Without the residual counts the numbers in the table wouldn't add up to the notification count in the message header, making the summary look wrong. + + Returns the block and the number of rows that didn't fit. """ if len(table_block.headers) <= 2: - return table_block # rendered as a bullet list rather than a table, see __to_slack_table + return table_block, 0 # rendered as a bullet list rather than a table, see __to_slack_table + + omitted_count = 0 def omission_note(omitted_rows: List[List[str]]) -> str: + nonlocal omitted_count + omitted_count = len(omitted_rows) fired = resolved = 0 for row in omitted_rows: try: @@ -880,7 +892,51 @@ def omission_note(omitted_rows: List[List[str]]) -> str: return TableBlock.default_omission_note(omitted_rows) return f"... {len(omitted_rows)} more groups ({fired} fired, {resolved} resolved) not shown" - return table_block.to_markdown(omission_note=omission_note) + block = table_block.to_markdown(omission_note=omission_note) + return block, omitted_count + + def __refresh_summary_attachment(self, table_block: TableBlock, summary_state) -> Optional[str]: + """Attach the complete table as a file, since the message can only show part of it. + + Slack files are immutable, but the summary message is rewritten on every notification, so + a fresh file is uploaded (and the previous one removed) rather than edited. That is + throttled - re-uploading on every single notification would be wasteful. + """ + if summary_state is None: + return None + + now = time.time() + if summary_state.attachment_permalink and now - summary_state.attachment_ts < SUMMARY_ATTACHMENT_REFRESH_SECONDS: + return summary_state.attachment_permalink + + # A much wider table fits in a file than in a message, so nothing wraps there. + contents = table_block.to_table_string(table_max_width=SUMMARY_ATTACHMENT_TABLE_WIDTH) + file_block = FileBlock("alerts-summary.txt", contents.encode("utf-8")) + try: + resp = self.slack_client.files_upload_v2( + title=file_block.filename, + filename=file_block.filename, + content=file_block.contents, + ) + permalink = resp["file"]["permalink"] + new_file_id = resp["file"]["id"] + except Exception: + logging.exception("Failed to upload the full summary table to Slack") + return summary_state.attachment_permalink + + previous_file_id = summary_state.attachment_file_id + summary_state.attachment_file_id = new_file_id + summary_state.attachment_permalink = permalink + summary_state.attachment_ts = now + + if previous_file_id: + # Best effort - a leftover file is preferable to failing the summary update. + try: + self.slack_client.files_delete(file=previous_file_id) + except Exception as e: + logging.warning(f"Could not delete the superseded summary attachment: {e}") + + return permalink def send_or_update_summary_message( self, @@ -895,6 +951,7 @@ def send_or_update_summary_message( investigate_uri: str = None, grouping_interval: int = None, # in seconds channel: str = None, # pre-resolved channel (when channel_override uses labels/annotations) + summary_state=None, # NotificationSummary, for tracking the attachment across updates ): """Create or update a summary message with tabular information about the amount of events fired/resolved and a header describing the event group that this information concerns.""" @@ -950,27 +1007,17 @@ def send_or_update_summary_message( else: blocks.append(MarkdownBlock(text=cluster_txt)) + summary_block, omitted_count = self.__summary_table_block(table_block) blocks.extend( [ MarkdownBlock(f"*Matching criteria*: {group_by_criteria_str}"), MarkdownBlock(text=time_text), - self.__summary_table_block(table_block), + summary_block, ] ) - if platform_enabled and sink_params.investigate_link and investigate_uri: - # Rows may have been dropped to fit the message size limit, so always offer a way - # to see every group. Must be outside the table's code block - Slack doesn't - # render links inside one. - blocks.append(MarkdownBlock(text=f"<{investigate_uri}|View all {len(rows)} groups →>")) - - if threaded: - blocks.append(MarkdownBlock(text="See thread for individual alerts")) - - output_blocks = [] - for block in blocks: - output_blocks.extend(self.__to_slack(block, sink_params.name)) - + # Resolve the target before building the rest of the message, so that bailing out + # doesn't leave an uploaded attachment behind. if not channel: # Fallback: resolve channel without labels/annotations (only cluster_name will work) channel = ChannelTransformer.template( @@ -996,10 +1043,31 @@ def send_or_update_summary_message( method = self.slack_client.chat_postMessage kwargs = {} + attachment_permalink = None + if omitted_count: + # Too many groups to show inline - attach the complete table as a file. The link has + # to sit outside the table's code block, Slack doesn't render links inside one. + attachment_permalink = self.__refresh_summary_attachment(table_block, summary_state) + if attachment_permalink: + blocks.append(MarkdownBlock(text=f"<{attachment_permalink}|See all {len(rows)} groups> (attached)")) + + if threaded: + blocks.append(MarkdownBlock(text="See thread for individual alerts")) + + output_blocks = [] + for block in blocks: + output_blocks.extend(self.__to_slack(block, sink_params.name)) + + message_text = "Summary for: " + ", ".join(group_by_classification_header) + if attachment_permalink: + # Uploaded files have to be referenced from the message text, not just the blocks, + # for Slack to actually share them (see prepare_slack_text). + message_text = f"{message_text}\n<{attachment_permalink}|alerts-summary.txt>" + try: resp = method( channel=channel, - text="Summary for: " + ", ".join(group_by_classification_header), + text=message_text, blocks=output_blocks, display_as_bot=True, **kwargs, diff --git a/tests/test_slack_summary_attachment.py b/tests/test_slack_summary_attachment.py new file mode 100644 index 000000000..625a942bc --- /dev/null +++ b/tests/test_slack_summary_attachment.py @@ -0,0 +1,138 @@ +"""Tests for the file attachment on the Slack "Alerts Summary" digest (ROB-3946 / FRO-211). + +The digest can only show part of a large table, so the complete one is attached as a file. +Slack files are immutable while the message is rewritten on every notification, so these tests +pin down the upload/refresh/delete lifecycle. +""" +import time +from unittest.mock import MagicMock, patch + +import pytest + +from robusta.core.sinks.sink_base import NotificationSummary +from robusta.core.sinks.slack.slack_sink_params import SlackSinkParams + +SUMMARY_HEADER = ["label:site", "label:component"] +GROUP_HEADER = ["cluster: prod-nj1"] + + +@pytest.fixture +def slack(): + """A SlackSender wired to a mock Slack client, plus the recorded uploads/deletes.""" + with patch("robusta.integrations.slack.sender.WebClient") as web_client: + from robusta.integrations.slack.sender import SlackSender + + client = MagicMock() + uploads, deletes = [], [] + + def upload(**kwargs): + uploads.append(kwargs) + return {"file": {"permalink": f"https://files.slack.com/f{len(uploads)}", "id": f"F{len(uploads)}"}} + + client.files_upload_v2.side_effect = upload + client.files_delete.side_effect = lambda **kwargs: deletes.append(kwargs["file"]) + client.chat_postMessage.return_value = {"ts": "111.1", "channel": "C1"} + client.chat_update.return_value = {"ts": "111.1", "channel": "C1"} + web_client.return_value = client + + sender = SlackSender("xoxb-test", "account", "prod-nj1", "key", "chan", registry=None) + yield sender, client, uploads, deletes + + +def _table(n_groups): + return { + (f"ats.betting.betcatcher.validation.impl.Validator{i:03d}Foo", "nj"): [i % 7, i % 3] + for i in range(n_groups) + } + + +def _send(sender, summary_table, state, **kwargs): + return sender.send_or_update_summary_message( + GROUP_HEADER, + SUMMARY_HEADER, + summary_table, + SlackSinkParams(name="test", slack_channel="chan", api_key=""), + False, + time.time(), + False, + grouping_interval=86400, + channel="chan", + summary_state=state, + **kwargs, + ) + + +def test_no_attachment_when_the_table_fits(slack): + sender, client, uploads, _ = slack + + _send(sender, _table(1), NotificationSummary()) + + assert uploads == [] # nothing was dropped, so there is nothing to attach + assert "files.slack.com" not in str(client.chat_postMessage.call_args.kwargs["blocks"]) + + +def test_attaches_the_full_table_when_rows_are_dropped(slack): + sender, client, uploads, _ = slack + state = NotificationSummary() + + _send(sender, _table(200), state) + + assert len(uploads) == 1 + assert state.attachment_permalink and state.attachment_file_id + # Every group is in the file, even though the message only shows a fraction of them. + assert uploads[0]["content"].decode().count("Validator") == 200 + + kwargs = client.chat_postMessage.call_args.kwargs + assert any("files.slack.com" in str(block) for block in kwargs["blocks"]) + # Slack only shares an uploaded file if it is referenced from the message text too. + assert "files.slack.com" in kwargs["text"] + + +def test_attachment_is_throttled_across_updates(slack): + sender, _, uploads, _ = slack + state = NotificationSummary() + + _send(sender, _table(200), state) + sender.channel_name_to_id["chan"] = "C1" + _send(sender, _table(200), state, msg_ts="111.1") + + assert len(uploads) == 1 # the message is rewritten per notification, the file is not + + +def test_refreshing_the_attachment_deletes_the_previous_file(slack): + sender, _, uploads, deletes = slack + state = NotificationSummary() + + _send(sender, _table(200), state) + state.attachment_ts = 0 # pretend the throttle window elapsed + sender.channel_name_to_id["chan"] = "C1" + _send(sender, _table(200), state, msg_ts="111.1") + + assert len(uploads) == 2 + assert deletes == ["F1"] + + +def test_previous_interval_file_is_cleaned_up_after_reset(slack): + sender, _, uploads, deletes = slack + state = NotificationSummary() + _send(sender, _table(200), state) + + state.start_ts = time.time() - 999999 # force the interval to expire + state.register_notification(("x", "y"), False, 86400) + # The link is dropped so the new summary can't point at the old file, but the id is kept + # so the next upload can still delete it. + assert state.attachment_permalink is None + assert state.attachment_file_id == "F1" + + _send(sender, _table(200), state) + assert deletes == ["F1"] + + +def test_upload_failure_does_not_break_the_summary(slack): + sender, client, _, _ = slack + client.files_upload_v2.side_effect = Exception("no files:write scope") + + ts = _send(sender, _table(200), NotificationSummary()) + + assert ts == "111.1" # the digest is still posted, just without the attachment + assert "files.slack.com" not in str(client.chat_postMessage.call_args.kwargs["blocks"]) From 0bbf0afa279af13eb1ddb1365f9cd432c4a1e0a5 Mon Sep 17 00:00:00 2001 From: alonelish Date: Mon, 27 Jul 2026 09:13:12 +0300 Subject: [PATCH 8/8] ROB-3946 Address review: oversized table names and un-sortable summary keys - to_markdown reserved the code-fence envelope after capping the budget, so a very long table_name (~2990+ chars) still produced a block over the limit, which MarkdownBlock then cut - losing the closing fence, the exact failure this PR set out to fix. The name is now trimmed to fit, and a non-positive content budget yields an empty but properly closed block rather than an overflow marker that wouldn't fit either. - Summary group keys hold raw attribute values, so a key can contain None (e.g. "workload" for a finding with no service). Comparing None with str raised TypeError while sorting; keys are now normalised to strings first. This crashed on master too, where the sort compared keys unconditionally. Both found by review; verified against a 192-combination sweep of row counts, value widths, table-name lengths and max_chars values. Co-Authored-By: Claude Opus 4.8 --- src/robusta/core/reporting/blocks.py | 29 ++++++++++++++++-------- src/robusta/integrations/slack/sender.py | 10 ++++++-- tests/test_slack_summary_attachment.py | 15 ++++++++++++ 3 files changed, 43 insertions(+), 11 deletions(-) diff --git a/src/robusta/core/reporting/blocks.py b/src/robusta/core/reporting/blocks.py index edbd2fbf9..9c5b941ee 100644 --- a/src/robusta/core/reporting/blocks.py +++ b/src/robusta/core/reporting/blocks.py @@ -491,19 +491,30 @@ def __to_strings_rows(cls, rows): def to_markdown(self, max_chars=None, add_table_header: bool = True, omission_note=None) -> MarkdownBlock: """:param omission_note: builds the note shown when rows are dropped to fit the size limit. Receives the dropped rows, so callers can summarise what was left out (e.g. residual counts).""" - table_header = f"{self.table_name}\n" if self.table_name else "" - table_header = "" if not add_table_header else table_header - prefix = f"{table_header}```\n" - suffix = "\n```" # Stay under BLOCK_SIZE_LIMIT even when no caller-supplied limit: MarkdownBlock # would otherwise blindly cut the text and lose the closing ``` fence, which # makes the whole table render as unformatted (non-monospace) text. budget = BLOCK_SIZE_LIMIT - 1 if max_chars is None else min(max_chars, BLOCK_SIZE_LIMIT - 1) - table_contents = self.__render_within_budget( - budget - len(prefix) - len(suffix), - PRINTED_TABLE_MAX_WIDTH, - "presto", - omission_note or self.default_omission_note, + + table_header = f"{self.table_name}\n" if self.table_name and add_table_header else "" + opening_fence, suffix = "```\n", "\n```" + # The table name is caller-supplied and can be arbitrarily long, so trim it rather than + # let the envelope alone push the block past the limit and cost us the closing fence. + table_header = table_header[: max(0, budget - len(opening_fence) - len(suffix))] + prefix = f"{table_header}{opening_fence}" + + content_budget = budget - len(prefix) - len(suffix) + # With no room left for content, emit an empty (but properly closed) block rather than + # an overflow marker that wouldn't fit either. + table_contents = ( + self.__render_within_budget( + content_budget, + PRINTED_TABLE_MAX_WIDTH, + "presto", + omission_note or self.default_omission_note, + ) + if content_budget > 0 + else "" ) return MarkdownBlock(f"{prefix}{table_contents}{suffix}") diff --git a/src/robusta/integrations/slack/sender.py b/src/robusta/integrations/slack/sender.py index db61bb34a..a5198d089 100644 --- a/src/robusta/integrations/slack/sender.py +++ b/src/robusta/integrations/slack/sender.py @@ -960,8 +960,14 @@ def send_or_update_summary_message( # Sort by firing count, then resolved count (both descending), so that when the table # is too long to fit in a message the rows that get dropped are the least significant # ones rather than whichever groups happen to sort last alphabetically. The key is only - # a final tie-break, to keep the order stable between updates of the same message. - for key, value in sorted(summary_table.items(), key=lambda item: (-item[1][0], -item[1][1], item[0])): + # a final tie-break, to keep the order stable between updates of the same message. It is + # normalised to strings first: keys hold raw attribute values, which may mix None with + # strings (e.g. "workload" is None for a finding with no service) and aren't comparable. + def sort_key(item): + group_key, (fired, resolved) = item + return -fired, -resolved, tuple(str(value) for value in group_key) + + for key, value in sorted(summary_table.items(), key=sort_key): # key is a tuple of attribute names; value is a 2-element list with # the number of firing and resolved notifications. row = list(str(e) for e in chain(key, value)) diff --git a/tests/test_slack_summary_attachment.py b/tests/test_slack_summary_attachment.py index 625a942bc..23e2d8817 100644 --- a/tests/test_slack_summary_attachment.py +++ b/tests/test_slack_summary_attachment.py @@ -136,3 +136,18 @@ def test_upload_failure_does_not_break_the_summary(slack): assert ts == "111.1" # the digest is still posted, just without the attachment assert "files.slack.com" not in str(client.chat_postMessage.call_args.kwargs["blocks"]) + + +def test_summary_keys_mixing_none_and_strings_do_not_crash(slack): + # Group keys hold raw attribute values - "workload" is None when a finding has no service - + # and None is not comparable with str, so sorting must normalise before comparing. + sender, client, _, _ = slack + summary_table = { + ("ats.wallet.DefaultFundsAdjuster", "nj"): [5, 2], + (None, "nj"): [5, 2], # same counts, so the tie-break has to compare the keys + } + + ts = _send(sender, summary_table, NotificationSummary()) + + assert ts == "111.1" + assert "None" in str(client.chat_postMessage.call_args.kwargs["blocks"])