Skip to content

Fix: comment field missing for host notification results in log views - #944

Open
PhilippLemke wants to merge 1 commit into
Checkmk:masterfrom
PhilippLemke:fix/host-notification-comment-field
Open

Fix: comment field missing for host notification results in log views#944
PhilippLemke wants to merge 1 commit into
Checkmk:masterfrom
PhilippLemke:fix/host-notification-comment-field

Conversation

@PhilippLemke

Copy link
Copy Markdown
Contributor

Summary

  • PainterLogComment.render() (cmk/gui/painter/v0/painters.py) decides whether a notification log line carries a trailing comment by checking len(parts) > 6 on the ;-split log_options string.
  • That threshold only matches the layout of service notification log lines, which carry an extra ;<service> segment (contact;host;service;state;plugin;output;comment = 7 fields with a comment).
  • Host notification log lines lack that segment (contact;host;state;plugin;output;comment = only 6 fields, even with a comment present), so they can never cross the > 6 bar.
  • Net effect: the "Comment" column in notification log views (e.g. WATO → Notifications → Analyse, or any GUI view/log table using the log_comment painter) is silently empty for every host notification, regardless of what the notification plugin wrote to stdout. Service notifications are unaffected.
  • Confirmed against the log-line producer, cmk.events.log_to_history._format_notification_message, which explicitly builds spec differently for host (spec = hostname) vs. service (spec = f"{hostname};{service}") — this is the source of the one-field difference.

Fix

Extracted the threshold into a small helper, _log_comment_min_fields(log_type), returning 6 for service log types and 5 for host log types (> 5 for host is the layout-correct equivalent of the old > 6 for service). PainterLogComment now also requests the log_type column (already used by the neighboring PainterLogPluginOutput) to tell host and service log lines apart.

Test plan

  • Added test_log_comment_field_count in tests/unit/cmk/gui/plugins/views/test_painters.py, parametrized over host/service × with/without comment, exercising the field-count logic directly.
  • python -m py_compile on both changed files.
  • Not run: full pytest/mypy (no local Bazel/hatch dev environment available in the sandbox this was authored in) — please let CI confirm.

PainterLogComment.render() used a hardcoded threshold of 6
";"-separated fields to decide whether a notification log line
carries a trailing comment. That threshold only matches the layout
of SERVICE notification log lines, which have an extra ";<service>"
segment (7 fields with a comment). HOST notification log lines lack
that segment, so even with a comment present they only ever reach 6
fields and never cross the "> 6" bar -- the comment column silently
stays empty for every host notification, regardless of what the
notification plugin wrote to stdout.

Extract the threshold into _log_comment_min_fields(log_type), which
returns 6 for service log types and 5 for host log types, and have
PainterLogComment request the log_type column (already used by the
neighboring PainterLogPluginOutput) to pick the right one.
@PhilippLemke

Copy link
Copy Markdown
Contributor Author

Manual validation on a real 2.4.0p35 environment

I ran this fix on a live 2.4.0p35 (CEE) installation to confirm it behaves correctly outside of the unit tests. Before the fix, HOST NOTIFICATION RESULT rows in the "Failed notifications" view had an empty "Comment" column even though the notification plugin wrote a trailing comment (SERVICE NOTIFICATION RESULT rows were unaffected). After applying the fix below, the host notification row correctly shows the comment.

Note for anyone backporting this to a 2.4.0 branch

⚠️ Don't copy the whole modified painters.py from this PR's branch onto a 2.4.0p35 (or similar stable) checkout — master's painters.py has since diverged in unrelated ways (e.g. from cmk.gui.color import render_color_icon, which doesn't exist as a module in 2.4.0p35 — that code lives at cmk.gui.graphing._color there instead). Dropping the whole file in breaks the site at import time with ModuleNotFoundError: No module named 'cmk.gui.color'.

Instead, apply only this targeted hunk to the 2.4.0p35 tree (verified against the real environment above):

--- a/cmk/gui/painter/v0/painters.py
+++ b/cmk/gui/painter/v0/painters.py
@@ -4955,6 +4955,19 @@
         return ("", row["log_options"])
 
 
+def _log_comment_min_fields(log_type: str) -> int:
+    """Minimum number of ";"-separated fields in a notification log line
+    before a trailing comment field is present.
+
+    Host notification log lines have one field fewer than service
+    notification log lines (they lack the ";<service>" segment), so the
+    threshold below which no comment field can be present differs between
+    the two. See cmk.events.log_to_history._format_notification_message,
+    which is the counterpart producing these log lines.
+    """
+    return 6 if "SERVICE" in log_type else 5
+
+
 class PainterLogComment(Painter):
     @property
     def ident(self) -> str:
@@ -4968,13 +4981,13 @@
 
     @property
     def columns(self) -> Sequence[ColumnName]:
-        return ["log_options"]
+        return ["log_options", "log_type"]
 
     def render(self, row: Row, cell: Cell) -> CellSpec:
         msg = row["log_options"]
         if ";" in msg:
             parts = msg.split(";")
-            if len(parts) > 6:
+            if len(parts) > _log_comment_min_fields(row.get("log_type", "")):
                 return ("", parts[-1])
         return ("", "")

(Line numbers above are for painter/v0/painters.py as shipped in 2.4.0p35.cee; adjust context if applying to a different point release.)


Disclosure: this PR (code, tests, and this comment) was authored with the assistance of Claude Code (Anthropic). All changes were reviewed and manually validated by me on a live 2.4.0p35 instance before submission.

@PhilippLemke

Copy link
Copy Markdown
Contributor Author

Before Patch

before_pr_944

After Patch

after_pr_944

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant