cleanup

internal/component/discovery/benchmark_data/06_typical_pipeline_grouping.txt

@@ -2,11 +2,11 @@ goos: darwin
 goarch: arm64
 pkg: github.com/grafana/alloy/internal/component/discovery
 cpu: Apple M2
-Benchmark_Targets_TypicalPipeline-8   	      36	  32660016 ns/op	10841145 B/op	   80541 allocs/op
-Benchmark_Targets_TypicalPipeline-8   	      36	  32693987 ns/op	10927902 B/op	   80541 allocs/op
-Benchmark_Targets_TypicalPipeline-8   	      37	  35438566 ns/op	10927976 B/op	   80543 allocs/op
-Benchmark_Targets_TypicalPipeline-8   	      34	  32530555 ns/op	10927663 B/op	   80540 allocs/op
-Benchmark_Targets_TypicalPipeline-8   	      30	  38546656 ns/op	10840045 B/op	   80538 allocs/op
-Benchmark_Targets_TypicalPipeline-8   	      34	  32129576 ns/op	10927397 B/op	   80537 allocs/op
+Benchmark_Targets_TypicalPipeline-8   	      34	  32415304 ns/op	10928191 B/op	   80544 allocs/op
+Benchmark_Targets_TypicalPipeline-8   	      36	  32742291 ns/op	10840570 B/op	   80541 allocs/op
+Benchmark_Targets_TypicalPipeline-8   	      36	  32810744 ns/op	10840857 B/op	   80539 allocs/op
+Benchmark_Targets_TypicalPipeline-8   	      37	  32687316 ns/op	10840767 B/op	   80536 allocs/op
+Benchmark_Targets_TypicalPipeline-8   	      34	  32600847 ns/op	10927485 B/op	   80538 allocs/op
+Benchmark_Targets_TypicalPipeline-8   	      34	  43407786 ns/op	10927321 B/op	   80536 allocs/op
 PASS
-ok  	github.com/grafana/alloy/internal/component/discovery	9.699s
+ok  	github.com/grafana/alloy/internal/component/discovery	8.845s

internal/component/discovery/relabel/relabel.go

@@ -86,11 +86,11 @@ func (c *Component) Update(args component.Arguments) error {
 	for _, t := range newArgs.Targets {
 		var (
 			relabelled discovery.Target
-			builder    = discovery.NewLabelsBuilderFromTarget(t)
+			builder    = discovery.NewTargetBuilderFrom(t)
 			keep       = alloy_relabel.ProcessBuilder(builder, newArgs.RelabelConfigs...)
 		)
 		if keep {
-			relabelled = discovery.NewTargetFromLabelsBuilder(builder)
+			relabelled = builder.Target()
 			targets = append(targets, relabelled)
 		}
 		componentID := livedebugging.ComponentID(c.opts.ID)

internal/component/discovery/target.go

@@ -90,7 +90,6 @@ func NewTargetFromSpecificAndBaseLabelSet(own, group commonlabels.LabelSet) Targ
 
 // NewTargetFromModelLabels creates a target from model Labels.
 // NOTE: this is not optimised and should be avoided on a hot path.
-// TODO(thampiotr): 27% allocs
 // TODO(thampiotr): discovery.relabel
 func NewTargetFromModelLabels(labels modellabels.Labels) Target {
 	l := make(commonlabels.LabelSet, len(labels))
@@ -100,7 +99,6 @@ func NewTargetFromModelLabels(labels modellabels.Labels) Target {
 	return NewTargetFromLabelSet(l)
 }
 
-// TODO(thampiotr): this uses LabelSet, but maybe should use model.Labels?
 func NewTargetFromMap(m map[string]string) Target {
 	l := make(commonlabels.LabelSet, len(m))
 	for k, v := range m {
@@ -109,7 +107,6 @@ func NewTargetFromMap(m map[string]string) Target {
 	return NewTargetFromLabelSet(l)
 }
 
-// TODO(thampiotr): 13% allocs
 // TODO(thampiotr): discovery.relabel
 // TODO(thampiotr): get rid of this one!!
 func (t Target) Labels() modellabels.Labels {

internal/component/discovery/target_builder.go

@@ -8,44 +8,44 @@ import (
 
 const initialLabelsBuilderCapacity = 5
 
-type TargetBuilderTwo interface {
+type TargetBuilder interface {
 	relabel.LabelBuilder
 	Target() Target
-	SetKV(kv ...string) TargetBuilderTwo
+	SetKV(kv ...string) TargetBuilder
 }
 
 // TODO(thampiotr): maybe all targets can have toDel map ready?
-type labelsBuilder struct {
+type targetBuilder struct {
 	group commonlabels.LabelSet
 	own   commonlabels.LabelSet
 
 	toAdd map[string]string
 	toDel map[string]struct{}
 }
 
-func (l labelsBuilder) SetKV(kv ...string) TargetBuilderTwo {
+func (t targetBuilder) SetKV(kv ...string) TargetBuilder {
 	for i := 0; i < len(kv); i += 2 {
-		l.Set(kv[i], kv[i+1])
+		t.Set(kv[i], kv[i+1])
 	}
-	return l
+	return t
 }
 
-// NewLabelsBuilder creates an empty labels builder.
-func NewLabelsBuilder() TargetBuilderTwo {
-	return labelsBuilder{
+// NewTargetBuilder creates an empty labels builder.
+func NewTargetBuilder() TargetBuilder {
+	return targetBuilder{
 		group: nil,
 		own:   make(commonlabels.LabelSet),
 		toAdd: make(map[string]string),
 		toDel: make(map[string]struct{}),
 	}
 }
 
-func NewLabelsBuilderFromTarget(t Target) TargetBuilderTwo {
-	return NewLabelsBuilderFromLabelSets(t.group, t.own)
+func NewTargetBuilderFrom(t Target) TargetBuilder {
+	return NewTargetBuilderFromLabelSets(t.group, t.own)
 }
 
-func NewLabelsBuilderFromLabelSets(group, own commonlabels.LabelSet) TargetBuilderTwo {
-	return labelsBuilder{
+func NewTargetBuilderFromLabelSets(group, own commonlabels.LabelSet) TargetBuilder {
+	return targetBuilder{
 		group: group,
 		own:   own,
 		// TODO(thampiotr): we could postulate that builder is throw-away after .Target() and use object pool for these.
@@ -54,120 +54,103 @@ func NewLabelsBuilderFromLabelSets(group, own commonlabels.LabelSet) TargetBuild
 	}
 }
 
-func NewTargetFromLabelsBuilder(lb relabel.LabelBuilder) Target {
-	// Use optimised path if possible
-	if ilb, ok := lb.(*labelsBuilder); ok {
-		return ilb.Target() // TODO(thampiotr): it could be part of interface and we'd avoid this type checking
-	}
-
-	// TODO(thampiotr): remove this, just checking for now
-	panic("not right - need to optimise this!")
-
-	// Otherwise, non-optimised merge for other implementations.
-	res := make(commonlabels.LabelSet)
-	lb.Range(func(k, v string) {
-		res[commonlabels.LabelName(k)] = commonlabels.LabelValue(v)
-	})
-	return NewTargetFromLabelSet(res)
-}
-
-func (l labelsBuilder) Get(label string) string {
-	if v, ok := l.toAdd[label]; ok {
+func (t targetBuilder) Get(label string) string {
+	if v, ok := t.toAdd[label]; ok {
 		return v
 	}
-	if _, ok := l.toDel[label]; ok {
+	if _, ok := t.toDel[label]; ok {
 		return ""
 	}
-	lv, ok := l.own[commonlabels.LabelName(label)]
+	lv, ok := t.own[commonlabels.LabelName(label)]
 	if ok {
 		return string(lv)
 	}
-	lv, ok = l.group[commonlabels.LabelName(label)]
+	lv, ok = t.group[commonlabels.LabelName(label)]
 	return string(lv)
 }
 
-func (l labelsBuilder) Range(f func(label string, value string)) {
-	for k, v := range l.toAdd {
+func (t targetBuilder) Range(f func(label string, value string)) {
+	for k, v := range t.toAdd {
 		f(k, v)
 	}
-	for k, v := range l.own {
-		if _, deleted := l.toDel[string(k)]; deleted {
+	for k, v := range t.own {
+		if _, deleted := t.toDel[string(k)]; deleted {
 			continue
 		}
 		f(string(k), string(v))
 	}
-	for k, v := range l.group {
-		if _, deleted := l.toDel[string(k)]; deleted {
+	for k, v := range t.group {
+		if _, deleted := t.toDel[string(k)]; deleted {
 			continue
 		}
-		if _, inOwn := l.own[k]; inOwn {
+		if _, inOwn := t.own[k]; inOwn {
 			continue
 		}
 		f(string(k), string(v))
 	}
 }
 
-func (l labelsBuilder) Set(label string, val string) {
-	l.toAdd[label] = val
+func (t targetBuilder) Set(label string, val string) {
+	t.toAdd[label] = val
 }
 
-func (l labelsBuilder) Del(labels ...string) {
+func (t targetBuilder) Del(labels ...string) {
 	for _, label := range labels {
-		l.toDel[label] = struct{}{}
+		t.toDel[label] = struct{}{}
 	}
 }
 
 // TODO(thampiotr): this can be more optimal still...
-func (l labelsBuilder) Target() Target {
-	if len(l.toAdd) == 0 && len(l.toDel) == 0 {
-		return NewTargetFromSpecificAndBaseLabelSet(l.own, l.group)
+func (t targetBuilder) Target() Target {
+	if len(t.toAdd) == 0 && len(t.toDel) == 0 {
+		return NewTargetFromSpecificAndBaseLabelSet(t.own, t.group)
 	}
 	// Figure out if we need to modify own set
 	modifyOwn := false
-	if len(l.toAdd) > 0 { // if there is anything to add
+	if len(t.toAdd) > 0 { // if there is anything to add
 		modifyOwn = true
 	} else {
-		for label := range l.toDel { // if there is anything to delete
-			if _, ok := l.own[commonlabels.LabelName(label)]; ok {
+		for label := range t.toDel { // if there is anything to delete
+			if _, ok := t.own[commonlabels.LabelName(label)]; ok {
 				modifyOwn = true
 				break
 			}
 		}
 	}
 
 	modifyGroup := false
-	for label := range l.toDel { // if there is anything to delete from group
-		if _, ok := l.group[commonlabels.LabelName(label)]; ok {
+	for label := range t.toDel { // if there is anything to delete from group
+		if _, ok := t.group[commonlabels.LabelName(label)]; ok {
 			modifyGroup = true
 			break
 		}
 	}
 
 	var (
-		newOwn   = l.own
-		newGroup = l.group
+		newOwn   = t.own
+		newGroup = t.group
 	)
 
 	if modifyOwn {
 		// TODO(thampiotr): further benchmarking is needed here, but if this is causing a lot of allocation, we could
 		// 					try implementing Target with toAdd and toDel overlays instead.
-		newOwn = make(commonlabels.LabelSet, len(l.own)+len(l.toAdd))
-		for k, v := range l.own {
-			if _, ok := l.toDel[string(k)]; ok {
+		newOwn = make(commonlabels.LabelSet, len(t.own)+len(t.toAdd))
+		for k, v := range t.own {
+			if _, ok := t.toDel[string(k)]; ok {
 				continue
 			}
 			newOwn[k] = v
 		}
-		for k, v := range l.toAdd {
+		for k, v := range t.toAdd {
 			newOwn[commonlabels.LabelName(k)] = commonlabels.LabelValue(v)
 		}
 	}
 	if modifyGroup {
-		// TODO(thampiotr): When relabeling a lot of targets we possibly will produce a lot of l.groups that will
+		// TODO(thampiotr): When relabeling a lot of targets we possibly will produce a lot of t.groups that will
 		//                  all be the same. So maybe we need a step to consolidate them?
-		newGroup = make(commonlabels.LabelSet, len(l.group))
-		for k, v := range l.group {
-			if _, ok := l.toDel[string(k)]; ok {
+		newGroup = make(commonlabels.LabelSet, len(t.group))
+		for k, v := range t.group {
+			if _, ok := t.toDel[string(k)]; ok {
 				continue
 			}
 			newGroup[k] = v

internal/component/discovery/target_test.go

@@ -49,21 +49,21 @@ func TestDecodeMap(t *testing.T) {
 
 func TestTargetBuilder(t *testing.T) {
 	target := NewTargetFromMap(map[string]string{"a": "5", "b": "10"})
-	builder := NewLabelsBuilderFromTarget(target)
+	builder := NewTargetBuilderFrom(target)
 
 	builder.Set("foo", "bar")
 	get, ok := builder.Target().Get("foo")
 	require.True(t, ok)
 	require.Equal(t, "bar", get)
 
-	builder = NewLabelsBuilderFromTarget(target)
+	builder = NewTargetBuilderFrom(target)
 	builder.Del("foo")
 	get, ok = builder.Target().Get("foo")
 	require.False(t, ok)
 	require.Equal(t, "", get)
 
 	// Test setting on empty target (verifies it won't panic)
-	NewLabelsBuilder().Set("foo", "bar")
+	NewTargetBuilder().Set("foo", "bar")
 }
 
 func TestConvertFromNative(t *testing.T) {
@@ -92,7 +92,7 @@ func TestEquals_Basic(t *testing.T) {
 	// NOTE: if we start caching anything as a field, the equality may break. We should test it.
 	t1 := NewTargetFromMap(map[string]string{"hip": "hop", "boom": "bap"})
 	require.Equal(t, 2, t1.Labels().Len())
-	tb := NewLabelsBuilderFromTarget(t1)
+	tb := NewTargetBuilderFrom(t1)
 	tb.Set("boom", "bap")
 	t2 := tb.Target()
 	// This is a way commonly used in tests.
@@ -114,6 +114,7 @@ func TestEquals_Custom(t *testing.T) {
 	)
 	require.NotEqual(t, t1, t2)
 	require.True(t, t1.EqualsTarget(t2))
+	require.True(t, t1.Equals(t2))
 }
 
 func Benchmark_Targets_TypicalPipeline(b *testing.B) {
@@ -165,9 +166,9 @@ func Benchmark_Targets_TypicalPipeline(b *testing.B) {
 
 		// Relabel of targets in discovery.relabel
 		for ind := range targets {
-			builder := NewLabelsBuilderFromTarget(targets[ind])
+			builder := NewTargetBuilderFrom(targets[ind])
 			// would do alloy_relabel.ProcessBuilder here to relabel
-			targets[ind] = NewTargetFromLabelsBuilder(builder)
+			targets[ind] = builder.Target()
 		}
 
 		// discovery.scrape: distributing targets for clustering

internal/runtime/equality/equality.go

@@ -15,6 +15,7 @@ type CustomEquality interface {
 // DeepEqual is a wrapper around reflect.DeepEqual, which first checks if arguments implement CustomEquality. If they
 // do, their Equals method is used for comparison instead of reflect.DeepEqual.
 // For simplicity, DeepEqual requires x and y to be of the same type before calling CustomEquality.Equals.
+// TODO(thampiotr): this will need a lot of tests!!!
 func DeepEqual(x, y any) bool {
 	if x == nil || y == nil {
 		return x == y