Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion notify/discord/discord.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)

alerts := types.Alerts(as...)
data := notify.GetTemplateData(ctx, n.tmpl, as, logger)
tmpl := notify.TmplText(n.tmpl, data, &err)
tmpl := notify.TmplTextWithLogger(n.tmpl, data, &err, logger)
if err != nil {
return false, err
}
Expand Down
2 changes: 1 addition & 1 deletion notify/email/email.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ func (n *Email) Notify(ctx context.Context, as ...*types.Alert) (bool, error) {
var (
tmplErr error
data = notify.GetTemplateData(ctx, n.tmpl, as, n.logger)
tmpl = notify.TmplText(n.tmpl, data, &tmplErr)
tmpl = notify.TmplTextWithLogger(n.tmpl, data, &tmplErr, n.logger)
)
from := tmpl(n.conf.From)
if tmplErr != nil {
Expand Down
2 changes: 1 addition & 1 deletion notify/jira/jira.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)

tmplTextErr error
data = notify.GetTemplateData(ctx, n.tmpl, as, logger)
tmplText = notify.TmplText(n.tmpl, data, &tmplTextErr)
tmplText = notify.TmplTextWithLogger(n.tmpl, data, &tmplTextErr, logger)
tmplTextFunc = func(tmpl string) (string, error) {
return tmplText(tmpl), tmplTextErr
}
Expand Down
3 changes: 2 additions & 1 deletion notify/mattermost/mattermost.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ func (n *Notifier) Notify(ctx context.Context, alert ...*types.Alert) (bool, err
return false, errors.New("webhook url missing")
}

req := n.createRequest(notify.TmplText(n.tmpl, data, &err))
tmplText := notify.TmplTextWithLogger(n.tmpl, data, &err, n.logger)
req := n.createRequest(tmplText)
if err != nil {
return false, err
}
Expand Down
2 changes: 1 addition & 1 deletion notify/msteams/msteams.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
logger.Debug("extracted group key")

data := notify.GetTemplateData(ctx, n.tmpl, as, logger)
tmpl := notify.TmplText(n.tmpl, data, &err)
tmpl := notify.TmplTextWithLogger(n.tmpl, data, &err, logger)
if err != nil {
return false, err
}
Expand Down
2 changes: 1 addition & 1 deletion notify/msteamsv2/msteamsv2.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
logger.Debug("extracted group key")

data := notify.GetTemplateData(ctx, n.tmpl, as, logger)
tmpl := notify.TmplText(n.tmpl, data, &err)
tmpl := notify.TmplTextWithLogger(n.tmpl, data, &err, logger)
if err != nil {
return false, err
}
Expand Down
2 changes: 1 addition & 1 deletion notify/opsgenie/opsgenie.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ func (n *Notifier) createRequests(ctx context.Context, as ...*types.Alert) ([]*h

data := notify.GetTemplateData(ctx, n.tmpl, as, logger)

tmpl := notify.TmplText(n.tmpl, data, &err)
tmpl := notify.TmplTextWithLogger(n.tmpl, data, &err, logger)

details := make(map[string]string)

Expand Down
6 changes: 3 additions & 3 deletions notify/pagerduty/pagerduty.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ func (n *Notifier) notifyV1(
details map[string]any,
) (bool, error) {
var tmplErr error
tmpl := notify.TmplText(n.tmpl, data, &tmplErr)
tmpl := notify.TmplTextWithLogger(n.tmpl, data, &tmplErr, n.logger)

description, truncated := notify.TruncateInRunes(tmpl(n.conf.Description), maxV1DescriptionLenRunes)
if truncated {
Expand Down Expand Up @@ -211,7 +211,7 @@ func (n *Notifier) notifyV2(
details map[string]any,
) (bool, error) {
var tmplErr error
tmpl := notify.TmplText(n.tmpl, data, &tmplErr)
tmpl := notify.TmplTextWithLogger(n.tmpl, data, &tmplErr, n.logger)

if n.conf.Severity == "" {
n.conf.Severity = "error"
Expand Down Expand Up @@ -367,7 +367,7 @@ func (n *Notifier) renderDetails(
) (map[string]any, error) {
var (
tmplTextErr error
tmplText = notify.TmplText(n.tmpl, data, &tmplTextErr)
tmplText = notify.TmplTextWithLogger(n.tmpl, data, &tmplTextErr, n.logger)
tmplTextFunc = func(tmpl string) (string, error) {
return tmplText(tmpl), tmplTextErr
}
Expand Down
3 changes: 2 additions & 1 deletion notify/pagerduty/pagerduty_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,8 @@ func TestRenderDetails(t *testing.T) {
conf: &config.PagerdutyConfig{
Details: tt.args.details,
},
tmpl: test.CreateTmpl(t),
tmpl: test.CreateTmpl(t),
logger: promslog.NewNopLogger(),
}
got, err := n.renderDetails(tt.args.data)
if (err != nil) != tt.wantErr {
Expand Down
4 changes: 2 additions & 2 deletions notify/pushover/pushover.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
err error
message string
)
tmpl := notify.TmplText(n.tmpl, data, &err)
tmplHTML := notify.TmplHTML(n.tmpl, data, &err)
tmpl := notify.TmplTextWithLogger(n.tmpl, data, &err, logger)
tmplHTML := notify.TmplHTMLWithLogger(n.tmpl, data, &err, logger)

var (
token string
Expand Down
2 changes: 1 addition & 1 deletion notify/rocketchat/rocketchat.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
logger.Debug("extracted group key")

data := notify.GetTemplateData(ctx, n.tmpl, as, logger)
tmplText := notify.TmplText(n.tmpl, data, &err)
tmplText := notify.TmplTextWithLogger(n.tmpl, data, &err, logger)
if err != nil {
return false, err
}
Expand Down
2 changes: 1 addition & 1 deletion notify/slack/slack.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)

var (
data = notify.GetTemplateData(ctx, n.tmpl, as, logger)
tmplText = notify.TmplText(n.tmpl, data, &err)
tmplText = notify.TmplTextWithLogger(n.tmpl, data, &err, logger)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Return template errors before sending the Slack request.

TmplTextWithLogger records failures in err, but Notify never checks err after rendering the request fields. Add an if err != nil { return false, err } check after the final template call and before JSON encoding/network delivery; otherwise a failure short-circuits subsequent fields to empty strings while the malformed request is still sent.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@notify/slack/slack.go` at line 68, In Notify, check err immediately after the
final TmplTextWithLogger call and before JSON encoding or Slack delivery; return
false and err when rendering fails, while preserving the existing request flow
when err is nil.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a pre-existing issue, but would be nice to adress @mihir-dixit2k27

)
var markdownIn []string

Expand Down
2 changes: 1 addition & 1 deletion notify/sns/sns.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func (n *Notifier) Notify(ctx context.Context, alert ...*types.Alert) (bool, err
var (
tmplErr error
data = notify.GetTemplateData(ctx, n.tmpl, alert, n.logger)
tmpl = notify.TmplText(n.tmpl, data, &tmplErr)
tmpl = notify.TmplTextWithLogger(n.tmpl, data, &tmplErr, n.logger)
)

client, err := n.createSNSClient(ctx, tmpl, &tmplErr)
Expand Down
4 changes: 2 additions & 2 deletions notify/telegram/telegram.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,14 @@ func (n *Notifier) Notify(ctx context.Context, alert ...*types.Alert) (bool, err
var (
err error
data = notify.GetTemplateData(ctx, n.tmpl, alert, logger)
tmpl = notify.TmplText(n.tmpl, data, &err)
tmpl = notify.TmplTextWithLogger(n.tmpl, data, &err, logger)
messageText string
truncated bool
)

switch n.conf.ParseMode {
case "HTML":
tmpl = notify.TmplHTML(n.tmpl, data, &err)
tmpl = notify.TmplHTMLWithLogger(n.tmpl, data, &err, logger)
messageText = tmpl(n.conf.Message)
if err != nil {
return false, err
Expand Down
38 changes: 38 additions & 0 deletions notify/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,44 @@ func TmplHTML(tmpl *template.Template, data *template.Data, err *error) func(str
}
}

// TmplTextWithLogger is like TmplText but additionally logs a warning whenever
// a template fails to render. This makes template errors visible in Alertmanager
// logs even when the calling notifier continues to deliver the notification with
// the partially rendered output.
func TmplTextWithLogger(tmpl *template.Template, data *template.Data, err *error, logger *slog.Logger) func(string) string {
return func(name string) (s string) {
if *err != nil {
return s
}
var tmplErr error
s, tmplErr = tmpl.ExecuteTextString(name, data)
if tmplErr != nil {
logger.Warn("template execution failed", "template", name, "err", tmplErr)
*err = tmplErr
}
return s
}
}

// TmplHTMLWithLogger is like TmplHTML but additionally logs a warning whenever
// a template fails to render. This makes template errors visible in Alertmanager
// logs even when the calling notifier continues to deliver the notification with
// the partially rendered output.
func TmplHTMLWithLogger(tmpl *template.Template, data *template.Data, err *error, logger *slog.Logger) func(string) string {
return func(name string) (s string) {
if *err != nil {
return s
}
var tmplErr error
s, tmplErr = tmpl.ExecuteHTMLString(name, data)
if tmplErr != nil {
logger.Warn("template execution failed", "template", name, "err", tmplErr)
*err = tmplErr
}
return s
}
}

// Key is a string that can be hashed.
type Key string

Expand Down
44 changes: 44 additions & 0 deletions notify/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -273,3 +273,47 @@ func TestGetFailureReasonFromStatusCode(t *testing.T) {
})
}
}

func TestTmplTextWithLogger_LogsOnError(t *testing.T) {
tmpl, err := template.FromGlobs([]string{})
require.NoError(t, err)

data := &template.Data{}
var tmplErr error

var buf bytes.Buffer
logger := promslog.New(&promslog.Config{Writer: &buf})

tmplFn := TmplTextWithLogger(tmpl, data, &tmplErr, logger)

// An unclosed action causes a parse/execute error.
result := tmplFn("{{ .Missing")

require.Error(t, tmplErr)
require.Empty(t, result)
require.Contains(t, buf.String(), "template execution failed")
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

// Subsequent calls must short-circuit without logging again.
buf.Reset()
_ = tmplFn("{{ .AnotherField }}")
require.Empty(t, buf.String(), "no second log expected after first error")
}

func TestTmplHTMLWithLogger_LogsOnError(t *testing.T) {
tmpl, err := template.FromGlobs([]string{})
require.NoError(t, err)

data := &template.Data{}
var tmplErr error

var buf bytes.Buffer
logger := promslog.New(&promslog.Config{Writer: &buf})

tmplFn := TmplHTMLWithLogger(tmpl, data, &tmplErr, logger)

result := tmplFn("{{ .Missing")

require.Error(t, tmplErr)
require.Empty(t, result)
require.Contains(t, buf.String(), "template execution failed")
}
4 changes: 2 additions & 2 deletions notify/victorops/victorops.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
var err error
var (
data = notify.GetTemplateData(ctx, n.tmpl, as, n.logger)
tmpl = notify.TmplText(n.tmpl, data, &err)
tmpl = notify.TmplTextWithLogger(n.tmpl, data, &err, n.logger)
apiURL = n.conf.APIURL.Copy()
)

Expand Down Expand Up @@ -125,7 +125,7 @@ func (n *Notifier) createVictorOpsPayload(ctx context.Context, as ...*types.Aler
var (
alerts = types.Alerts(as...)
data = notify.GetTemplateData(ctx, n.tmpl, as, n.logger)
tmpl = notify.TmplText(n.tmpl, data, &err)
tmpl = notify.TmplTextWithLogger(n.tmpl, data, &err, n.logger)

messageType = tmpl(n.conf.MessageType)
stateMessage = tmpl(n.conf.StateMessage)
Expand Down
2 changes: 1 addition & 1 deletion notify/webex/webex.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
logger.Debug("extracted group key")

data := notify.GetTemplateData(ctx, n.tmpl, as, logger)
tmpl := notify.TmplText(n.tmpl, data, &err)
tmpl := notify.TmplTextWithLogger(n.tmpl, data, &err, logger)
if err != nil {
return false, err
}
Expand Down
4 changes: 2 additions & 2 deletions notify/webhook/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func (n *Notifier) Notify(ctx context.Context, alerts ...*types.Alert) (bool, er

var url string
var tmplErr error
tmpl := notify.TmplText(n.tmpl, data, &tmplErr)
tmpl := notify.TmplTextWithLogger(n.tmpl, data, &tmplErr, logger)

if n.conf.URL != "" {
url = tmpl(string(n.conf.URL))
Expand Down Expand Up @@ -157,7 +157,7 @@ func (n *Notifier) renderPayload(
) (bytes.Buffer, error) {
var (
tmplTextErr error
tmplText = notify.TmplText(n.tmpl, data.Data, &tmplTextErr)
tmplText = notify.TmplTextWithLogger(n.tmpl, data.Data, &tmplTextErr, n.logger)
tmplTextFunc = func(tmpl string) (string, error) {
return tmplText(tmpl), tmplTextErr
}
Expand Down
2 changes: 1 addition & 1 deletion notify/wechat/wechat.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)

data := notify.GetTemplateData(ctx, n.tmpl, as, logger)

tmpl := notify.TmplText(n.tmpl, data, &err)
tmpl := notify.TmplTextWithLogger(n.tmpl, data, &err, logger)
if err != nil {
return false, err
}
Expand Down
Loading