feat: Enable per-alert Incident Severity, metadata improvements #1970
Conversation
WalkthroughThe Incident.io sink is refactored to restructure metadata handling into a list of dictionaries, extends block type support via dynamic handling, introduces a private helper method for normalized block type naming, and conditionally includes source_url in payloads based on platform enablement. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/robusta/core/sinks/incidentio/incidentio_sink.py (1)
124-159: Consider using isinstance checks instead of string comparisons.The extended block handling is functional, but has maintainability concerns:
Fragile string comparisons (lines 133, 149): Checking
block_class == "EmptyFileBlock"andblock_class == "DividerBlock"by string matching is fragile—renaming or moving these classes breaks the logic silently.Inconsistent pattern: Some block types are identified via
hasattr()attribute checks, while others use string class name comparison, making the logic harder to follow.Apply this diff to use isinstance checks for EmptyFileBlock and DividerBlock:
+ # Import at top of file if not already present + from robusta.core.reporting.blocks import EmptyFileBlock, DividerBlock + else: # Handle additional block types dynamically - block_class = block.__class__.__name__ # FileBlock: has file_content attribute if hasattr(block, "file_content") and block.file_content: return block.file_content # EmptyFileBlock: just return a placeholder - elif block_class == "EmptyFileBlock": + elif isinstance(block, EmptyFileBlock): return "[Empty File]" # PrometheusBlock: has query results elif hasattr(block, "query") and hasattr(block, "series_data"): return f"Query: {block.query}\nResults: {len(block.series_data)} series" # ScanReportBlock: has scan results elif hasattr(block, "title") and hasattr(block, "score"): return f"Scan: {block.title}, Score: {block.score}" # CallbackBlock: has callback info elif hasattr(block, "action_name"): return f"Action: {block.action_name}" # DividerBlock: just a visual separator - elif block_class == "DividerBlock": + elif isinstance(block, DividerBlock): return "[Divider]"Note: Verify that EmptyFileBlock and DividerBlock are importable from the blocks module before applying this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/robusta/core/sinks/incidentio/incidentio_sink.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/robusta/core/sinks/incidentio/incidentio_sink.py (5)
src/robusta/core/sinks/incidentio/incidentio_client.py (2)
IncidentIoClient(4-18)request(12-18)src/robusta/core/sinks/incidentio/incidentio_sink_params.py (1)
IncidentioSinkParams(11-38)src/robusta/core/sinks/incidentio/incidentio_api.py (2)
AlertEventsApi(3-20)build_url(16-20)src/robusta/core/sinks/sink_base.py (1)
SinkBase(51-185)src/robusta/core/reporting/base.py (3)
BaseBlock(23-25)Finding(248-406)get_investigate_uri(318-341)
🔇 Additional comments (3)
src/robusta/core/sinks/incidentio/incidentio_sink.py (3)
102-108: LGTM!The helper method correctly extracts and normalizes block type names for metadata. The logic is clear and handles the "Block" suffix removal appropriately.
12-12: Verify removed imports are no longer referenced.Per the AI summary,
LinksBlock,Link, andLinkTypewere removed from the imports. Confirm these are not referenced elsewhere in this file or by consumers of this sink.
44-90: Verify metadata structure compatibility with Incident.io API.The review raises two concerns:
Metadata structure compatibility: According to Incident.io API documentation, the
metadataobject typically contains key/value pairs with alert data structured undermetadata.dataas an array of objects. The current code implementation usesadditional_infoas a sibling field inmetadatacontaining a list of{"type": ..., "content": ...}objects. Confirm that this structure aligns with how Incident.io processes the metadata field, or restructure to usemetadata.dataif required by the API.PR objectives mismatch: The PR description claims to add
severity_alert_label_name,severity_default,dashboard_url_annotation_name, andrunbook_url_annotation_nameconfiguration options, but none of this logic appears in the code changes shown. Either this functionality exists in other files not yet reviewed, or the implementation is incomplete relative to the stated objectives.
| logging.error( | ||
| f"Error sending alert to Incident.io: {response.status_code}, {response.text}" | ||
| ) | ||
| logging.error("Error sending alert to Incident.io: %s, %s", {response.status_code}, {response.text}) |
There was a problem hiding this comment.
Fix the logging statement - critical bug.
The logging arguments are wrapped in curly braces, which creates set literals in Python instead of passing the actual values. This will log useless output like {<HTTPStatus.NOT_FOUND: 404>} instead of the actual status code and response text.
Apply this diff to fix the logging:
- logging.error("Error sending alert to Incident.io: %s, %s", {response.status_code}, {response.text})
+ logging.error("Error sending alert to Incident.io: %s, %s", response.status_code, response.text)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logging.error("Error sending alert to Incident.io: %s, %s", {response.status_code}, {response.text}) | |
| logging.error("Error sending alert to Incident.io: %s, %s", response.status_code, response.text) |
🤖 Prompt for AI Agents
In src/robusta/core/sinks/incidentio/incidentio_sink.py around line 100, the
logging call uses curly braces which create set literals instead of passing
values; replace the arguments so the logger receives the actual values (e.g.
change to logging.error("Error sending alert to Incident.io: %s, %s",
response.status_code, response.text) or use an f-string like
logging.error(f"Error sending alert to Incident.io: {response.status_code},
{response.text}")).
arikalon1
left a comment
arikalon1
left a comment
There was a problem hiding this comment.
Thanks for the PR @igorhrcek
Left one small comment
| logging.error( | ||
| f"Error sending alert to Incident.io: {response.status_code}, {response.text}" | ||
| ) | ||
| logging.error("Error sending alert to Incident.io: %s, %s", {response.status_code}, {response.text}) |
There was a problem hiding this comment.
Looks like code rabbit is right, and this is indeed a bug
2 participants
Changes
This PR improves on existing Incident.io implementation and allows to define per-alert incident severity in Incident.io (until now severity was determined by the alert severity, however not all alerts with the critical severity should have the same Incident.io severity).
The following configurational changes were made:
severity_alert_label_name- label on the alert that will contain information about Incident.io severity. Default set torobusta_incidentio_severity.severity_default- sane default in case when label is not defineddashboard_url_annotation_name- Incident.io has something called thesource_url. Usually, that can be a link toward Grafana dashboard or anything else. In this case, it is a name of the alert annotation. Default set todashboard_url.runbook_url_annotation_name- same as with the Dashboard URL, stored as metadata information that can be extracted and parsed in Incident.io interface.metadata["additional_info"]for passing additional metadata information to Incident.ioPlease note that severity names must be mapped in the Incident.io interface (whatever you pass - minor, major, critical) to the actual severity ID.
These changes are running on our production for months without any issues.
Risks
None
Performance impact
None
Security impact
None
How to QA