From b370795bbf1f7b4a1c7fa3da81dc9bf6b03b58f5 Mon Sep 17 00:00:00 2001
From: Piotr <17101802+thampiotr@users.noreply.github.com>
Date: Tue, 28 Jan 2025 16:02:53 +0000
Subject: [PATCH] cleanup

---
 internal/component/discovery/discovery.go        |  1 -
 internal/component/discovery/target.go           | 16 ++++++----------
 internal/component/discovery/target_test.go      |  4 ++--
 .../controller/node_builtin_component.go         |  1 -
 4 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/internal/component/discovery/discovery.go b/internal/component/discovery/discovery.go
index 8f2081a136..9be7520312 100644
--- a/internal/component/discovery/discovery.go
+++ b/internal/component/discovery/discovery.go
@@ -228,7 +228,6 @@ func toAlloyTargets(cache map[string]*targetgroup.Group) []Target {
 
 	for _, group := range cache {
 		for _, target := range group.Targets {
-			// TODO(thampiotr): Merge 20% allocs
 			allTargets = append(allTargets, NewTargetFromSpecificAndBaseLabelSet(target, group.Labels))
 		}
 	}
diff --git a/internal/component/discovery/target.go b/internal/component/discovery/target.go
index 476322ed95..fb2842b547 100644
--- a/internal/component/discovery/target.go
+++ b/internal/component/discovery/target.go
@@ -16,6 +16,8 @@ type Target struct {
 	// Prometheus codebase (even for logs, we have loki.relabel) and this representation helps reduce
 	// unnecessary conversions and allocations. We can add another internal representations in the future if needed.
 	labelSet commonlabels.LabelSet
+	// NOTE: it is essential that equality between targets continues to work as it is used by Alloy runtime to
+	// decide whether updates need to be propagated throughout the pipeline. See tests.
 }
 
 var (
@@ -40,7 +42,7 @@ func NewTargetFromLabelSet(targetLabels commonlabels.LabelSet) Target {
 	}
 }
 
-// TODO(thampiotr): 37% allocs
+// TODO(thampiotr): 27% allocs
 // TODO(thampiotr): discovery.*
 func NewTargetFromSpecificAndBaseLabelSet(specific, base commonlabels.LabelSet) Target {
 	merged := make(commonlabels.LabelSet, len(specific)+len(base))
@@ -53,7 +55,7 @@ func NewTargetFromSpecificAndBaseLabelSet(specific, base commonlabels.LabelSet)
 	return NewTargetFromLabelSet(merged)
 }
 
-// TODO(thampiotr): 23% allocs
+// TODO(thampiotr): 27% allocs
 // TODO(thampiotr): discovery.relabel
 func NewTargetFromModelLabels(labels modellabels.Labels) Target {
 	// TODO(thampiotr): save labels as cached value?
@@ -74,14 +76,14 @@ func NewTargetFromMap(m map[string]string) Target {
 	return NewTargetFromLabelSet(l)
 }
 
-// TODO(thampiotr): 12% allocs
+// TODO(thampiotr): 13% allocs
 // TODO(thampiotr): discovery.relabel
 func (t Target) Labels() modellabels.Labels {
 	// TODO(thampiotr): We can cache this!
 	return t.LabelsWithPredicate(nil)
 }
 
-// TODO(thampiotr): 11% allocs
+// TODO(thampiotr): 13% allocs
 // TODO(thampiotr): prometheus.scrape / distributed_targets
 func (t Target) NonMetaLabels() modellabels.Labels {
 	return t.LabelsWithPredicate(func(key string) bool {
@@ -174,12 +176,6 @@ func (t Target) Len() int {
 	return len(t.labelSet)
 }
 
-// Equals should be called to compare two Target objects.
-// TODO(thampiotr): make sure this is called when Alloy is deciding whether to propagate updates
-func (t Target) Equals(other Target) bool {
-	return t.labelSet.Equal(other.labelSet)
-}
-
 func (t Target) Clone() Target {
 	return Target{
 		labelSet: t.labelSet.Clone(),
diff --git a/internal/component/discovery/target_test.go b/internal/component/discovery/target_test.go
index e8a4d256c2..6980e75c3d 100644
--- a/internal/component/discovery/target_test.go
+++ b/internal/component/discovery/target_test.go
@@ -33,7 +33,7 @@ func TestDecodeMap(t *testing.T) {
 	require.NoError(t, eval.Evaluate(scope, &actual))
 	require.Equal(t, expected, actual)
 
-	// Test can use it like a map
+	// Test can iterate over it
 	var seen []string
 	actual.ForEachLabel(func(k string, v string) bool {
 		seen = append(seen, fmt.Sprintf("%s=%s", k, v))
@@ -84,8 +84,8 @@ func TestConvertFromNative(t *testing.T) {
 }
 
 func TestEquals(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"})
-	// TODO(thampiotr): if we start caching this as a field, the equality may break.
 	require.Equal(t, 2, t1.Labels().Len())
 	t2 := NewTargetFromMap(map[string]string{"hip": "hop"})
 	t2.Set("boom", "bap")
diff --git a/internal/runtime/internal/controller/node_builtin_component.go b/internal/runtime/internal/controller/node_builtin_component.go
index 09fe664eb8..43e4893a2f 100644
--- a/internal/runtime/internal/controller/node_builtin_component.go
+++ b/internal/runtime/internal/controller/node_builtin_component.go
@@ -371,7 +371,6 @@ func (cn *BuiltinComponentNode) setExports(e component.Exports) {
 	var changed bool
 
 	cn.exportsMut.Lock()
-	// TODO(thampiotr): If we export a capsule and don't want to trigger updates, we would want to have a customisable way of comparing them to each other here.
 	if !reflect.DeepEqual(cn.exports, e) {
 		changed = true
 		cn.exports = e