ronantakizawa/codereview-bench huggingface dataset:
Repository: prometheus/alertmanager
File: silence/cache.go (Go)

Code before:
// Copyright The Prometheus Authors
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package silence

import (
	"sync"

	"github.com/prometheus/common/model"
)

type cacheEntry struct {
	activeIDs  []string
	pendingIDs []string
	version    int
}

func newCacheEntry(activeIDs, pendingIDs []string, version int) *cacheEntry {
	return &cacheEntry{
		activeIDs:  activeIDs,
		pendingIDs: pendingIDs,
		version:    version,
	}
}

func (e *cacheEntry) count() int {
	return len(e.activeIDs) + len(e.pendingIDs)
}

type cache struct {
	entries map[model.Fingerprint]*cacheEntry
	mu      sync.RWMutex
}

func (c *cache) delete(fp model.Fingerprint) {
	c.mu.Lock()
	defer c.mu.Unlock()
	delete(c.entries, fp)
}

Review comment (performance):
We seem to always concatenate these? Would it make sense to only keep the concatenated version in the cache? We always reevaluate their current state so maybe this distinction is not useful anymore...

  The only place we care about active vs pending separation is:
  - At the end (line 285) when updating the marker with only activeIDs
  - But that comes from the re-evaluation loop (lines 262-273), not from the cache

Code after:
// Copyright The Prometheus Authors
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package silence

import (
	"sync"

	"github.com/prometheus/common/model"
)

// cacheEntry stores the IDs of silences that match an alert and the version of the silences state the
// result is based on.
type cacheEntry struct {
	silenceIDs []string
	version    int
}

// newCacheEntry creates a new cacheEntry.
func newCacheEntry(version int, silenceIDs ...string) *cacheEntry {
	return &cacheEntry{
		silenceIDs: silenceIDs,
		version:    version,
	}
}

// count returns the number of silence IDs in the cacheEntry.
func (e *cacheEntry) count() int {
	return len(e.silenceIDs)
}

// cache stores the IDs of silences that match an alert and the version of the silences state the
// result is based on.
type cache struct {
	entries map[model.Fingerprint]*cacheEntry
	mtx     sync.RWMutex
}

// delete removes the cacheEntry for the given fingerprint.
Repository: prometheus/alertmanager
File: silence/cache.go (Go)

Code before:

// cacheEntry stores the IDs of silences that match an alert and the version of the silences state the
// result is based on.
type cacheEntry struct {
	silenceIDs []string
	version    int
}

// newCacheEntry creates a new cacheEntry.
func newCacheEntry(version int, silenceIDs ...string) *cacheEntry {
	return &cacheEntry{
		silenceIDs: silenceIDs,
		version:    version,
	}
}

// count returns the number of silence IDs in the cacheEntry.
func (e *cacheEntry) count() int {
	return len(e.silenceIDs)
}

// cache stores the IDs of silences that match an alert and the version of the silences state the
// result is based on.
type cache struct {
	entries map[model.Fingerprint]*cacheEntry
	mu      sync.RWMutex
}

// delete removes the cacheEntry for the given fingerprint.
func (c *cache) delete(fp model.Fingerprint) {
	c.mu.Lock()
	defer c.mu.Unlock()
	delete(c.entries, fp)
}

// get returns the cacheEntry for the given fingerprint.
// The returned entry is not a copy, so it should not be modified.
func (c *cache) get(fp model.Fingerprint) *cacheEntry {
	c.mu.RLock()
	defer c.mu.RUnlock()
	if e, found := c.entries[fp]; found {
		return e
	}
	return &cacheEntry{}
}

// set sets the cacheEntry for the given fingerprint.
func (c *cache) set(fp model.Fingerprint, entry *cacheEntry) {
	c.mu.Lock()
	defer c.mu.Unlock()
	c.entries[fp] = entry

Review comment (suggestion):
tiny nitpick: we call this `mtx` everywhere else in alertmanager. I think we should be consistent where we can

Code after:

// cacheEntry stores the IDs of silences that match an alert and the version of the silences state the
// result is based on.
type cacheEntry struct {
	silenceIDs []string
	version    int
}

// newCacheEntry creates a new cacheEntry.
func newCacheEntry(version int, silenceIDs ...string) *cacheEntry {
	return &cacheEntry{
		silenceIDs: silenceIDs,
		version:    version,
	}
}

// count returns the number of silence IDs in the cacheEntry.
func (e *cacheEntry) count() int {
	return len(e.silenceIDs)
}

// cache stores the IDs of silences that match an alert and the version of the silences state the
// result is based on.
type cache struct {
	entries map[model.Fingerprint]*cacheEntry
	mtx     sync.RWMutex
}

// delete removes the cacheEntry for the given fingerprint.
func (c *cache) delete(fp model.Fingerprint) {
	c.mtx.Lock()
	defer c.mtx.Unlock()
	delete(c.entries, fp)
}

// get returns the cacheEntry for the given fingerprint.
// The returned entry is not a copy, so it should not be modified.
func (c *cache) get(fp model.Fingerprint) *cacheEntry {
	c.mtx.RLock()
	defer c.mtx.RUnlock()
	if e, found := c.entries[fp]; found {
		return e
	}
	return &cacheEntry{}
}

// set sets the cacheEntry for the given fingerprint.
func (c *cache) set(fp model.Fingerprint, entry *cacheEntry) {
	c.mtx.Lock()
	defer c.mtx.Unlock()
	c.entries[fp] = entry
Repository: prometheus/alertmanager
File: dispatch/dispatch_bench_test.go (Go)

Code before:
// Copyright 2025 The Prometheus Authors
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package dispatch

import (
	"context"
	"fmt"
	"testing"
	"time"

	"github.com/prometheus/client_golang/prometheus"
	"github.com/prometheus/common/model"
	"github.com/prometheus/common/promslog"
	"github.com/stretchr/testify/require"


Review comment (security):
```suggestion
// Copyright The Prometheus Authors
```

Code after:
// Copyright The Prometheus Authors
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package dispatch

import (
	"context"
	"fmt"
	"testing"
	"time"

	"github.com/prometheus/client_golang/prometheus"
	"github.com/prometheus/common/model"
	"github.com/prometheus/common/promslog"
	"github.com/stretchr/testify/require"
Repository: prometheus/alertmanager
File: notify/slack/slack.go (Go)

Code before:
					OkText:      tmplText(action.ConfirmField.OkText),
					DismissText: tmplText(action.ConfirmField.DismissText),
				}
			}

			actions[index] = slackAction
		}
		att.Actions = actions
	}

	var threadTs string
	var channelId string

	store, ok := notify.NflogStore(ctx)
	if !ok {
		logger.Error("cannot create NflogStore")
	} else {
		threadTs, ok = store.GetStr("threadTs")
		if !ok {
			logger.Debug("no associated threadTs")
		}
		channelId, ok = store.GetStr("channelId")
		if !ok {
			logger.Debug("no associated channelId")
		}
		logger.With("threadTs", threadTs).With("channelId", channelId).Debug("attempt recovering threadTs and channelId")
	}

	var u string
	if n.conf.APIURL != nil {
		u = n.conf.APIURL.String()
	} else {
		content, err := os.ReadFile(n.conf.APIURLFile)
		if err != nil {
			return false, err
		}
		u = strings.TrimSpace(string(content))
	}

	if n.conf.Timeout > 0 {
		postCtx, cancel := context.WithTimeoutCause(ctx, n.conf.Timeout, fmt.Errorf("configured slack timeout reached (%s)", n.conf.Timeout))
		defer cancel()
		ctx = postCtx
	}

	req := &request{
		Channel:     tmplText(n.conf.Channel),
		Username:    tmplText(n.conf.Username),
		IconEmoji:   tmplText(n.conf.IconEmoji),
		IconURL:     tmplText(n.conf.IconURL),
		LinkNames:   n.conf.LinkNames,

Review comment (bug):
This will print 3 lines of logs, we can instead do `.With()` at each step and finally call `.Debug()` to print one line of log with all the information attached to it.

Code after:
					OkText:      tmplText(action.ConfirmField.OkText),
					DismissText: tmplText(action.ConfirmField.DismissText),
				}
			}

			actions[index] = slackAction
		}
		att.Actions = actions
	}

	var u string
	if n.conf.APIURL != nil {
		u = n.conf.APIURL.String()
	} else {
		content, err := os.ReadFile(n.conf.APIURLFile)
		if err != nil {
			return false, err
		}
		u = strings.TrimSpace(string(content))
	}

	if n.conf.Timeout > 0 {
		postCtx, cancel := context.WithTimeoutCause(ctx, n.conf.Timeout, fmt.Errorf("configured slack timeout reached (%s)", n.conf.Timeout))
		defer cancel()
		ctx = postCtx
	}

	req := &request{
		Channel:     tmplText(n.conf.Channel),
		Username:    tmplText(n.conf.Username),
		IconEmoji:   tmplText(n.conf.IconEmoji),
		IconURL:     tmplText(n.conf.IconURL),
		LinkNames:   n.conf.LinkNames,
		Text:        tmplText(n.conf.MessageText),
		Attachments: []attachment{*att},
	}

	// If a notification for this alert group has already been sent and `update_message` config is set
	// edit API endpoint and payload to update notification instead of sending a new one.
	var store *nflog.Store

	if n.conf.UpdateMessage {
		var ok bool
		store, ok = notify.NflogStore(ctx)
		if !ok {
			logger.Warn("cannot create NflogStore, updatable messages will be disabled.")
		} else {
			threadTs, _ := store.GetStr("threadTs")
			channelId, _ := store.GetStr("channelId")
			logger.Debug("attempt recovering threadTs and channelId to update an existing message", "threadTs", threadTs, "channelId", channelId)
			if threadTs != "" && channelId != "" {
Repository: prometheus/alertmanager
File: store/store.go (Go)

Code before:
}

// GC deletes resolved alerts and returns them.
func (a *Alerts) GC() (deleted []*types.Alert) {
	// Remove stale alert limit buckets.
	a.gcLimitBuckets()

	// Delete resolved alerts.
	deleted = a.gcAlerts()

	// Execute GC callback if needed.
	if len(deleted) > 0 {
		a.gcCallback(deleted)
	}

	return deleted
}

// gcAlerts deletes resolved alerts and returns a copy of them.
func (a *Alerts) gcAlerts() (deleted []*types.Alert) {
	a.Lock()
	defer a.Unlock()
	for fp, alert := range a.alerts {
		if alert.Resolved() {
			delete(a.alerts, fp)
			deleted = append(deleted, &types.Alert{
				Alert: model.Alert{
					Labels:       alert.Labels.Clone(),
					Annotations:  alert.Annotations.Clone(),
					StartsAt:     alert.StartsAt,
					EndsAt:       alert.EndsAt,
					GeneratorURL: alert.GeneratorURL,
				},
				UpdatedAt: alert.UpdatedAt,
				Timeout:   alert.Timeout,
			})
		}
	}
	return deleted
}

// gcLimitBuckets removes stale alert limit buckets.
func (a *Alerts) gcLimitBuckets() {
	a.Lock()
	defer a.Unlock()

	for alertName, bucket := range a.limits {
		if bucket.IsStale() {
			delete(a.limits, alertName)
		}
	}

Review comment (suggestion):
Is there a reason why we can't return just pointers to the existing alerts... We have removed them from the store, what are we worried about that we have to copy them? We just need to make sure the callbacks don't modify them... We would save a lot of allocations while holding the lock...) If we can't potentially let's check if it isn't better to just do what we did before: at least there would be fewer allocations, just the slice backing array growth, instead of that, plus one per alert...

Code after:
}

// GC deletes resolved alerts and returns them.
func (a *Alerts) GC() (deleted []*types.Alert) {
	// Remove stale alert limit buckets.
	a.gcLimitBuckets()

	// Delete resolved alerts.
	deleted = a.gcAlerts()

	// Execute GC callback if needed.
	if len(deleted) > 0 {
		a.gcCallback(deleted)
	}

	return deleted
}

// gcAlerts deletes resolved alerts and returns a copy of them.
func (a *Alerts) gcAlerts() (deleted []*types.Alert) {
	a.Lock()
	defer a.Unlock()
	for fp, alert := range a.alerts {
		if alert.Resolved() {
			deleted = append(deleted, alert)
			delete(a.alerts, fp)
		}
	}
	return deleted
}

// gcLimitBuckets removes stale alert limit buckets.
func (a *Alerts) gcLimitBuckets() {
	a.Lock()
	defer a.Unlock()

	for alertName, bucket := range a.limits {
		if bucket.IsStale() {
			delete(a.limits, alertName)
		}
	}
}

// Get returns the Alert with the matching fingerprint, or an error if it is
// not found.
func (a *Alerts) Get(fp model.Fingerprint) (*types.Alert, error) {
	a.Lock()
	defer a.Unlock()

	alert, prs := a.alerts[fp]
	if !prs {
Repository: prometheus/alertmanager
File: config/receiver/receiver.go (Go)

Code before:
	for i, c := range nc.DiscordConfigs {
		add("discord", i, c, func(l *slog.Logger) (notify.Notifier, error) { return discord.New(c, tmpl, l, httpOpts...) })
	}
	for i, c := range nc.WebexConfigs {
		add("webex", i, c, func(l *slog.Logger) (notify.Notifier, error) { return webex.New(c, tmpl, l, httpOpts...) })
	}
	for i, c := range nc.MSTeamsConfigs {
		add("msteams", i, c, func(l *slog.Logger) (notify.Notifier, error) { return msteams.New(c, tmpl, l, httpOpts...) })
	}
	for i, c := range nc.MSTeamsV2Configs {
		add("msteamsv2", i, c, func(l *slog.Logger) (notify.Notifier, error) { return msteamsv2.New(c, tmpl, l, httpOpts...) })
	}
	for i, c := range nc.JiraConfigs {
		add("jira", i, c, func(l *slog.Logger) (notify.Notifier, error) { return jira.New(c, tmpl, l, httpOpts...) })
	}
	for i, c := range nc.IncidentioConfigs {
		add("incidentio", i, c, func(l *slog.Logger) (notify.Notifier, error) { return incidentio.New(c, tmpl, l, httpOpts...) })
	}
	for i, c := range nc.RocketchatConfigs {
		add("rocketchat", i, c, func(l *slog.Logger) (notify.Notifier, error) { return rocketchat.New(c, tmpl, l, httpOpts...) })
	}
	for i, c := range nc.MattermostConfigs {
		add("mattermost", i, c, func(l *slog.Logger) (notify.Notifier, error) { return mattermost.New(c, tmpl, l, httpOpts...) })
	}

	if errs != nil {
		return nil, errs
	}
	return integrations, nil
}


Review comment (suggestion):
I think the previous version was better. `integrations` only contains those without errors:
```go
if err != nil {
	errs = errors.Join(errs, err)
	return
}
integrations = append(integrations, notify.NewIntegration(n, rs, name, i, nc.Name))
```

Code after:
	for i, c := range nc.DiscordConfigs {
		add("discord", i, c, func(l *slog.Logger) (notify.Notifier, error) { return discord.New(c, tmpl, l, httpOpts...) })
	}
	for i, c := range nc.WebexConfigs {
		add("webex", i, c, func(l *slog.Logger) (notify.Notifier, error) { return webex.New(c, tmpl, l, httpOpts...) })
	}
	for i, c := range nc.MSTeamsConfigs {
		add("msteams", i, c, func(l *slog.Logger) (notify.Notifier, error) { return msteams.New(c, tmpl, l, httpOpts...) })
	}
	for i, c := range nc.MSTeamsV2Configs {
		add("msteamsv2", i, c, func(l *slog.Logger) (notify.Notifier, error) { return msteamsv2.New(c, tmpl, l, httpOpts...) })
	}
	for i, c := range nc.JiraConfigs {
		add("jira", i, c, func(l *slog.Logger) (notify.Notifier, error) { return jira.New(c, tmpl, l, httpOpts...) })
	}
	for i, c := range nc.IncidentioConfigs {
		add("incidentio", i, c, func(l *slog.Logger) (notify.Notifier, error) { return incidentio.New(c, tmpl, l, httpOpts...) })
	}
	for i, c := range nc.RocketchatConfigs {
		add("rocketchat", i, c, func(l *slog.Logger) (notify.Notifier, error) { return rocketchat.New(c, tmpl, l, httpOpts...) })
	}
	for i, c := range nc.MattermostConfigs {
		add("mattermost", i, c, func(l *slog.Logger) (notify.Notifier, error) { return mattermost.New(c, tmpl, l, httpOpts...) })
	}

	return integrations, errs
}
Repository: prometheus/prometheus
File: cmd/promtool/tsdb.go (Go)

Code before:
func CondensedString(ls labels.Labels) string {
	var b bytes.Buffer

	b.WriteByte('{')
	i := 0
	ls.Range(func(l labels.Label) {
		if i > 0 {
			b.WriteByte(',')
		}
		b.WriteString(l.Name)
		b.WriteByte('=')
		b.WriteString(strconv.Quote(l.Value))
		i++
	})
	b.WriteByte('}')
	return b.String()
}

func formatSeriesSetToJSON(ss storage.SeriesSet) error {
	getSeriesID := func(in []byte) uint64 {
		hash := fnv.New64()
		_, _ = hash.Write(in)
		return hash.Sum64()
	}

	seriesCache := make(map[uint64]struct{})
	for ss.Next() {
		series := ss.At()
		lbs := series.Labels()

		b, err := json.Marshal(lbs)
		if err != nil {
			return err
		}

		if len(b) == 0 {
			continue
		}

		id := getSeriesID(b)
		if _, ok := seriesCache[id]; !ok {
			fmt.Println(string(b))
			seriesCache[id] = struct{}{}
		}
	}
	return nil
}

func formatSeriesSetOpenMetrics(ss storage.SeriesSet) error {
	for ss.Next() {
		series := ss.At()

Review comment (performance):
This feels like a premature optimisation -- the `json.Marshal` is likely the expensive part and I can't imagine storing the labels in memory is that bad. 64-bit FNV-1 probably won't collide, but it could; just use the JSON serialised labels as the key and let Go's map implementation do the hashing.

Code after:

	b.WriteByte('{')
	i := 0
	ls.Range(func(l labels.Label) {
		if i > 0 {
			b.WriteByte(',')
		}
		b.WriteString(l.Name)
		b.WriteByte('=')
		b.WriteString(strconv.Quote(l.Value))
		i++
	})
	b.WriteByte('}')
	return b.String()
}

func formatSeriesSetLabelsToJSON(ss storage.SeriesSet) error {
	seriesCache := make(map[string]struct{})
	for ss.Next() {
		series := ss.At()
		lbs := series.Labels()

		b, err := json.Marshal(lbs)
		if err != nil {
			return err
		}

		if len(b) == 0 {
			continue
		}

		s := string(b)
		if _, ok := seriesCache[s]; !ok {
			fmt.Println(s)
			seriesCache[s] = struct{}{}
		}
	}
	return nil
}

func formatSeriesSetOpenMetrics(ss storage.SeriesSet) error {
	for ss.Next() {
		series := ss.At()
		lbs := series.Labels()
		metricName := lbs.Get(labels.MetricName)
		lbs = lbs.DropMetricName()
		it := series.Iterator(nil)
		for it.Next() == chunkenc.ValFloat {
			ts, val := it.At()
			fmt.Printf("%s%s %g %.3f\n", metricName, CondensedString(lbs), val, float64(ts)/1000)
		}
Repository: prometheus/prometheus
File: tsdb/index/postings.go (Go)

Code before:
}

// LabelValues returns label values for the given name.
func (p *MemPostings) LabelValues(_ context.Context, name string) []string {
	p.mtx.RLock()
	defer p.mtx.RUnlock()

	values := make([]string, 0, len(p.m[name]))
	for v := range p.m[name] {
		values = append(values, v)
	}
	return values
}

// PostingsStats contains cardinality based statistics for postings.
type PostingsStats struct {
	CardinalityMetricsStats []Stat
	CardinalityLabelStats   []Stat
	LabelValueStats         []Stat
	LabelValuePairsStats    []Stat
	NumLabelPairs           int
}

// Stats calculates the cardinality statistics from postings.
// Caller can pass in a function which computes the space required for n series with a given label.
func (p *MemPostings) Stats(label string, limit int, bytes func(string, uint64) uint64) *PostingsStats {
	var size uint64
	p.mtx.RLock()

	metrics := &maxHeap{}
	labels := &maxHeap{}
	labelValueLength := &maxHeap{}
	labelValuePairs := &maxHeap{}
	numLabelPairs := 0

	metrics.init(limit)
	labels.init(limit)
	labelValueLength.init(limit)
	labelValuePairs.init(limit)

	for n, e := range p.m {
		if n == "" {
			continue
		}
		labels.push(Stat{Name: n, Count: uint64(len(e))})
		numLabelPairs += len(e)
		size = 0
		for name, values := range e {
			if n == label {
				metrics.push(Stat{Name: name, Count: uint64(len(values))})
			}

Review comment (suggestion):
`bytes` could be confusing, how about:
```suggestion
func (p *MemPostings) Stats(label string, limit int, labelValueSizeFunc func(string, uint64) uint64) *PostingsStats {
```
or `sizeOfLabelValue`.

Code after:
	return names
}

// LabelValues returns label values for the given name.
func (p *MemPostings) LabelValues(_ context.Context, name string) []string {
	p.mtx.RLock()
	defer p.mtx.RUnlock()

	values := make([]string, 0, len(p.m[name]))
	for v := range p.m[name] {
		values = append(values, v)
	}
	return values
}

// PostingsStats contains cardinality based statistics for postings.
type PostingsStats struct {
	CardinalityMetricsStats []Stat
	CardinalityLabelStats   []Stat
	LabelValueStats         []Stat
	LabelValuePairsStats    []Stat
	NumLabelPairs           int
}

// Stats calculates the cardinality statistics from postings.
// Caller can pass in a function which computes the space required for n series with a given label.
func (p *MemPostings) Stats(label string, limit int, labelSizeFunc func(string, string, uint64) uint64) *PostingsStats {
	var size uint64
	p.mtx.RLock()

	metrics := &maxHeap{}
	labels := &maxHeap{}
	labelValueLength := &maxHeap{}
	labelValuePairs := &maxHeap{}
	numLabelPairs := 0

	metrics.init(limit)
	labels.init(limit)
	labelValueLength.init(limit)
	labelValuePairs.init(limit)

	for n, e := range p.m {
		if n == "" {
			continue
		}
		labels.push(Stat{Name: n, Count: uint64(len(e))})
		numLabelPairs += len(e)
		size = 0
		for name, values := range e {
			if n == label {
				metrics.push(Stat{Name: name, Count: uint64(len(values))})