From c82e8211171f3ff623a8cb2a2645766f9daa6c64 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?=
 <mail+sumo@mikolajswiatek.com>
Date: Sun, 12 May 2024 17:57:52 +0200
Subject: [PATCH] Use a different struct for scrape target serialization

---
 cmd/otel-allocator/server/bench_test.go  | 13 +++++-
 cmd/otel-allocator/server/server.go      | 50 +++++++++++++++++++-----
 cmd/otel-allocator/server/server_test.go | 28 ++++++-------
 cmd/otel-allocator/target/target.go      | 18 ++-------
 4 files changed, 69 insertions(+), 40 deletions(-)

diff --git a/cmd/otel-allocator/server/bench_test.go b/cmd/otel-allocator/server/bench_test.go
index 8fcea90b0e..b4f24f480d 100644
--- a/cmd/otel-allocator/server/bench_test.go
+++ b/cmd/otel-allocator/server/bench_test.go
@@ -198,7 +198,7 @@ func BenchmarkTargetItemsJSONHandler(b *testing.B) {
 		},
 	}
 	for _, tc := range tests {
-		data := makeNTargetItems(*random, tc.numTargets, tc.numLabels)
+		data := makeNTargetJSON(*random, tc.numTargets, tc.numLabels)
 		b.Run(fmt.Sprintf("%d_targets_%d_labels", tc.numTargets, tc.numLabels), func(b *testing.B) {
 			b.ReportAllocs()
 			for i := 0; i < b.N; i++ {
@@ -242,7 +242,7 @@ func makeNCollectorJSON(random rand.Rand, numCollectors, numItems int) map[strin
 	for i := 0; i < numCollectors; i++ {
 		items[randSeq(random, 20)] = collectorJSON{
 			Link: randSeq(random, 120),
-			Jobs: makeNTargetItems(random, numItems, 50),
+			Jobs: makeNTargetJSON(random, numItems, 50),
 		}
 	}
 	return items
@@ -261,6 +261,15 @@ func makeNTargetItems(random rand.Rand, numItems, numLabels int) []*target.Item
 	return items
 }
 
+func makeNTargetJSON(random rand.Rand, numItems, numLabels int) []*targetJSON {
+	items := makeNTargetItems(random, numItems, numLabels)
+	targets := make([]*targetJSON, numItems)
+	for i := 0; i < numItems; i++ {
+		targets[i] = targetJsonFromTargetItem(items[i])
+	}
+	return targets
+}
+
 func makeNNewLabels(random rand.Rand, n int) model.LabelSet {
 	labels := make(map[model.LabelName]model.LabelValue, n)
 	for i := 0; i < n; i++ {
diff --git a/cmd/otel-allocator/server/server.go b/cmd/otel-allocator/server/server.go
index 33e845103f..cb5e1d1873 100644
--- a/cmd/otel-allocator/server/server.go
+++ b/cmd/otel-allocator/server/server.go
@@ -34,6 +34,7 @@ import (
 	"github.com/prometheus/client_golang/prometheus/promauto"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	promcommconfig "github.com/prometheus/common/config"
+	"github.com/prometheus/common/model"
 	promconfig "github.com/prometheus/prometheus/config"
 	"gopkg.in/yaml.v2"
 
@@ -57,8 +58,17 @@ var (
 )
 
 type collectorJSON struct {
-	Link string         `json:"_link"`
-	Jobs []*target.Item `json:"targets"`
+	Link string        `json:"_link"`
+	Jobs []*targetJSON `json:"targets"`
+}
+
+type linkJSON struct {
+	Link string `json:"_link"`
+}
+
+type targetJSON struct {
+	TargetURL []string       `json:"targets"`
+	Labels    model.LabelSet `json:"labels"`
 }
 
 type Server struct {
@@ -263,9 +273,9 @@ func (s *Server) ReadinessProbeHandler(c *gin.Context) {
 }
 
 func (s *Server) JobHandler(c *gin.Context) {
-	displayData := make(map[string]target.LinkJSON)
+	displayData := make(map[string]linkJSON)
 	for _, v := range s.allocator.TargetItems() {
-		displayData[v.JobName] = target.LinkJSON{Link: v.Link.Link}
+		displayData[v.JobName] = linkJSON{Link: fmt.Sprintf("/jobs/%s/targets", url.QueryEscape(v.JobName))}
 	}
 	s.jsonHandler(c.Writer, displayData)
 }
@@ -294,16 +304,16 @@ func (s *Server) TargetsHandler(c *gin.Context) {
 	if len(q) == 0 {
 		displayData := GetAllTargetsByJob(s.allocator, jobId)
 		s.jsonHandler(c.Writer, displayData)
-
 	} else {
-		tgs := s.allocator.GetTargetsForCollectorAndJob(q[0], jobId)
+		targets := GetAllTargetsByCollectorAndJob(s.allocator, q[0], jobId)
 		// Displays empty list if nothing matches
-		if len(tgs) == 0 {
+		if len(targets) == 0 {
 			s.jsonHandler(c.Writer, []interface{}{})
 			return
 		}
-		s.jsonHandler(c.Writer, tgs)
+		s.jsonHandler(c.Writer, targets)
 	}
+
 }
 
 func (s *Server) errorHandler(w http.ResponseWriter, err error) {
@@ -323,12 +333,25 @@ func (s *Server) jsonHandler(w http.ResponseWriter, data interface{}) {
 func GetAllTargetsByJob(allocator allocation.Allocator, job string) map[string]collectorJSON {
 	displayData := make(map[string]collectorJSON)
 	for _, col := range allocator.Collectors() {
-		items := allocator.GetTargetsForCollectorAndJob(col.Name, job)
-		displayData[col.Name] = collectorJSON{Link: fmt.Sprintf("/jobs/%s/targets?collector_id=%s", url.QueryEscape(job), col.Name), Jobs: items}
+		targets := GetAllTargetsByCollectorAndJob(allocator, col.Name, job)
+		displayData[col.Name] = collectorJSON{
+			Link: fmt.Sprintf("/jobs/%s/targets?collector_id=%s", url.QueryEscape(job), col.Name),
+			Jobs: targets,
+		}
 	}
 	return displayData
 }
 
+// GetAllTargetsByCollector returns all the targets for a given collector and job.
+func GetAllTargetsByCollectorAndJob(allocator allocation.Allocator, collectorName string, jobName string) []*targetJSON {
+	items := allocator.GetTargetsForCollectorAndJob(collectorName, jobName)
+	targets := make([]*targetJSON, len(items))
+	for i, item := range items {
+		targets[i] = targetJsonFromTargetItem(item)
+	}
+	return targets
+}
+
 // registerPprof registers the pprof handlers and either serves the requested
 // specific profile or falls back to index handler.
 func registerPprof(g *gin.RouterGroup) {
@@ -348,3 +371,10 @@ func registerPprof(g *gin.RouterGroup) {
 		}
 	})
 }
+
+func targetJsonFromTargetItem(item *target.Item) *targetJSON {
+	return &targetJSON{
+		TargetURL: item.TargetURL,
+		Labels:    item.Labels,
+	}
+}
diff --git a/cmd/otel-allocator/server/server_test.go b/cmd/otel-allocator/server/server_test.go
index 88b8ad9368..b7f9ad73b5 100644
--- a/cmd/otel-allocator/server/server_test.go
+++ b/cmd/otel-allocator/server/server_test.go
@@ -74,7 +74,7 @@ func TestServer_TargetsHandler(t *testing.T) {
 		allocator allocation.Allocator
 	}
 	type want struct {
-		items     []*target.Item
+		items     []*targetJSON
 		errString string
 	}
 	tests := []struct {
@@ -91,7 +91,7 @@ func TestServer_TargetsHandler(t *testing.T) {
 				allocator: leastWeighted,
 			},
 			want: want{
-				items: []*target.Item{},
+				items: []*targetJSON{},
 			},
 		},
 		{
@@ -105,7 +105,7 @@ func TestServer_TargetsHandler(t *testing.T) {
 				allocator: leastWeighted,
 			},
 			want: want{
-				items: []*target.Item{
+				items: []*targetJSON{
 					{
 						TargetURL: []string{"test-url"},
 						Labels: map[model.LabelName]model.LabelValue{
@@ -127,7 +127,7 @@ func TestServer_TargetsHandler(t *testing.T) {
 				allocator: leastWeighted,
 			},
 			want: want{
-				items: []*target.Item{
+				items: []*targetJSON{
 					{
 						TargetURL: []string{"test-url"},
 						Labels: map[model.LabelName]model.LabelValue{
@@ -149,7 +149,7 @@ func TestServer_TargetsHandler(t *testing.T) {
 				allocator: leastWeighted,
 			},
 			want: want{
-				items: []*target.Item{
+				items: []*targetJSON{
 					{
 						TargetURL: []string{"test-url"},
 						Labels: map[model.LabelName]model.LabelValue{
@@ -186,7 +186,7 @@ func TestServer_TargetsHandler(t *testing.T) {
 				assert.EqualError(t, err, tt.want.errString)
 				return
 			}
-			var itemResponse []*target.Item
+			var itemResponse []*targetJSON
 			err = json.Unmarshal(bodyBytes, &itemResponse)
 			assert.NoError(t, err)
 			assert.ElementsMatch(t, tt.want.items, itemResponse)
@@ -555,19 +555,19 @@ func TestServer_JobHandler(t *testing.T) {
 		description  string
 		targetItems  map[string]*target.Item
 		expectedCode int
-		expectedJobs map[string]target.LinkJSON
+		expectedJobs map[string]linkJSON
 	}{
 		{
 			description:  "nil jobs",
 			targetItems:  nil,
 			expectedCode: http.StatusOK,
-			expectedJobs: make(map[string]target.LinkJSON),
+			expectedJobs: make(map[string]linkJSON),
 		},
 		{
 			description:  "empty jobs",
 			targetItems:  map[string]*target.Item{},
 			expectedCode: http.StatusOK,
-			expectedJobs: make(map[string]target.LinkJSON),
+			expectedJobs: make(map[string]linkJSON),
 		},
 		{
 			description: "one job",
@@ -575,7 +575,7 @@ func TestServer_JobHandler(t *testing.T) {
 				"targetitem": target.NewItem("job1", "", model.LabelSet{}, ""),
 			},
 			expectedCode: http.StatusOK,
-			expectedJobs: map[string]target.LinkJSON{
+			expectedJobs: map[string]linkJSON{
 				"job1": newLink("job1"),
 			},
 		},
@@ -588,7 +588,7 @@ func TestServer_JobHandler(t *testing.T) {
 				"d": target.NewItem("job3", "", model.LabelSet{}, ""),
 				"e": target.NewItem("job3", "", model.LabelSet{}, "")},
 			expectedCode: http.StatusOK,
-			expectedJobs: map[string]target.LinkJSON{
+			expectedJobs: map[string]linkJSON{
 				"job1": newLink("job1"),
 				"job2": newLink("job2"),
 				"job3": newLink("job3"),
@@ -609,7 +609,7 @@ func TestServer_JobHandler(t *testing.T) {
 			assert.Equal(t, tc.expectedCode, result.StatusCode)
 			bodyBytes, err := io.ReadAll(result.Body)
 			require.NoError(t, err)
-			jobs := map[string]target.LinkJSON{}
+			jobs := map[string]linkJSON{}
 			err = json.Unmarshal(bodyBytes, &jobs)
 			require.NoError(t, err)
 			assert.Equal(t, tc.expectedJobs, jobs)
@@ -737,6 +737,6 @@ func TestServer_ScrapeConfigRespose(t *testing.T) {
 	}
 }
 
-func newLink(jobName string) target.LinkJSON {
-	return target.LinkJSON{Link: fmt.Sprintf("/jobs/%s/targets", url.QueryEscape(jobName))}
+func newLink(jobName string) linkJSON {
+	return linkJSON{Link: fmt.Sprintf("/jobs/%s/targets", url.QueryEscape(jobName))}
 }
diff --git a/cmd/otel-allocator/target/target.go b/cmd/otel-allocator/target/target.go
index 3341560329..ce80450088 100644
--- a/cmd/otel-allocator/target/target.go
+++ b/cmd/otel-allocator/target/target.go
@@ -15,9 +15,6 @@
 package target
 
 import (
-	"fmt"
-	"net/url"
-
 	"github.com/prometheus/common/model"
 )
 
@@ -34,17 +31,11 @@ var (
 	endpointSliceTargetNameLabel model.LabelName = "__meta_kubernetes_endpointslice_address_target_name"
 )
 
-// LinkJSON This package contains common structs and methods that relate to scrape targets.
-type LinkJSON struct {
-	Link string `json:"_link"`
-}
-
 type Item struct {
-	JobName       string         `json:"-"`
-	Link          LinkJSON       `json:"-"`
-	TargetURL     []string       `json:"targets"`
-	Labels        model.LabelSet `json:"labels"`
-	CollectorName string         `json:"-"`
+	JobName       string
+	TargetURL     []string
+	Labels        model.LabelSet
+	CollectorName string
 	hash          string
 }
 
@@ -73,7 +64,6 @@ func (t *Item) GetNodeName() string {
 func NewItem(jobName string, targetURL string, label model.LabelSet, collectorName string) *Item {
 	return &Item{
 		JobName:       jobName,
-		Link:          LinkJSON{Link: fmt.Sprintf("/jobs/%s/targets", url.QueryEscape(jobName))},
 		hash:          jobName + targetURL + label.Fingerprint().String(),
 		TargetURL:     []string{targetURL},
 		Labels:        label,