feat: add threaded message strategy for Slack notifier#5150
Open
santiagofn wants to merge 3 commits intoprometheus:mainfrom
Open
feat: add threaded message strategy for Slack notifier#5150santiagofn wants to merge 3 commits intoprometheus:mainfrom
santiagofn wants to merge 3 commits intoprometheus:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Slack message strategies ("new", "update", "thread"), validates strategy-specific configuration, introduces an API URL resolver, implements threaded messaging (summary-header and direct modes) with persistence and reactions, refactors notifier flow/types, and adds tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Notifier as Slack Notifier
participant NFL as nflog (store)
participant APIURL as API URL Resolver
participant Slack as Slack API
Notifier->>APIURL: Resolve endpoint for method (postMessage/update/reactions.add)
APIURL-->>Notifier: Return method-specific URL
Notifier->>NFL: Read stored thread_ts & channel
alt no stored parent
NFL-->>Notifier: none
Notifier->>Slack: POST chat.postMessage (create parent or direct message)
Slack-->>Notifier: {ok, ts, channel}
Notifier->>NFL: Persist thread_ts, channel, transitions
Notifier->>Slack: POST chat.postMessage (threaded reply using thread_ts) or done
Slack-->>Notifier: Success
else parent exists
NFL-->>Notifier: Return thread_ts & channel
Notifier->>Slack: POST chat.postMessage (threaded reply with thread_ts)
Slack-->>Notifier: Success
alt summary-header mode
Notifier->>Slack: POST chat.update (refresh parent summary)
Slack-->>Notifier: Success
Notifier->>NFL: Update transitions
end
end
alt resolution event
Notifier->>Slack: POST chat.postMessage (resolved threaded reply)
Slack-->>Notifier: Success
Notifier->>Slack: POST reactions.add (resolve emoji)
Slack-->>Notifier: Success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
santiagofn
force-pushed
the
feat/support-threaded-msgs-in-slack
branch
4 times, most recently
from
7ea82dd to
94e61c6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/config_test.go`:
- Around line 1084-1086: The test fixture file
conf.slack-update-message-and-webhook.yml used by the test case with name
"update strategy with webhook URL" should be updated to validate the new
message_strategy field: replace the deprecated update_message: true entry with
message_strategy: update (or add message_strategy: update alongside removing
update_message) so the test case (file referenced by the test struct field file
and strategy `update`) exercises the new configuration key.
In `@docs/configuration.md`:
- Around line 1695-1700: Change the heading level for
`<summary_header_thread_config>` from a fifth-level heading to a fourth-level
heading (replace `#####` with `####`) so it fits under the surrounding `###
<slack_config>` section and resolves the MD001/document outline error; update
the heading line that currently reads `##### <summary_header_thread_config>` to
`#### <summary_header_thread_config>` while leaving the following YAML block and
description unchanged.
In `@notify/slack/slack_thread.go`:
- Around line 65-82: The code currently disables store writes whenever threadTs
is found even if channelId is missing, causing new parents to be posted without
persisting repaired ids; update the logic in the slack thread flow (the block
that sets req.ThreadTimestamp/req.Channel, the storeForPost assignment, and the
post-handling/read-back) to require both tsFound && chFound before setting
req.ThreadTimestamp/req.Channel and before setting storeForPost = nil; otherwise
keep storeForPost = store so postAndHandle can persist missing ids, and after
the post use the same combined condition (if !(tsFound && chFound)) when calling
store.GetStr(storeKeyThreadTs) and store.GetStr(storeKeyChannelId) to reload
repaired values.
- Around line 151-153: The thread-summary path is sending untruncated titles
(summaryTitle) which can exceed Slack's title length and cause
chat.postMessage/chat.update failures; fix by applying the same truncation used
in notify/slack/slack.go to the summary header before sending—e.g., after
building summaryTitle via buildTransitionTitle(alertName, transitions) call,
truncate it to Slack's title max length (reuse the existing truncate utility or
the exact logic from slack.go lines 89-92) and then use the truncated value
(still keep summaryColor from tmplText(n.conf.Color)). Ensure you update the
code paths that set summaryTitle in slack_thread.go (references:
buildTransitionTitle, tmplText, summaryTitle, alertName) so thread messages and
updates use the clamped title.
- Around line 146-149: The code appends a separator even when reasonToTransition
returned an empty label, producing trailing pipes like "FIRING|"; change the
logic around transitions/previousTransitions/label so you only modify
transitions when label != "" (i.e., skip writes for non-transition reasons).
Specifically, in the block that computes transitions (using variables
transitions, previousTransitions, label), only set transitions =
previousTransitions + "|" + label if both previousTransitions != "" and label !=
""; if label != "" and previousTransitions == "" set transitions = label;
otherwise leave transitions as previousTransitions (or keep it unchanged) so no
extra separators are introduced.
In `@notify/slack/slack.go`:
- Around line 270-278: The current logic in the HasThreadStrategy branch only
checks existence of storeKeyThreadTs and skips updating when it exists, which
leaves partial/stale state (threadTs without channelId) unrecovered; change the
check to fetch both store.GetStr(storeKeyThreadTs) and
store.GetStr(storeKeyChannelId) and treat the thread state as missing if either
key is absent or empty, then call store.SetStr for both storeKeyThreadTs and
storeKeyChannelId with data.Timestamp and data.Channel to overwrite incomplete
state; update the n.logger.Debug messages accordingly (referencing
n.conf.HasThreadStrategy(), store.GetStr, store.SetStr, storeKeyThreadTs,
storeKeyChannelId, data.Timestamp, data.Channel).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9aea3c8b-2205-4d35-b0a4-221e2b039daa
📒 Files selected for processing (17)
.cursor/.gitignore.gitignoreconfig/config_test.goconfig/notifiers.goconfig/testdata/conf.slack-resolve-color-without-summary-header.ymlconfig/testdata/conf.slack-thread-message-and-webhook.ymlconfig/testdata/conf.slack-thread-resolve-emoji-without-thread-message.ymlconfig/testdata/conf.slack-thread-update-parent-without-thread-message.ymlconfig/testdata/conf.slack-update-message-and-webhook-with-new-field.ymldocs/configuration.mdnotify/slack/internal/apiurl/apiurl.gonotify/slack/internal/apiurl/apiurl_test.gonotify/slack/slack.gonotify/slack/slack_test.gonotify/slack/slack_thread.gonotify/slack/slack_thread_test.gonotify/slack/types.go
💤 Files with no reviewable changes (1)
- notify/slack/slack_test.go
santiagofn
force-pushed
the
feat/support-threaded-msgs-in-slack
branch
from
94e61c6 to
76f63f2
Compare
Signed-off-by: Santiago Fernández Núñez <santiago.nunez@nubank.com.br>
Signed-off-by: Santiago Fernández Núñez <santiago.nunez@nubank.com.br> Made-with: Cursor
santiagofn
force-pushed
the
feat/support-threaded-msgs-in-slack
branch
from
76f63f2 to
bebf220
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/notifiers.go`:
- Around line 645-654: The validation in the switch on c.MessageStrategy
(SlackMessageStrategyNew / SlackMessageStrategyUpdate /
SlackMessageStrategyThread) runs too early against c.APIURL and c.APIURLFile
before global defaults are applied; move or defer this URL/source validation
until after global/default merging or explicitly resolve/read both inline and
global sources first (the resolved slack API URL and its source) and then
enforce that for Update/Thread the resolved URL exists and, if it is an inline
URL, that it equals "https://slack.com/api/chat.postMessage"; update the logic
referencing c.APIURL, c.APIURLFile and c.MessageStrategy so it validates the
final resolved URL/source (not the raw receiver-level fields) to avoid rejecting
configs that inherit global.slack_api_url or use api_url_file that points to a
webhook.
In `@docs/configuration.md`:
- Around line 1665-1670: Update the documentation around message_strategy to
explicitly mention api_url_file alongside api_url: state that both api_url and
api_url_file must resolve to the exact URL
"https://slack.com/api/chat.postMessage" for the "update" and "thread"
strategies to work, and clarify that file-backed values (api_url_file) pointing
to incoming-webhook URLs will not enable update/thread behavior; refer to the
existing symbol names message_strategy, api_url, and api_url_file so the change
is added next to the current block describing "new", "update", and "thread".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e1f07e7-df31-4e20-9485-da7c1d11fa5d
📒 Files selected for processing (12)
config/config_test.goconfig/notifiers.goconfig/testdata/conf.slack-resolve-color-without-summary-header.ymlconfig/testdata/conf.slack-thread-message-and-webhook.ymlconfig/testdata/conf.slack-thread-resolve-emoji-without-thread-message.ymlconfig/testdata/conf.slack-thread-update-parent-without-thread-message.ymlconfig/testdata/conf.slack-update-message-and-webhook-with-new-field.ymldocs/configuration.mdnotify/slack/slack.gonotify/slack/slack_thread.gonotify/slack/slack_thread_test.gonotify/slack/types.go
✅ Files skipped from review due to trivial changes (4)
- config/testdata/conf.slack-resolve-color-without-summary-header.yml
- config/testdata/conf.slack-thread-update-parent-without-thread-message.yml
- config/testdata/conf.slack-thread-resolve-emoji-without-thread-message.yml
- config/config_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- notify/slack/types.go
- notify/slack/slack_thread_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
notify/slack/slack_thread.go (1)
262-266: Dead code: unreachable length check.
strings.Splitnever returns an empty slice—it returns at least[""]for an empty input. The check at lines 264-266 is unreachable. The correct handling is already at lines 284-286 wherelen(collapsed) == 0catches the empty/whitespace-only case after filtering.🧹 Suggested cleanup
func buildTransitionTitle(transitions, alertName string) string { parts := strings.Split(transitions, "|") - if len(parts) == 0 { - return alertName - } type entry struct {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notify/slack/slack_thread.go` around lines 262 - 266, In buildTransitionTitle, remove the unreachable guard that checks len(parts) == 0 after calling strings.Split(transitions, "|") (the check around parts) because strings.Split never returns an empty slice; rely on the existing collapsed/filtered handling later in the function (the collapsed empty check) to handle empty or whitespace-only inputs instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@notify/slack/slack_thread.go`:
- Around line 262-266: In buildTransitionTitle, remove the unreachable guard
that checks len(parts) == 0 after calling strings.Split(transitions, "|") (the
check around parts) because strings.Split never returns an empty slice; rely on
the existing collapsed/filtered handling later in the function (the collapsed
empty check) to handle empty or whitespace-only inputs instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7577f6ce-b985-4d63-bdc2-e32fe4b799cf
📒 Files selected for processing (17)
.cursor/.gitignore.gitignoreconfig/config_test.goconfig/notifiers.goconfig/testdata/conf.slack-resolve-color-without-summary-header.ymlconfig/testdata/conf.slack-thread-message-and-webhook.ymlconfig/testdata/conf.slack-thread-resolve-emoji-without-thread-message.ymlconfig/testdata/conf.slack-thread-update-parent-without-thread-message.ymlconfig/testdata/conf.slack-update-message-and-webhook-with-new-field.ymldocs/configuration.mdnotify/slack/internal/apiurl/apiurl.gonotify/slack/internal/apiurl/apiurl_test.gonotify/slack/slack.gonotify/slack/slack_test.gonotify/slack/slack_thread.gonotify/slack/slack_thread_test.gonotify/slack/types.go
💤 Files with no reviewable changes (1)
- notify/slack/slack_test.go
✅ Files skipped from review due to trivial changes (7)
- .cursor/.gitignore
- .gitignore
- config/testdata/conf.slack-resolve-color-without-summary-header.yml
- config/testdata/conf.slack-thread-resolve-emoji-without-thread-message.yml
- config/testdata/conf.slack-thread-update-parent-without-thread-message.yml
- config/config_test.go
- notify/slack/slack_thread_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- notify/slack/internal/apiurl/apiurl.go
- notify/slack/slack.go
santiagofn
force-pushed
the
feat/support-threaded-msgs-in-slack
branch
2 times, most recently
from
f66b6ff to
a855ed5
Compare
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds Slack delivery strategies ("new", "update", "thread"), validates strategy-specific config, introduces an API URL resolver, implements threaded messaging (summary-header and direct modes) with persistence and reactions, refactors notifier flow/types, and adds extensive tests and documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Notifier as Slack Notifier
participant NFL as nflog (store)
participant APIURL as API URL Resolver
participant Slack as Slack API
Notifier->>APIURL: Resolve endpoint for method (postMessage/update/reactions.add)
APIURL-->>Notifier: Return method-specific URL
Notifier->>NFL: Read stored thread_ts & channel
alt first notification (no parent)
NFL-->>Notifier: none
Notifier->>Slack: POST chat.postMessage (parent if summary or parent message)
Slack-->>Notifier: {ok, ts, channel}
Notifier->>NFL: Persist thread_ts, channel, transitions
Notifier->>Slack: POST chat.postMessage (threaded reply using thread_ts) or done if direct parent
Slack-->>Notifier: Success
else subsequent notification (parent exists)
NFL-->>Notifier: Return thread_ts & channel
Notifier->>Slack: POST chat.postMessage (threaded reply with thread_ts)
Slack-->>Notifier: Success
alt summary-header mode
Notifier->>Slack: POST chat.update (refresh parent summary)
Slack-->>Notifier: Success
Notifier->>NFL: Update transitions
end
end
alt resolution event
Notifier->>Slack: POST chat.postMessage (resolved threaded reply)
Slack-->>Notifier: Success
Notifier->>Slack: POST reactions.add (resolve emoji)
Slack-->>Notifier: Success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
santiagofn
force-pushed
the
feat/support-threaded-msgs-in-slack
branch
3 times, most recently
from
c0e0526 to
2db9624
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
notify/slack/slack.go (1)
53-60:⚠️ Potential issue | 🔴 CriticalPreserve
app_urlas a request URL source.
Newnow seedsurlResolveronly fromc.APIURL/c.APIURLFile, butNotifyalways asks that resolver for the initial send URL. That drops the existingapp_token/app_urlpath: configs that only setapp_urlno longer have a base URL and will fail before the first POST. Please keepc.AppURLas a first-class fallback for the initial send path.Also applies to: 171-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notify/slack/slack.go` around lines 53 - 60, The urlResolver is being created from only c.APIURL / c.APIURLFile which drops configs that set only c.AppURL; update the Notifier construction to seed apiurl.NewResolver with c.AppURL as the primary fallback (e.g., include c.AppURL along with c.APIURL and c.APIURLFile when creating urlResolver) so the initial send URL can come from AppURL, and apply the same change where urlResolver is constructed/used in Notify (the code around the Notify method at the 171-174 area) to preserve the app_url/app_token path.
♻️ Duplicate comments (1)
config/notifiers.go (1)
664-675:⚠️ Potential issue | 🟠 MajorValidate
api_url_fileagainstchat.postMessagetoo.This only rejects webhook-style inline
api_url. A receiver withmessage_strategy: updateorthreadandapi_url_filepointing at an incoming-webhook URL still loads, but runtime then either derives invalidchat.update/reactions.addendpoints or silently falls back to plain posts because webhook responses do not returntsandchannel. Please validate the resolved file-backed value againsthttps://slack.com/api/chat.postMessageas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/notifiers.go` around lines 664 - 675, In ValidateMessageStrategy, extend the check that currently enforces c.APIURL to also validate the resolved value of c.APIURLFile when MessageStrategy is SlackMessageStrategyUpdate or SlackMessageStrategyThread: if c.APIURLFile is set, read and parse its contents into a URL (similar to how c.APIURL is represented) and ensure its String() equals "https://slack.com/api/chat.postMessage"; otherwise return the same error used for c.APIURL (message_strategy %q requires a bot token; api_url must be https://slack.com/api/chat.postMessage). This update should live inside the existing switch in ValidateMessageStrategy and reference the APIURL and APIURLFile fields and the SlackMessageStrategyUpdate/SlackMessageStrategyThread constants.
🧹 Nitpick comments (1)
notify/slack/slack_thread_test.go (1)
765-781: Usedefer mu.Unlock()to prevent deadlock on assertion failure.If any
requireassertion fails betweenmu.Lock()andmu.Unlock(), the mutex remains locked. While test termination typically cleans this up, it's better practice to usedeferfor robustness and consistency.♻️ Suggested improvement
mu.Lock() + defer mu.Unlock() require.Len(t, calls, 2, "step 1: expect 2 calls (summary parent + reply)") var parentReq request require.NoError(t, json.Unmarshal(calls[0].Body, &parentReq)) // ... assertions ... t.Logf("step 1: parent summary + reply posted, parent ts=%s", parentTs) - mu.Unlock()Apply similar pattern to the other locked sections at lines 792-802 and 821-840.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notify/slack/slack_thread_test.go` around lines 765 - 781, The test holds a mutex with mu.Lock() but calls mu.Unlock() explicitly after many require assertions, which can deadlock if an assertion fails; change each critical section by calling mu.Lock() immediately followed by defer mu.Unlock() (e.g., around the block that checks calls, unmarshals into parentReq/firstReplyReq, and asserts parentTs/transitions) so the mutex is always released on test failure, and apply the same defer mu.Unlock() pattern to the other locked sections that use mu.Lock()/mu.Unlock() in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@notify/slack/slack_thread_test.go`:
- Around line 695-721: The HTTP handler uses require.NoError(t, err) inside the
goroutine — change this to capture the error into a shared variable (e.g.
declare var handlerErr error above the httptest.NewServer, then inside the
http.HandlerFunc set handlerErr = err while holding mu instead of calling
require) and continue handling; after you call server.Close() in the test,
acquire mu and assert require.NoError(t, handlerErr, "HTTP handler encountered
an error"). Update references around io.ReadAll, the http.HandlerFunc, and the
capturedCall/calls slice handling to use mu when writing handlerErr.
---
Outside diff comments:
In `@notify/slack/slack.go`:
- Around line 53-60: The urlResolver is being created from only c.APIURL /
c.APIURLFile which drops configs that set only c.AppURL; update the Notifier
construction to seed apiurl.NewResolver with c.AppURL as the primary fallback
(e.g., include c.AppURL along with c.APIURL and c.APIURLFile when creating
urlResolver) so the initial send URL can come from AppURL, and apply the same
change where urlResolver is constructed/used in Notify (the code around the
Notify method at the 171-174 area) to preserve the app_url/app_token path.
---
Duplicate comments:
In `@config/notifiers.go`:
- Around line 664-675: In ValidateMessageStrategy, extend the check that
currently enforces c.APIURL to also validate the resolved value of c.APIURLFile
when MessageStrategy is SlackMessageStrategyUpdate or
SlackMessageStrategyThread: if c.APIURLFile is set, read and parse its contents
into a URL (similar to how c.APIURL is represented) and ensure its String()
equals "https://slack.com/api/chat.postMessage"; otherwise return the same error
used for c.APIURL (message_strategy %q requires a bot token; api_url must be
https://slack.com/api/chat.postMessage). This update should live inside the
existing switch in ValidateMessageStrategy and reference the APIURL and
APIURLFile fields and the SlackMessageStrategyUpdate/SlackMessageStrategyThread
constants.
---
Nitpick comments:
In `@notify/slack/slack_thread_test.go`:
- Around line 765-781: The test holds a mutex with mu.Lock() but calls
mu.Unlock() explicitly after many require assertions, which can deadlock if an
assertion fails; change each critical section by calling mu.Lock() immediately
followed by defer mu.Unlock() (e.g., around the block that checks calls,
unmarshals into parentReq/firstReplyReq, and asserts parentTs/transitions) so
the mutex is always released on test failure, and apply the same defer
mu.Unlock() pattern to the other locked sections that use mu.Lock()/mu.Unlock()
in this file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b21e3110-2c35-4d1f-8004-cf37a344ce37
📒 Files selected for processing (16)
config/config.goconfig/config_test.goconfig/notifiers.goconfig/testdata/conf.slack-resolve-color-without-summary-header.ymlconfig/testdata/conf.slack-thread-message-and-webhook.ymlconfig/testdata/conf.slack-thread-resolve-emoji-without-thread-message.ymlconfig/testdata/conf.slack-thread-strategy-with-global-api-url.ymlconfig/testdata/conf.slack-thread-strategy-without-api-url.ymlconfig/testdata/conf.slack-thread-update-parent-without-thread-message.ymlconfig/testdata/conf.slack-update-message-and-webhook-with-new-field.ymldocs/configuration.mdnotify/slack/internal/apiurl/apiurl.gonotify/slack/slack.gonotify/slack/slack_thread.gonotify/slack/slack_thread_test.gonotify/slack/types.go
✅ Files skipped from review due to trivial changes (6)
- config/config.go
- config/testdata/conf.slack-thread-strategy-with-global-api-url.yml
- config/testdata/conf.slack-thread-update-parent-without-thread-message.yml
- config/testdata/conf.slack-thread-strategy-without-api-url.yml
- config/testdata/conf.slack-resolve-color-without-summary-header.yml
- config/testdata/conf.slack-thread-resolve-emoji-without-thread-message.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- config/config_test.go
Signed-off-by: Santiago Fernández Núñez <santiago.nunez@nubank.com.br> Made-with: Cursor
santiagofn
force-pushed
the
feat/support-threaded-msgs-in-slack
branch
from
2db9624 to
8eb198f
Compare
Author
|
Regarding 2 comments from CodeRabbit that I didn't address:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces a new
message_strategyfield for the Slack notifier, replacing and deprecating the existingupdate_messageboolean (while keeping backward compatibility). The new field accepts three values:new: default, current behaviorupdate: equivalent to the oldupdate_message: truethread: allows subsequent notifications for the same alert group to be posted as threaded replies instead of standalone messages. This reduces channel noise and keeps related alert state changes grouped together.Builds on the arbitrary key/value data store added to the nflog in #4899, using it to persist
threadTs,channelId, andtransitionsacross restarts and HA cluster nodes.Relates to #3221.
Commits
The changes are split into three commits for easier review:
threadmessage strategy with both modes, resolve actions, transition tracking, config validation, and comprehensive tests.The
threadstrategyThreading modes
Controlled by
threaded_options.use_summary_header:true(default) — The thread parent is a lightweight, auto-updated summary showing the alert group's transition history (e.g. "FIRING (2) → SILENCED → RESOLVED"). All alert content is posted as replies. The parent's color and title update on every notification cycle.false— The first alert message itself becomes the thread parent. Subsequent alerts are threaded replies. No parent updates are performed.Advanced
threaded_optionsresolve_emoji— Optional emoji reaction on the thread parent when all alerts resolve (requiresreactions:writescope).summary_header.resolve_color— Optional color override for the parent summary attachment on resolution (only valid whenuse_summary_headeris enabled).Configuration example
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
Chores