ronantakizawa/codereview-bench huggingface dataset:
Repository: prometheus/prometheus
File: web/api/v1/api.go (Go)

Code before:
					return u, err
				}
				u.Host = host
				if port != "" {
					u.Host = net.JoinHostPort(u.Host, port)
				}
			}
			break
		}
	}

	return u, nil
}

func (api *API) scrapePools(r *http.Request) apiFuncResult {
	names := api.scrapePoolsRetriever(r.Context()).ScrapePools()
	sort.Strings(names)
	res := &ScrapePoolsDiscovery{ScrapePools: names}
	return apiFuncResult{data: res, err: nil, warnings: nil, finalizer: nil}
}

func (api *API) targets(r *http.Request) apiFuncResult {
	getSortedPools := func(targets map[string][]*scrape.Target) ([]string, int) {
		var n int
		pools := make([]string, 0, len(targets))
		for p := range targets {
			pools = append(pools, p)
			n += len(targets[p])
		}
		slices.Sort(pools)
		return pools, n
	}

	scrapePool := r.URL.Query().Get("scrapePool")
	state := strings.ToLower(r.URL.Query().Get("state"))
	showActive := state == "" || state == "any" || state == "active"
	showDropped := state == "" || state == "any" || state == "dropped"
	res := &TargetDiscovery{}
	builder := labels.NewBuilder(labels.EmptyLabels())

	if showActive {
		targetsActive := api.targetRetriever(r.Context()).TargetsActive()
		activePools, numTargets := getSortedPools(targetsActive)
		res.ActiveTargets = make([]*Target, 0, numTargets)

		for _, p := range activePools {
			if scrapePool != "" && p != scrapePool {
				continue
			}
			for _, target := range targetsActive[p] {
				lastErrStr := ""

Review comment (performance):
Probably more efficient to pull the value out here and use it 2 lines later, rather than doing a map lookup every time.
```suggestion
		for p, t := range targets {
```

Code after:
					return u, err
				}
				u.Host = host
				if port != "" {
					u.Host = net.JoinHostPort(u.Host, port)
				}
			}
			break
		}
	}

	return u, nil
}

func (api *API) scrapePools(r *http.Request) apiFuncResult {
	names := api.scrapePoolsRetriever(r.Context()).ScrapePools()
	sort.Strings(names)
	res := &ScrapePoolsDiscovery{ScrapePools: names}
	return apiFuncResult{data: res, err: nil, warnings: nil, finalizer: nil}
}

func (api *API) targets(r *http.Request) apiFuncResult {
	getSortedPools := func(targets map[string][]*scrape.Target) ([]string, int) {
		var n int
		pools := make([]string, 0, len(targets))
		for p, t := range targets {
			pools = append(pools, p)
			n += len(t)
		}
		slices.Sort(pools)
		return pools, n
	}

	scrapePool := r.URL.Query().Get("scrapePool")
	state := strings.ToLower(r.URL.Query().Get("state"))
	showActive := state == "" || state == "any" || state == "active"
	showDropped := state == "" || state == "any" || state == "dropped"
	res := &TargetDiscovery{}
	builder := labels.NewBuilder(labels.EmptyLabels())

	if showActive {
		targetsActive := api.targetRetriever(r.Context()).TargetsActive()
		activePools, numTargets := getSortedPools(targetsActive)
		res.ActiveTargets = make([]*Target, 0, numTargets)

		for _, pool := range activePools {
			if scrapePool != "" && pool != scrapePool {
				continue
			}
			for _, target := range targetsActive[pool] {
				lastErrStr := ""
Repository: prometheus/prometheus
File: scrape/scrape_test.go (Go)

Code before:
		},
	})

	require.NoError(t, p.reload(cfg))

	<-time.After(1 * time.Second)
}

// Regression test against https://github.com/prometheus/prometheus/issues/16160.
func TestScrapeAppendWithParseError(t *testing.T) {
	const (
		scrape1 = `metric_a 1
`
		scrape2 = `metric_a 11
# EOF`
	)

	sl := newBasicScrapeLoop(t, context.Background(), nil, nil, 0)
	sl.cache = newScrapeCache(sl.metrics)

	now := time.Now()
	capp := &collectResultAppender{next: nopAppender{}}
	_, _, _, err := sl.append(capp, []byte(scrape1), "application/openmetrics-text", now)
	require.Error(t, err)
	_, _, _, err = sl.append(capp, nil, "application/openmetrics-text", now)
	require.NoError(t, err)
	require.Empty(t, capp.resultFloats)

	capp = &collectResultAppender{next: nopAppender{}}
	_, _, _, err = sl.append(capp, []byte(scrape2), "application/openmetrics-text", now.Add(15*time.Second))
	require.NoError(t, err)
	require.NoError(t, capp.Commit())

	want := []floatSample{
		{
			metric: labels.FromStrings(model.MetricNameLabel, "metric_a"),
			t:      timestamp.FromTime(now.Add(15 * time.Second)),
			f:      11,
		},
	}
	requireEqual(t, want, capp.resultFloats, "Appended samples not as expected:\n%s", capp)
}


Review comment (bug):
maybe add a `// The append fails with a parsing error`

Code after:
		},
	})

	require.NoError(t, p.reload(cfg))

	<-time.After(1 * time.Second)
}

// Regression test against https://github.com/prometheus/prometheus/issues/16160.
// The first scrape fails with a parsing error, but the second should
// succeed and cause `metric_1=11` to appear in the appender.
func TestScrapeAppendWithParseError(t *testing.T) {
	const (
		scrape1 = `metric_a 1
`
		scrape2 = `metric_a 11
# EOF`
	)

	sl := newBasicScrapeLoop(t, context.Background(), nil, nil, 0)
	sl.cache = newScrapeCache(sl.metrics)

	now := time.Now()
	capp := &collectResultAppender{next: nopAppender{}}
	_, _, _, err := sl.append(capp, []byte(scrape1), "application/openmetrics-text", now)
	require.Error(t, err)
	_, _, _, err = sl.append(capp, nil, "application/openmetrics-text", now)
	require.NoError(t, err)
	require.Empty(t, capp.resultFloats)

	capp = &collectResultAppender{next: nopAppender{}}
	_, _, _, err = sl.append(capp, []byte(scrape2), "application/openmetrics-text", now.Add(15*time.Second))
	require.NoError(t, err)
	require.NoError(t, capp.Commit())

	want := []floatSample{
		{
			metric: labels.FromStrings(model.MetricNameLabel, "metric_a"),
			t:      timestamp.FromTime(now.Add(15 * time.Second)),
			f:      11,
		},
	}
	requireEqual(t, want, capp.resultFloats, "Appended samples not as expected:\n%s", capp)
}
Repository: prometheus/prometheus
File: scrape/scrape_test.go (Go)

Code before:
	sl := newBasicScrapeLoop(t, context.Background(), nil, nil, 0)
	sl.cache = newScrapeCache(sl.metrics)

	now := time.Now()
	capp := &collectResultAppender{next: nopAppender{}}
	_, _, _, err := sl.append(capp, []byte(scrape1), "application/openmetrics-text", now)
	require.Error(t, err)
	_, _, _, err = sl.append(capp, nil, "application/openmetrics-text", now)
	require.NoError(t, err)
	require.Empty(t, capp.resultFloats)

	capp = &collectResultAppender{next: nopAppender{}}
	_, _, _, err = sl.append(capp, []byte(scrape2), "application/openmetrics-text", now.Add(15*time.Second))
	require.NoError(t, err)
	require.NoError(t, capp.Commit())

	want := []floatSample{
		{
			metric: labels.FromStrings(model.MetricNameLabel, "metric_a"),
			t:      timestamp.FromTime(now.Add(15 * time.Second)),
			f:      11,
		},
	}
	requireEqual(t, want, capp.resultFloats, "Appended samples not as expected:\n%s", capp)
}


Review comment (bug):
maybe add `// The second append should be considered even when the first one fails`

Suggesting such comments to ease the understanding

Code after:
	)

	sl := newBasicScrapeLoop(t, context.Background(), nil, nil, 0)
	sl.cache = newScrapeCache(sl.metrics)

	now := time.Now()
	capp := &collectResultAppender{next: nopAppender{}}
	_, _, _, err := sl.append(capp, []byte(scrape1), "application/openmetrics-text", now)
	require.Error(t, err)
	_, _, _, err = sl.append(capp, nil, "application/openmetrics-text", now)
	require.NoError(t, err)
	require.Empty(t, capp.resultFloats)

	capp = &collectResultAppender{next: nopAppender{}}
	_, _, _, err = sl.append(capp, []byte(scrape2), "application/openmetrics-text", now.Add(15*time.Second))
	require.NoError(t, err)
	require.NoError(t, capp.Commit())

	want := []floatSample{
		{
			metric: labels.FromStrings(model.MetricNameLabel, "metric_a"),
			t:      timestamp.FromTime(now.Add(15 * time.Second)),
			f:      11,
		},
	}
	requireEqual(t, want, capp.resultFloats, "Appended samples not as expected:\n%s", capp)
}
Repository: prometheus/prometheus
File: web/api/v1/api.go (Go)

Code before:

	// Non-standard status code (originally introduced by nginx) for the case when a client closes
	// the connection while the server is still processing the request.
	statusClientClosedConnection = 499

	// checkContextEveryNIterations is used in some tight loops to check if the context is done.
	checkContextEveryNIterations = 128
)

type errorType string

const (
	ErrorNone          errorType = ""
	ErrorTimeout       errorType = "timeout"
	ErrorCanceled      errorType = "canceled"
	ErrorExec          errorType = "execution"
	ErrorBadData       errorType = "bad_data"
	ErrorInternal      errorType = "internal"
	ErrorUnavailable   errorType = "unavailable"
	ErrorNotFound      errorType = "not_found"
	ErrorNotAcceptable errorType = "not_acceptable"
)

type ErrorTypeToStatusCode map[errorType]func(error) int

var defaultErrorTypeToStatusCode = map[errorType]func(error) int{
	ErrorBadData: func(_ error) int {
		return http.StatusBadRequest
	},
	ErrorExec: func(_ error) int {
		return http.StatusUnprocessableEntity
	},
	ErrorCanceled: func(_ error) int {
		return statusClientClosedConnection
	},
	ErrorTimeout: func(_ error) int {
		return http.StatusServiceUnavailable
	},
	ErrorInternal: func(_ error) int {
		return http.StatusInternalServerError
	},
	ErrorNotFound: func(_ error) int {
		return http.StatusNotFound
	},
	ErrorNotAcceptable: func(_ error) int {
		return http.StatusNotAcceptable
	},
}

var LocalhostRepresentations = []string{"127.0.0.1", "localhost", "::1"}


Review comment (suggestion):
```suggestion
var defaultErrorTypeToStatusCode = ErrorTypeToStatusCode{
```

Code after:

	// Non-standard status code (originally introduced by nginx) for the case when a client closes
	// the connection while the server is still processing the request.
	statusClientClosedConnection = 499

	// checkContextEveryNIterations is used in some tight loops to check if the context is done.
	checkContextEveryNIterations = 128
)

type errorNum int

type errorType struct {
	num errorNum
	str string
}

const (
	ErrorNone errorNum = iota
	ErrorTimeout
	ErrorCanceled
	ErrorExec
	ErrorBadData
	ErrorInternal
	ErrorUnavailable
	ErrorNotFound
	ErrorNotAcceptable
)

var (
	errorNone          = errorType{ErrorNone, ""}
	errorTimeout       = errorType{ErrorTimeout, "timeout"}
	errorCanceled      = errorType{ErrorCanceled, "canceled"}
	errorExec          = errorType{ErrorExec, "execution"}
	errorBadData       = errorType{ErrorBadData, "bad_data"}
	errorInternal      = errorType{ErrorInternal, "internal"}
	errorUnavailable   = errorType{ErrorUnavailable, "unavailable"}
	errorNotFound      = errorType{ErrorNotFound, "not_found"}
	errorNotAcceptable = errorType{ErrorNotAcceptable, "not_acceptable"}
)

// OverrideErrorCode can be used to override status code for different error types.
// Return false to fall back to default status code.
type OverrideErrorCode func(errorNum, error) (code int, override bool)

var LocalhostRepresentations = []string{"127.0.0.1", "localhost", "::1"}

type apiError struct {
	typ errorType
	err error
}
Repository: prometheus/prometheus
File: web/api/v1/api.go (Go)

Code before:

const (
	ErrorNone errorNum = iota
	ErrorTimeout
	ErrorCanceled
	ErrorExec
	ErrorBadData
	ErrorInternal
	ErrorUnavailable
	ErrorNotFound
	ErrorNotAcceptable
)

var (
	errorNone          = errorType{ErrorNone, ""}
	errorTimeout       = errorType{ErrorTimeout, "timeout"}
	errorCanceled      = errorType{ErrorCanceled, "canceled"}
	errorExec          = errorType{ErrorExec, "execution"}
	errorBadData       = errorType{ErrorBadData, "bad_data"}
	errorInternal      = errorType{ErrorInternal, "internal"}
	errorUnavailable   = errorType{ErrorUnavailable, "unavailable"}
	errorNotFound      = errorType{ErrorNotFound, "not_found"}
	errorNotAcceptable = errorType{ErrorNotAcceptable, "not_acceptable"}
)

type ErrorTypeToStatusCode map[errorNum]func(error) int

var LocalhostRepresentations = []string{"127.0.0.1", "localhost", "::1"}

type apiError struct {
	typ errorType
	err error
}

func (e *apiError) Error() string {
	return fmt.Sprintf("%s: %s", e.typ.str, e.err)
}

// ScrapePoolsRetriever provide the list of all scrape pools.
type ScrapePoolsRetriever interface {
	ScrapePools() []string
}

// TargetRetriever provides the list of active/dropped targets to scrape or not.
type TargetRetriever interface {
	TargetsActive() map[string][]*scrape.Target
	TargetsDropped() map[string][]*scrape.Target
	TargetsDroppedCounts() map[string]int
}

// AlertmanagerRetriever provides a list of all/dropped AlertManager URLs.

Review comment (suggestion):
Could this be simplified to `func(errorNum, error) int` ?

Or possibly `func(errorNum, error) (bool, int)` with the bool indicating you want to fall back.

Code after:
const (
	ErrorNone errorNum = iota
	ErrorTimeout
	ErrorCanceled
	ErrorExec
	ErrorBadData
	ErrorInternal
	ErrorUnavailable
	ErrorNotFound
	ErrorNotAcceptable
)

var (
	errorNone          = errorType{ErrorNone, ""}
	errorTimeout       = errorType{ErrorTimeout, "timeout"}
	errorCanceled      = errorType{ErrorCanceled, "canceled"}
	errorExec          = errorType{ErrorExec, "execution"}
	errorBadData       = errorType{ErrorBadData, "bad_data"}
	errorInternal      = errorType{ErrorInternal, "internal"}
	errorUnavailable   = errorType{ErrorUnavailable, "unavailable"}
	errorNotFound      = errorType{ErrorNotFound, "not_found"}
	errorNotAcceptable = errorType{ErrorNotAcceptable, "not_acceptable"}
)

// OverrideErrorCode can be used to override status code for different error types.
// Return false to fall back to default status code.
type OverrideErrorCode func(errorNum, error) (code int, override bool)

var LocalhostRepresentations = []string{"127.0.0.1", "localhost", "::1"}

type apiError struct {
	typ errorType
	err error
}

func (e *apiError) Error() string {
	return fmt.Sprintf("%s: %s", e.typ.str, e.err)
}

// ScrapePoolsRetriever provide the list of all scrape pools.
type ScrapePoolsRetriever interface {
	ScrapePools() []string
}

// TargetRetriever provides the list of active/dropped targets to scrape or not.
type TargetRetriever interface {
	TargetsActive() map[string][]*scrape.Target
	TargetsDropped() map[string][]*scrape.Target
	TargetsDroppedCounts() map[string]int
}
Repository: prometheus/prometheus
File: storage/remote/intern_test.go (Go)

Code before:
}

func TestIntern_MultiRef_Concurrent(t *testing.T) {
	interner := newPool()
	testString := "TestIntern_MultiRef_Concurrent"

	interner.intern(testString)
	interned, ok := interner.pool[testString]
	require.True(t, ok)
	require.Equalf(t, int64(1), interned.refs.Load(), "wrong interned refs count")

	for i := 0; i < 1000; i++ {
		released := make(chan struct{})
		go func() {
			interner.release(testString)
			close(released)
		}()
		interner.intern(testString)
		<-released
	}

	interner.mtx.RLock()
	interned, ok = interner.pool[testString]
	interner.mtx.RUnlock()
	require.True(t, ok)
	require.Equalf(t, int64(1), interned.refs.Load(), "wrong interned refs count")
}


Review comment (suggestion):
Note for reviewers: `require.Equal` will already say "expected X but got Y", there's no need to say that, we just need to explain that difference.

Why did I change this? Because we were doing `interned.refs.Load()` twice, so the numbers could be different between the one passed to the message and to the expectation.

Code after:
}

func TestIntern_MultiRef_Concurrent(t *testing.T) {
	interner := newPool()
	testString := "TestIntern_MultiRef_Concurrent"

	interner.intern(testString)
	interned, ok := interner.pool[testString]
	require.True(t, ok)
	require.Equal(t, int64(1), interned.refs.Load(), "wrong interned refs count")

	for i := 0; i < 1000; i++ {
		released := make(chan struct{})
		go func() {
			interner.release(testString)
			close(released)
		}()
		interner.intern(testString)
		<-released
	}

	interner.mtx.RLock()
	interned, ok = interner.pool[testString]
	interner.mtx.RUnlock()
	require.True(t, ok)
	require.Equal(t, int64(1), interned.refs.Load(), "wrong interned refs count")
}
Repository: prometheus/prometheus
File: discovery/kubernetes/endpointslice_test.go (Go)

Code before:
	}.Run(t)
}

// TestEndpointSliceInfIndexersCount makes sure that RoleEndpointSlice discovery
// sets up indexing for the main Kube informer only when needed.
// See: https://github.com/prometheus/prometheus/pull/13554#discussion_r1490965817
func TestEndpointSliceInfIndexersCount(t *testing.T) {
	t.Parallel()
	tests := []struct {
		name             string
		withNodeMetadata bool
	}{
		{"with node metadata", true},
		{"without node metadata", false},
	}

	for _, tc := range tests {
		t.Run(tc.name, func(t *testing.T) {
			t.Parallel()
			var (
				n *Discovery
				// service indexer is enabled by default for better iteration performance
				mainInfIndexersCount = 1
			)
			if tc.withNodeMetadata {
				mainInfIndexersCount = 2
				n, _ = makeDiscoveryWithMetadata(RoleEndpointSlice, NamespaceDiscovery{}, AttachMetadataConfig{Node: true})
			} else {
				n, _ = makeDiscovery(RoleEndpointSlice, NamespaceDiscovery{})
			}

			k8sDiscoveryTest{
				discovery: n,
				afterStart: func() {
					n.RLock()
					defer n.RUnlock()
					require.Len(t, n.discoverers, 1)
					require.Len(t, n.discoverers[0].(*EndpointSlice).endpointSliceInf.GetIndexer().GetIndexers(), mainInfIndexersCount)
				},
			}.Run(t)
		})
	}
}

func TestEndpointSliceDiscoverySidecarContainer(t *testing.T) {
	t.Parallel()
	objs := []runtime.Object{
		&v1.EndpointSlice{
			ObjectMeta: metav1.ObjectMeta{
				Name:      "testsidecar",
				Namespace: "default",

Review comment (suggestion):
nit
```suggestion
				mainInfIndexersCount++
```

Code after:
	}.Run(t)
}

// TestEndpointSliceInfIndexersCount makes sure that RoleEndpointSlice discovery
// sets up indexing for the main Kube informer only when needed.
// See: https://github.com/prometheus/prometheus/pull/13554#discussion_r1490965817
func TestEndpointSliceInfIndexersCount(t *testing.T) {
	t.Parallel()
	tests := []struct {
		name             string
		withNodeMetadata bool
	}{
		{"with node metadata", true},
		{"without node metadata", false},
	}

	for _, tc := range tests {
		t.Run(tc.name, func(t *testing.T) {
			t.Parallel()
			var (
				n *Discovery
				// service indexer is enabled by default
				mainInfIndexersCount = 1
			)
			if tc.withNodeMetadata {
				mainInfIndexersCount++
				n, _ = makeDiscoveryWithMetadata(RoleEndpointSlice, NamespaceDiscovery{}, AttachMetadataConfig{Node: true})
			} else {
				n, _ = makeDiscovery(RoleEndpointSlice, NamespaceDiscovery{})
			}

			k8sDiscoveryTest{
				discovery: n,
				afterStart: func() {
					n.RLock()
					defer n.RUnlock()
					require.Len(t, n.discoverers, 1)
					require.Len(t, n.discoverers[0].(*EndpointSlice).endpointSliceInf.GetIndexer().GetIndexers(), mainInfIndexersCount)
				},
			}.Run(t)
		})
	}
}

func TestEndpointSliceDiscoverySidecarContainer(t *testing.T) {
	t.Parallel()
	objs := []runtime.Object{
		&v1.EndpointSlice{
			ObjectMeta: metav1.ObjectMeta{
				Name:      "testsidecar",
				Namespace: "default",
Repository: prometheus/prometheus
File: discovery/kubernetes/endpointslice_test.go (Go)

Code before:
	k8sDiscoveryTest{
		discovery:        n,
		expectedMaxItems: 0,
		expectedRes:      map[string]*targetgroup.Group{},
	}.Run(t)
}

// TestEndpointSliceInfIndexersCount makes sure that RoleEndpointSlice discovery
// sets up indexing for the main Kube informer only when needed.
// See: https://github.com/prometheus/prometheus/pull/13554#discussion_r1490965817
func TestEndpointSliceInfIndexersCount(t *testing.T) {
	t.Parallel()
	tests := []struct {
		name             string
		withNodeMetadata bool
	}{
		{"with node metadata", true},
		{"without node metadata", false},
	}

	for _, tc := range tests {
		t.Run(tc.name, func(t *testing.T) {
			t.Parallel()
			var (
				n *Discovery
				// service indexer is enabled by default for better iteration performance
				mainInfIndexersCount = 1
			)
			if tc.withNodeMetadata {
				mainInfIndexersCount = 2
				n, _ = makeDiscoveryWithMetadata(RoleEndpointSlice, NamespaceDiscovery{}, AttachMetadataConfig{Node: true})
			} else {
				n, _ = makeDiscovery(RoleEndpointSlice, NamespaceDiscovery{})
			}

			k8sDiscoveryTest{
				discovery: n,
				afterStart: func() {
					n.RLock()
					defer n.RUnlock()
					require.Len(t, n.discoverers, 1)
					require.Len(t, n.discoverers[0].(*EndpointSlice).endpointSliceInf.GetIndexer().GetIndexers(), mainInfIndexersCount)
				},
			}.Run(t)
		})
	}
}

func TestEndpointSliceDiscoverySidecarContainer(t *testing.T) {
	t.Parallel()
	objs := []runtime.Object{

Review comment (suggestion):
```suggestion
				// service indexer is enabled by default
```

Code after:
	k8sDiscoveryTest{
		discovery:        n,
		expectedMaxItems: 0,
		expectedRes:      map[string]*targetgroup.Group{},
	}.Run(t)
}

// TestEndpointSliceInfIndexersCount makes sure that RoleEndpointSlice discovery
// sets up indexing for the main Kube informer only when needed.
// See: https://github.com/prometheus/prometheus/pull/13554#discussion_r1490965817
func TestEndpointSliceInfIndexersCount(t *testing.T) {
	t.Parallel()
	tests := []struct {
		name             string
		withNodeMetadata bool
	}{
		{"with node metadata", true},
		{"without node metadata", false},
	}

	for _, tc := range tests {
		t.Run(tc.name, func(t *testing.T) {
			t.Parallel()
			var (
				n *Discovery
				// service indexer is enabled by default
				mainInfIndexersCount = 1
			)
			if tc.withNodeMetadata {
				mainInfIndexersCount++
				n, _ = makeDiscoveryWithMetadata(RoleEndpointSlice, NamespaceDiscovery{}, AttachMetadataConfig{Node: true})
			} else {
				n, _ = makeDiscovery(RoleEndpointSlice, NamespaceDiscovery{})
			}

			k8sDiscoveryTest{
				discovery: n,
				afterStart: func() {
					n.RLock()
					defer n.RUnlock()
					require.Len(t, n.discoverers, 1)
					require.Len(t, n.discoverers[0].(*EndpointSlice).endpointSliceInf.GetIndexer().GetIndexers(), mainInfIndexersCount)
				},
			}.Run(t)
		})
	}
}

func TestEndpointSliceDiscoverySidecarContainer(t *testing.T) {
	t.Parallel()
	objs := []runtime.Object{