ronantakizawa/codereview-bench huggingface dataset:
Repository: prometheus/prometheus
File: cmd/prometheus/main_test.go (Go)

Code before:
			prom := exec.Command(promPath, "-test.main", "--storage.tsdb.wal-segment-size="+tc.size, "--web.listen-address=0.0.0.0:0", "--config.file="+promConfig, "--storage.tsdb.path="+filepath.Join(t.TempDir(), "data"))

			// Log stderr in case of failure.
			stderr, err := prom.StderrPipe()
			require.NoError(t, err)

			// WaitGroup is used to ensure that we don't call t.Log() after the test has finished.
			var wg sync.WaitGroup
			wg.Add(1)
			defer wg.Wait()

			go func() {
				defer wg.Done()
				slurp, _ := io.ReadAll(stderr)
				t.Log(string(slurp))
			}()

			err = prom.Start()
			require.NoError(t, err)

			if tc.exitCode == 0 {
				done := make(chan error, 1)
				go func() { done <- prom.Wait() }()
				select {
				case err := <-done:
					require.Fail(t, fmt.Sprintf("prometheus should be still running: %v", err))
				case <-time.After(startupTime):
					prom.Process.Kill()
					<-done
				}
				return
			}

			err = prom.Wait()
			require.Error(t, err)
			var exitError *exec.ExitError
			require.ErrorAs(t, err, &exitError)
			status := exitError.Sys().(syscall.WaitStatus)
			require.Equal(t, tc.exitCode, status.ExitStatus())
		})
	}
}

func TestMaxBlockChunkSegmentSizeBounds(t *testing.T) {
	if testing.Short() {
		t.Skip("skipping test in short mode.")
	}
	t.Parallel()

	for _, tc := range []struct {
		size     string

Review comment (bug):
Is this what we want? Do you mind simplifying to:

```suggestion
					require.Failf(t, "prometheus should be still running: %v", err)
```



Code after:
			prom := exec.Command(promPath, "-test.main", "--storage.tsdb.wal-segment-size="+tc.size, "--web.listen-address=0.0.0.0:0", "--config.file="+promConfig, "--storage.tsdb.path="+filepath.Join(t.TempDir(), "data"))

			// Log stderr in case of failure.
			stderr, err := prom.StderrPipe()
			require.NoError(t, err)

			// WaitGroup is used to ensure that we don't call t.Log() after the test has finished.
			var wg sync.WaitGroup
			wg.Add(1)
			defer wg.Wait()

			go func() {
				defer wg.Done()
				slurp, _ := io.ReadAll(stderr)
				t.Log(string(slurp))
			}()

			err = prom.Start()
			require.NoError(t, err)

			if tc.exitCode == 0 {
				done := make(chan error, 1)
				go func() { done <- prom.Wait() }()
				select {
				case err := <-done:
					t.Fatalf("prometheus should be still running: %v", err)
				case <-time.After(startupTime):
					prom.Process.Kill()
					<-done
				}
				return
			}

			err = prom.Wait()
			require.Error(t, err)
			var exitError *exec.ExitError
			require.ErrorAs(t, err, &exitError)
			status := exitError.Sys().(syscall.WaitStatus)
			require.Equal(t, tc.exitCode, status.ExitStatus())
		})
	}
}

func TestMaxBlockChunkSegmentSizeBounds(t *testing.T) {
	if testing.Short() {
		t.Skip("skipping test in short mode.")
	}
	t.Parallel()

	for _, tc := range []struct {
		size     string
Repository: prometheus/prometheus
File: discovery/manager_test.go (Go)

Code before:
		t.Fatalf("%q should %s be present in Group labels: %q", label, msg, mergedTargets)
	}
}

func verifyPresence(t *testing.T, tSets map[poolKey]map[string]*targetgroup.Group, poolKey poolKey, label string, present bool) {
	t.Helper()
	_, ok := tSets[poolKey]
	require.True(t, ok, "'%s' should be present in Pool keys: %v", poolKey, tSets)

	match := false
	var mergedTargets string
	for _, targetGroup := range tSets[poolKey] {
		for _, l := range targetGroup.Targets {
			mergedTargets = mergedTargets + " " + l.String()
			if l.String() == label {
				match = true
			}
		}
	}
	if match != present {
		msg := " "
		if !present {
			msg = " not "
		}
		t.Fatalf("%q should%sbe present in Targets labels: %q", label, msg, mergedTargets)
	}
}

func pk(provider, setName string, n int) poolKey {
	return poolKey{
		setName:  setName,
		provider: fmt.Sprintf("%s/%d", provider, n),
	}
}

func TestTargetSetTargetGroupsPresentOnConfigReload(t *testing.T) {
	ctx, cancel := context.WithCancel(context.Background())
	defer cancel()

	reg := prometheus.NewRegistry()
	_, sdMetrics := NewTestMetrics(t, reg)

	discoveryManager := NewManager(ctx, promslog.NewNopLogger(), reg, sdMetrics)
	require.NotNil(t, discoveryManager)
	discoveryManager.updatert = 100 * time.Millisecond
	go discoveryManager.Run()

	c := map[string]Configs{
		"prometheus": {
			staticConfig("foo:9090"),
		},

Review comment (suggestion):
```suggestion
	if present {
		require.Truef(t, match, "%q must be present in Targets labels: %q", label, mergedTargets)
	} else {
		require.Falsef(t, match, "%q must be absent in Targets labels: %q", label, mergedTargets)
	}
```

Code after:
		t.Fatalf("%q should %s be present in Group labels: %q", label, msg, mergedTargets)
	}
}

func verifyPresence(t *testing.T, tSets map[poolKey]map[string]*targetgroup.Group, poolKey poolKey, label string, present bool) {
	t.Helper()
	_, ok := tSets[poolKey]
	require.True(t, ok, "'%s' should be present in Pool keys: %v", poolKey, tSets)

	match := false
	var mergedTargets string
	for _, targetGroup := range tSets[poolKey] {
		for _, l := range targetGroup.Targets {
			mergedTargets = mergedTargets + " " + l.String()
			if l.String() == label {
				match = true
			}
		}
	}
	if present {
		require.Truef(t, match, "%q must be present in Targets labels: %q", label, mergedTargets)
	} else {
		require.Falsef(t, match, "%q must be absent in Targets labels: %q", label, mergedTargets)
	}
}

func pk(provider, setName string, n int) poolKey {
	return poolKey{
		setName:  setName,
		provider: fmt.Sprintf("%s/%d", provider, n),
	}
}

func TestTargetSetTargetGroupsPresentOnConfigReload(t *testing.T) {
	ctx, cancel := context.WithCancel(context.Background())
	defer cancel()

	reg := prometheus.NewRegistry()
	_, sdMetrics := NewTestMetrics(t, reg)

	discoveryManager := NewManager(ctx, promslog.NewNopLogger(), reg, sdMetrics)
	require.NotNil(t, discoveryManager)
	discoveryManager.updatert = 100 * time.Millisecond
	go discoveryManager.Run()

	c := map[string]Configs{
		"prometheus": {
			staticConfig("foo:9090"),
		},
	}
	discoveryManager.ApplyConfig(c)
Repository: prometheus/prometheus
File: storage/remote/otlptranslator/prometheusremotewrite/helper.go (Go)

Code before:
	}

	attributes := resource.Attributes()
	identifyingAttrs := []string{
		conventions.AttributeServiceNamespace,
		conventions.AttributeServiceName,
		conventions.AttributeServiceInstanceID,
	}
	nonIdentifyingAttrsCount := attributes.Len()
	for _, a := range identifyingAttrs {
		_, haveAttr := attributes.Get(a)
		if haveAttr {
			nonIdentifyingAttrsCount--
		}
	}
	if nonIdentifyingAttrsCount == 0 {
		// If we only have job + instance, then target_info isn't useful, so don't add it.
		return
	}

	name := targetMetricName
	if len(settings.Namespace) > 0 {
		name = settings.Namespace + "_" + name
	}

	settings.ResourceAttributesSetting.reset()
	if settings.KeepIdentifyingResourceAttributes {
		// Do not pass identifying attributes as ignoreAttrs below.
		identifyingAttrs = nil
	}
	labels := createAttributes(resource, attributes, settings, identifyingAttrs, false, model.MetricNameLabel, name)
	haveIdentifier := false
	for _, l := range labels {
		if l.Name == model.JobLabel || l.Name == model.InstanceLabel {
			haveIdentifier = true
			break
		}
	}

	if !haveIdentifier {
		// We need at least one identifying label to generate target_info.
		return
	}

	sample := &prompb.Sample{
		Value: float64(1),
		// convert ns to ms
		Timestamp: convertTimeStamp(timestamp),
	}
	converter.addSample(sample, labels)
}

Review comment (refactor):
The `reset` method won't be necessary, with proper abstraction:
```suggestion
	settings.PromoteResourceAttributes = nil
```

Code after:
	for _, a := range identifyingAttrs {
		_, haveAttr := attributes.Get(a)
		if haveAttr {
			nonIdentifyingAttrsCount--
		}
	}
	if nonIdentifyingAttrsCount == 0 {
		// If we only have job + instance, then target_info isn't useful, so don't add it.
		return
	}

	name := targetMetricName
	if len(settings.Namespace) > 0 {
		name = settings.Namespace + "_" + name
	}

	settings.PromoteResourceAttributes = nil
	if settings.KeepIdentifyingResourceAttributes {
		// Do not pass identifying attributes as ignoreAttrs below.
		identifyingAttrs = nil
	}
	labels := createAttributes(resource, attributes, settings, identifyingAttrs, false, model.MetricNameLabel, name)
	haveIdentifier := false
	for _, l := range labels {
		if l.Name == model.JobLabel || l.Name == model.InstanceLabel {
			haveIdentifier = true
			break
		}
	}

	if !haveIdentifier {
		// We need at least one identifying label to generate target_info.
		return
	}

	sample := &prompb.Sample{
		Value: float64(1),
		// convert ns to ms
		Timestamp: convertTimeStamp(timestamp),
	}
	converter.addSample(sample, labels)
}

// convertTimeStamp converts OTLP timestamp in ns to timestamp in ms.
func convertTimeStamp(timestamp pcommon.Timestamp) int64 {
	return int64(timestamp) / 1_000_000
}
Repository: prometheus/prometheus
File: storage/remote/otlptranslator/prometheusremotewrite/metrics_to_prw.go (Go)

Code before:
	}

	return ResourceAttributesSetting{
		Action: PromoteSpecificResourceAttributeAction,
		Attr:   createAttr(otlpCfg.PromoteResourceAttributes),
	}
}

func (s *ResourceAttributesSetting) shouldPromote(name string) bool {
	switch s.Action {
	case PromoteAllExceptIgnoreResourceAttributeAction:
		_, exist := s.Attr[name]
		return !exist
	case PromoteSpecificResourceAttributeAction:
		_, exist := s.Attr[name]
		return exist
	default:
		return false
	}
}

// Not promote anything.
func (s *ResourceAttributesSetting) reset() {
	s.Action = PromoteSpecificResourceAttributeAction
	s.Attr = make(map[string]struct{})
}


Review comment (refactor):
The following is a full abstraction of how promoted resource attributes are determined, and much easier to deal with for the caller:

```suggestion
// promotedAttributes returns labels for promoted resourceAttributes.
func (s *PromoteResourceAttributes) promotedAttributes(resourceAttributes pcommon.Map) []prompb.Label {
	if s == nil {
		return nil
	}

	var promotedAttrs []prompb.Label
	if s.promoteAll {
		promotedAttrs = make([]prompb.Label, 0, resourceAttributes.Len())
		resourceAttributes.Range(func(name string, value pcommon.Value) bool {
			if _, exists := s.attrs[name]; !exists {
				promotedAttrs = append(promotedAttrs, prompb.Label{Name: name, Value: value.AsString()})
			}
			return true
		})
	} else {
		promotedAttrs = make([]prompb.Label, 0, len(s.attrs))
		resourceAttributes.Range(func(name string, value pcommon.Value) bool {
			if _, exists := s.attrs[name]; exists {
				promotedAttrs = append(promotedAttrs, prompb.Label{Name: name, Value: value.AsString()})
			}
			return true
		})
	}
	sort.Stable(ByLabelName(promotedAttrs))
	return promotedAttrs
}
```

Code after:
	}
	return &PromoteResourceAttributes{
		promoteAll: otlpCfg.PromoteAllResourceAttributes,
		attrs:      attrsMap,
	}
}

// promotedAttributes returns labels for promoted resourceAttributes.
func (s *PromoteResourceAttributes) promotedAttributes(resourceAttributes pcommon.Map) []prompb.Label {
	if s == nil {
		return nil
	}

	var promotedAttrs []prompb.Label
	if s.promoteAll {
		promotedAttrs = make([]prompb.Label, 0, resourceAttributes.Len())
		resourceAttributes.Range(func(name string, value pcommon.Value) bool {
			if _, exists := s.attrs[name]; !exists {
				promotedAttrs = append(promotedAttrs, prompb.Label{Name: name, Value: value.AsString()})
			}
			return true
		})
	} else {
		promotedAttrs = make([]prompb.Label, 0, len(s.attrs))
		resourceAttributes.Range(func(name string, value pcommon.Value) bool {
			if _, exists := s.attrs[name]; exists {
				promotedAttrs = append(promotedAttrs, prompb.Label{Name: name, Value: value.AsString()})
			}
			return true
		})
	}
	sort.Stable(ByLabelName(promotedAttrs))
	return promotedAttrs
}
Repository: prometheus/prometheus
File: config/config.go (Go)

Code before:
	*c = DefaultOTLPConfig
	type plain OTLPConfig
	if err := unmarshal((*plain)(c)); err != nil {
		return err
	}

	if c.PromoteAllResourceAttributes {
		if len(c.PromoteResourceAttributes) > 0 {
			return errors.New("'promote_all_resource_attributes' and 'promote_resource_attributes' cannot be configured simultaneously")
		}
		if err := sanitizeAttributes(c.IgnoreResourceAttributes); err != nil {
			return fmt.Errorf("invalid 'ignore_resource_attributes': %w", err)
		}
	} else {
		if len(c.IgnoreResourceAttributes) > 0 {
			return errors.New("'promote_resource_attributes' and 'ignore_resource_attributes' cannot be configured simultaneously")
		}
		if err := sanitizeAttributes(c.PromoteResourceAttributes); err != nil {
			return fmt.Errorf("invalid 'promote_resource_attributes': %w", err)
		}
	}

	return nil
}

func sanitizeAttributes(attributes []string) error {
	seen := map[string]struct{}{}
	var err error
	for i, attr := range attributes {
		attr = strings.TrimSpace(attr)
		if attr == "" {
			err = errors.Join(err, errors.New("empty OTel resource attribute"))
			continue
		}
		if _, exists := seen[attr]; exists {
			err = errors.Join(err, fmt.Errorf("duplicated OTel resource attribute %q", attr))
			continue
		}

		seen[attr] = struct{}{}
		attributes[i] = attr
	}
	return err
}


Review comment (security):
I realize now that you are making the error messages here less descriptive and thus less helpful than before:
```suggestion
func sanitizeAttributes(attributes []string, adjective string) error {
```

Code after:
	*c = DefaultOTLPConfig
	type plain OTLPConfig
	if err := unmarshal((*plain)(c)); err != nil {
		return err
	}

	if c.PromoteAllResourceAttributes {
		if len(c.PromoteResourceAttributes) > 0 {
			return errors.New("'promote_all_resource_attributes' and 'promote_resource_attributes' cannot be configured simultaneously")
		}
		if err := sanitizeAttributes(c.IgnoreResourceAttributes, "ignored"); err != nil {
			return fmt.Errorf("invalid 'ignore_resource_attributes': %w", err)
		}
	} else {
		if len(c.IgnoreResourceAttributes) > 0 {
			return errors.New("'ignore_resource_attributes' cannot be configured unless 'promote_all_resource_attributes' is true")
		}
		if err := sanitizeAttributes(c.PromoteResourceAttributes, "promoted"); err != nil {
			return fmt.Errorf("invalid 'promote_resource_attributes': %w", err)
		}
	}

	return nil
}

func sanitizeAttributes(attributes []string, adjective string) error {
	seen := map[string]struct{}{}
	var err error
	for i, attr := range attributes {
		attr = strings.TrimSpace(attr)
		if attr == "" {
			err = errors.Join(err, fmt.Errorf("empty %s OTel resource attribute", adjective))
			continue
		}
		if _, exists := seen[attr]; exists {
			err = errors.Join(err, fmt.Errorf("duplicated %s OTel resource attribute %q", adjective, attr))
			continue
		}

		seen[attr] = struct{}{}
		attributes[i] = attr
	}
	return err
}
Repository: prometheus/prometheus
File: config/config.go (Go)

Code before:
	if c.PromoteAllResourceAttributes {
		if len(c.PromoteResourceAttributes) > 0 {
			return errors.New("'promote_all_resource_attributes' and 'promote_resource_attributes' cannot be configured simultaneously")
		}
		if err := sanitizeAttributes(c.IgnoreResourceAttributes); err != nil {
			return fmt.Errorf("invalid 'ignore_resource_attributes': %w", err)
		}
	} else {
		if len(c.IgnoreResourceAttributes) > 0 {
			return errors.New("'promote_resource_attributes' and 'ignore_resource_attributes' cannot be configured simultaneously")
		}
		if err := sanitizeAttributes(c.PromoteResourceAttributes); err != nil {
			return fmt.Errorf("invalid 'promote_resource_attributes': %w", err)
		}
	}

	return nil
}

func sanitizeAttributes(attributes []string) error {
	seen := map[string]struct{}{}
	var err error
	for i, attr := range attributes {
		attr = strings.TrimSpace(attr)
		if attr == "" {
			err = errors.Join(err, errors.New("empty OTel resource attribute"))
			continue
		}
		if _, exists := seen[attr]; exists {
			err = errors.Join(err, fmt.Errorf("duplicated OTel resource attribute %q", attr))
			continue
		}

		seen[attr] = struct{}{}
		attributes[i] = attr
	}
	return err
}


Review comment (suggestion):
```suggestion
			err = errors.Join(err, fmt.Errorf("empty %s OTel resource attribute", adjective))
```

Code after:
	if c.PromoteAllResourceAttributes {
		if len(c.PromoteResourceAttributes) > 0 {
			return errors.New("'promote_all_resource_attributes' and 'promote_resource_attributes' cannot be configured simultaneously")
		}
		if err := sanitizeAttributes(c.IgnoreResourceAttributes, "ignored"); err != nil {
			return fmt.Errorf("invalid 'ignore_resource_attributes': %w", err)
		}
	} else {
		if len(c.IgnoreResourceAttributes) > 0 {
			return errors.New("'ignore_resource_attributes' cannot be configured unless 'promote_all_resource_attributes' is true")
		}
		if err := sanitizeAttributes(c.PromoteResourceAttributes, "promoted"); err != nil {
			return fmt.Errorf("invalid 'promote_resource_attributes': %w", err)
		}
	}

	return nil
}

func sanitizeAttributes(attributes []string, adjective string) error {
	seen := map[string]struct{}{}
	var err error
	for i, attr := range attributes {
		attr = strings.TrimSpace(attr)
		if attr == "" {
			err = errors.Join(err, fmt.Errorf("empty %s OTel resource attribute", adjective))
			continue
		}
		if _, exists := seen[attr]; exists {
			err = errors.Join(err, fmt.Errorf("duplicated %s OTel resource attribute %q", adjective, attr))
			continue
		}

		seen[attr] = struct{}{}
		attributes[i] = attr
	}
	return err
}
Repository: prometheus/prometheus
File: config/config.go (Go)

Code before:
		if err := sanitizeAttributes(c.IgnoreResourceAttributes); err != nil {
			return fmt.Errorf("invalid 'ignore_resource_attributes': %w", err)
		}
	} else {
		if len(c.IgnoreResourceAttributes) > 0 {
			return errors.New("'promote_resource_attributes' and 'ignore_resource_attributes' cannot be configured simultaneously")
		}
		if err := sanitizeAttributes(c.PromoteResourceAttributes); err != nil {
			return fmt.Errorf("invalid 'promote_resource_attributes': %w", err)
		}
	}

	return nil
}

func sanitizeAttributes(attributes []string) error {
	seen := map[string]struct{}{}
	var err error
	for i, attr := range attributes {
		attr = strings.TrimSpace(attr)
		if attr == "" {
			err = errors.Join(err, errors.New("empty OTel resource attribute"))
			continue
		}
		if _, exists := seen[attr]; exists {
			err = errors.Join(err, fmt.Errorf("duplicated OTel resource attribute %q", attr))
			continue
		}

		seen[attr] = struct{}{}
		attributes[i] = attr
	}
	return err
}


Review comment (refactor):
```suggestion
			err = errors.Join(err, fmt.Errorf("duplicated %s OTel resource attribute %q", adjective, attr))
```

Code after:
		if err := sanitizeAttributes(c.IgnoreResourceAttributes, "ignored"); err != nil {
			return fmt.Errorf("invalid 'ignore_resource_attributes': %w", err)
		}
	} else {
		if len(c.IgnoreResourceAttributes) > 0 {
			return errors.New("'ignore_resource_attributes' cannot be configured unless 'promote_all_resource_attributes' is true")
		}
		if err := sanitizeAttributes(c.PromoteResourceAttributes, "promoted"); err != nil {
			return fmt.Errorf("invalid 'promote_resource_attributes': %w", err)
		}
	}

	return nil
}

func sanitizeAttributes(attributes []string, adjective string) error {
	seen := map[string]struct{}{}
	var err error
	for i, attr := range attributes {
		attr = strings.TrimSpace(attr)
		if attr == "" {
			err = errors.Join(err, fmt.Errorf("empty %s OTel resource attribute", adjective))
			continue
		}
		if _, exists := seen[attr]; exists {
			err = errors.Join(err, fmt.Errorf("duplicated %s OTel resource attribute %q", adjective, attr))
			continue
		}

		seen[attr] = struct{}{}
		attributes[i] = attr
	}
	return err
}
Repository: prometheus/prometheus
File: storage/remote/otlptranslator/prometheusremotewrite/helper_test.go (Go)

Code before:
					Value: "service ID",
				},
				{
					Name:  "job",
					Value: "service name",
				},
				{
					Name:  "existent_attr",
					Value: "resource value",
				},
				{
					Name:  "metric_attr",
					Value: "metric value",
				},
				{
					Name:  "metric_attr_other",
					Value: "metric value other",
				},
			},
		},
		{
			name: "Successful conversion promoting all resource attribute",
			promoteResourceAttributes: &PromoteResourceAttributes{
				promoteAll: true,
				attrs:      map[string]struct{}{},
			},
			expectedLabels: []prompb.Label{
				{
					Name:  "__name__",
					Value: "test_metric",
				},
				{
					Name:  "instance",
					Value: "service ID",
				},
				{
					Name:  "job",
					Value: "service name",
				},
				{
					Name:  "existent_attr",
					Value: "resource value",
				},
				{
					Name:  "metric_attr",
					Value: "metric value",
				},
				{
					Name:  "metric_attr_other",
					Value: "metric value other",
				},

Review comment (suggestion):
```suggestion
			promoteAllResourceAttributes: true,
```

Code after:
			expectedLabels: []prompb.Label{
				{
					Name:  "__name__",
					Value: "test_metric",
				},
				{
					Name:  "instance",
					Value: "service ID",
				},
				{
					Name:  "job",
					Value: "service name",
				},
				{
					Name:  "existent_attr",
					Value: "resource value",
				},
				{
					Name:  "metric_attr",
					Value: "metric value",
				},
				{
					Name:  "metric_attr_other",
					Value: "metric value other",
				},
				{
					Name:  "service_name",
					Value: "service name",
				},
				{
					Name:  "service_instance_id",
					Value: "service ID",
				},
			},
		},
		{
			name:                         "Successful conversion promoting all resource attributes, ignoring 'service.instance.id'",
			promoteAllResourceAttributes: true,
			ignoreResourceAttributes: []string{
				"service.instance.id",
			},
			expectedLabels: []prompb.Label{
				{
					Name:  "__name__",
					Value: "test_metric",
				},
				{
					Name:  "instance",
					Value: "service ID",
				},
				{