Skip to content

Conversation

nikhilsinhaparseable
Copy link
Contributor

@nikhilsinhaparseable nikhilsinhaparseable commented Sep 24, 2025

duplicate entries in message -

  1. AlertName
  2. Triggered TimeStamp
  3. Severity

Summary by CodeRabbit

  • Bug Fixes
    • Webhook alert messages (Slack and generic webhooks) now use the alert’s message content instead of an auto-generated string, aligning notifications with what you see in the app.
    • Removes extra prefixes (e.g., timestamps/severity) from payloads for clearer, consistent notifications across channels.
    • No configuration or integration changes required.

duplicate entries in message -
1. AlertName
2. Triggered TimeStamp
3. Severity
Copy link
Contributor

coderabbitai bot commented Sep 24, 2025

Walkthrough

Removed Context::default_alert_string from alert_structs and updated webhook targets to use payload.message directly for Triggered alerts. This changes the exact message content sent to Slack and other webhooks without altering control flow or public APIs.

Changes

Cohort / File(s) Summary
Alert struct cleanup
src/alerts/alert_structs.rs
Removed Context::default_alert_string(&self) -> String, which previously formatted a timestamped, severity-tagged alert string.
Webhook payload adjustment
src/alerts/target.rs
For Triggered alerts, replaced usage of default_alert_string() with payload.message (and .clone() where needed) for SlackWebHook and OtherWebHook payloads.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant A as Alert Producer
  participant C as Context
  participant P as Payload
  participant W as Webhook (Slack/Other)

  A->>C: Build Context
  A->>P: Create Payload (message)
  note over P: message now used directly
  A->>W: Send Triggered alert with P.message
  W-->>A: Response (success/error)
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I thump my paws—a tidy tweak!
The timestamps hop away this week.
Now messages leap, clear and bright,
Straight to the webhooks’ moonlit night.
Fewer frills, a cleaner track—
Alerting swift as a rabbit’s dash. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description only lists that there are duplicate entries (AlertName, Triggered TimeStamp, Severity) and does not follow the repository template: it lacks a Fixes # line, a clear description of the implemented changes and rationale, which files/functions were modified, and the checklist or testing information. This level of detail is insufficient for reviewers to understand the exact code changes and how the fix was validated. Please expand the description to include the Fixes # issue reference if applicable and a concise summary of the chosen solution, listing the files and functions changed and why payload.message replaces the previous default string. Add what testing was performed and update the checklist items (tests, comments, docs) or explain omissions. Also include an example showing the previous vs new alert message so reviewers can quickly verify that duplicates were removed.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "fix: remove redundant part from alert notification message" succinctly and accurately describes the primary change to alert messages and matches the diffs that remove duplicated alert fields from the payload. It is concise, specific to the change, and appropriate for a teammate scanning history to understand the intent.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 61eb9f9 and 305a6a0.

📒 Files selected for processing (2)
  • src/alerts/alert_structs.rs (0 hunks)
  • src/alerts/target.rs (2 hunks)
💤 Files with no reviewable changes (1)
  • src/alerts/alert_structs.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: Build Default x86_64-pc-windows-msvc
  • GitHub Check: Build Default x86_64-unknown-linux-gnu
  • GitHub Check: Build Default aarch64-unknown-linux-gnu
  • GitHub Check: Quest Smoke and Load Tests for Standalone deployments
  • GitHub Check: Build Kafka x86_64-unknown-linux-gnu
  • GitHub Check: Build Default x86_64-apple-darwin
  • GitHub Check: Build Default aarch64-apple-darwin
  • GitHub Check: Quest Smoke and Load Tests for Distributed deployments
  • GitHub Check: Build Kafka aarch64-apple-darwin
  • GitHub Check: coverage
🔇 Additional comments (2)
src/alerts/target.rs (2)

478-482: Forward raw message for Triggered to generic webhooks — LGTM

Change is consistent with Slack behavior and should remove duplicates for generic webhooks, too. If downstream expects JSON, ensure headers/content-type are set accordingly.

Please confirm downstream consumers accept text/plain bodies for Triggered alerts. If not, we can switch to JSON and set Content-Type explicitly.


438-444: Use payload.message for Triggered Slack alerts — LGTM

Approves change; aligns with PR goal and removes duplicated Slack fields. Ripgrep output provided was empty — cannot confirm absence of the removed default formatter; run locally and paste results:

rg -n --hidden --no-ignore '\.default_[A-Za-z0-9_]+\s*\(' || true
rg -nP '(default_alert_string\s*\()|(\.default_alert_string\s*\(\))' || true

Comment @coderabbitai help to get the list of available commands and usage tips.

@nitisht nitisht merged commit 5364dc3 into parseablehq:main Sep 24, 2025
13 checks passed
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.

2 participants