From ded4c2c5b2fcf23af93d841024ada6ea3ddc1fbb Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Fri, 6 Jan 2023 00:13:37 -0800 Subject: [PATCH] Revert "[target-allocator] marshal with jsoniter (#1323)" (#1351) This reverts commit 74581ea322aa7a24d412bb11d3f6ff89934ea33e. --- .chloggen/use-json-iterator.yaml | 17 -- cmd/otel-allocator/go.mod | 2 +- cmd/otel-allocator/server/server.go | 53 ++---- cmd/otel-allocator/server/server_test.go | 221 +---------------------- go.mod | 4 +- 5 files changed, 31 insertions(+), 266 deletions(-) delete mode 100755 .chloggen/use-json-iterator.yaml diff --git a/.chloggen/use-json-iterator.yaml b/.chloggen/use-json-iterator.yaml deleted file mode 100755 index 8eefa84e57..0000000000 --- a/.chloggen/use-json-iterator.yaml +++ /dev/null @@ -1,17 +0,0 @@ -# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' -change_type: enhancement - -# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action) -component: Target Allocator - -# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: Use json-iter to marshal json - -# One or more tracking issues related to the change -issues: - - 1336 - -# (Optional) One or more lines of additional information to render under the primary note. -# These lines will be padded with 2 spaces and then inserted directly into the document. -# Use pipe (|) for multiline entries. -subtext: diff --git a/cmd/otel-allocator/go.mod b/cmd/otel-allocator/go.mod index 14d9bfc439..ecd00cf117 100644 --- a/cmd/otel-allocator/go.mod +++ b/cmd/otel-allocator/go.mod @@ -10,7 +10,6 @@ require ( github.com/go-kit/log v0.2.1 github.com/go-logr/logr v1.2.3 github.com/gorilla/mux v1.8.0 - github.com/json-iterator/go v1.1.12 github.com/mitchellh/hashstructure v1.1.0 github.com/oklog/run v1.1.0 github.com/prometheus-operator/prometheus-operator v0.53.1 @@ -112,6 +111,7 @@ require ( github.com/jmespath/go-jmespath v0.4.0 // indirect github.com/josharian/intern v1.0.0 // indirect github.com/jpillora/backoff v1.0.0 // indirect + github.com/json-iterator/go v1.1.12 // indirect github.com/kolo/xmlrpc v0.0.0-20201022064351-38db28db192b // indirect github.com/linode/linodego v1.8.0 // indirect github.com/mailru/easyjson v0.7.7 // indirect diff --git a/cmd/otel-allocator/server/server.go b/cmd/otel-allocator/server/server.go index 68661bac79..61807b0052 100644 --- a/cmd/otel-allocator/server/server.go +++ b/cmd/otel-allocator/server/server.go @@ -16,19 +16,20 @@ package server import ( "context" + "encoding/json" "fmt" "net/http" "net/url" "time" + yaml2 "github.com/ghodss/yaml" "github.com/go-logr/logr" "github.com/gorilla/mux" - jsoniter "github.com/json-iterator/go" "github.com/mitchellh/hashstructure" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" "github.com/prometheus/client_golang/prometheus/promhttp" - promconfig "github.com/prometheus/prometheus/config" + "gopkg.in/yaml.v2" "github.com/open-telemetry/opentelemetry-operator/cmd/otel-allocator/allocation" "github.com/open-telemetry/opentelemetry-operator/cmd/otel-allocator/target" @@ -41,49 +42,27 @@ var ( }, []string{"path"}) ) -var ( - yamlConfig = jsoniter.Config{ - EscapeHTML: false, - MarshalFloatWith6Digits: true, - ObjectFieldMustBeSimpleString: true, - TagKey: "yaml", - }.Froze() - jsonConfig = jsoniter.Config{ - EscapeHTML: false, - MarshalFloatWith6Digits: true, - ObjectFieldMustBeSimpleString: true, - }.Froze() -) - type collectorJSON struct { Link string `json:"_link"` Jobs []*target.Item `json:"targets"` } -type DiscoveryManager interface { - GetScrapeConfigs() map[string]*promconfig.ScrapeConfig -} - type Server struct { logger logr.Logger allocator allocation.Allocator - discoveryManager DiscoveryManager + discoveryManager *target.Discoverer server *http.Server - yamlMarshaller jsoniter.API // TODO: for testing, it would make a lot of sense to have a simpler interface - jsonMarshaller jsoniter.API // TODO: for testing, it would make a lot of sense to have a simpler interface compareHash uint64 scrapeConfigResponse []byte } -func NewServer(log logr.Logger, allocator allocation.Allocator, discoveryManager DiscoveryManager, listenAddr *string) *Server { +func NewServer(log logr.Logger, allocator allocation.Allocator, discoveryManager *target.Discoverer, listenAddr *string) *Server { s := &Server{ logger: log, allocator: allocator, discoveryManager: discoveryManager, compareHash: uint64(0), - yamlMarshaller: yamlConfig, - jsonMarshaller: jsonConfig, } router := mux.NewRouter().UseEncodedPath() router.Use(s.PrometheusMiddleware) @@ -108,7 +87,7 @@ func (s *Server) Shutdown(ctx context.Context) error { // ScrapeConfigsHandler returns the available scrape configuration discovered by the target allocator. // The target allocator first marshals these configurations such that the underlying prometheus marshaling is used. // After that, the YAML is converted in to a JSON format for consumers to use. -func (s *Server) ScrapeConfigsHandler(w http.ResponseWriter, _ *http.Request) { +func (s *Server) ScrapeConfigsHandler(w http.ResponseWriter, r *http.Request) { configs := s.discoveryManager.GetScrapeConfigs() hash, err := hashstructure.Hash(configs, nil) @@ -119,13 +98,19 @@ func (s *Server) ScrapeConfigsHandler(w http.ResponseWriter, _ *http.Request) { } // if the hashes are different, we need to recompute the scrape config if hash != s.compareHash { - configBytes, mErr := s.yamlMarshaller.Marshal(configs) - if mErr != nil { - s.errorHandler(w, mErr) + var configBytes []byte + configBytes, err = yaml.Marshal(configs) + if err != nil { + s.errorHandler(w, err) + return + } + var jsonConfig []byte + jsonConfig, err = yaml2.YAMLToJSON(configBytes) + if err != nil { + s.errorHandler(w, err) return } - // Update the response and the hash - s.scrapeConfigResponse = configBytes + s.scrapeConfigResponse = jsonConfig s.compareHash = hash } // We don't use the jsonHandler method because we don't want our bytes to be re-encoded @@ -136,7 +121,7 @@ func (s *Server) ScrapeConfigsHandler(w http.ResponseWriter, _ *http.Request) { } } -func (s *Server) JobHandler(w http.ResponseWriter, _ *http.Request) { +func (s *Server) JobHandler(w http.ResponseWriter, r *http.Request) { displayData := make(map[string]target.LinkJSON) for _, v := range s.allocator.TargetItems() { displayData[v.JobName] = target.LinkJSON{Link: v.Link.Link} @@ -187,7 +172,7 @@ func (s *Server) errorHandler(w http.ResponseWriter, err error) { func (s *Server) jsonHandler(w http.ResponseWriter, data interface{}) { w.Header().Set("Content-Type", "application/json") - err := s.jsonMarshaller.NewEncoder(w).Encode(data) + err := json.NewEncoder(w).Encode(data) if err != nil { s.logger.Error(err, "failed to encode data for http response") } diff --git a/cmd/otel-allocator/server/server_test.go b/cmd/otel-allocator/server/server_test.go index afc6f7e782..3f3c7da118 100644 --- a/cmd/otel-allocator/server/server_test.go +++ b/cmd/otel-allocator/server/server_test.go @@ -15,16 +15,15 @@ package server import ( + "crypto/rand" "encoding/json" "fmt" "io" - "math/rand" + "math/big" "net/http/httptest" "testing" - "time" "github.com/prometheus/common/model" - promconfig "github.com/prometheus/prometheus/config" "github.com/stretchr/testify/assert" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -45,14 +44,6 @@ var ( testJobTargetItemTwo = target.NewItem("test-job", "test-url2", testJobLabelSetTwo, "test-collector2") ) -type mockDiscoveryManager struct { - m map[string]*promconfig.ScrapeConfig -} - -func (m *mockDiscoveryManager) GetScrapeConfigs() map[string]*promconfig.ScrapeConfig { - return m.m -} - func TestServer_TargetsHandler(t *testing.T) { leastWeighted, _ := allocation.New("least-weighted", logger) type args struct { @@ -179,8 +170,12 @@ func TestServer_TargetsHandler(t *testing.T) { } } +func randInt(max int64) int64 { + nBig, _ := rand.Int(rand.Reader, big.NewInt(max)) + return nBig.Int64() +} + func BenchmarkServerTargetsHandler(b *testing.B) { - rand.Seed(time.Now().UnixNano()) var table = []struct { numCollectors int numJobs int @@ -207,8 +202,8 @@ func BenchmarkServerTargetsHandler(b *testing.B) { b.Run(fmt.Sprintf("%s_num_cols_%d_num_jobs_%d", allocatorName, v.numCollectors, v.numJobs), func(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { - randomJob := rand.Intn(v.numJobs) //nolint: gosec - randomCol := rand.Intn(v.numCollectors) //nolint: gosec + randomJob := randInt(int64(v.numJobs)) + randomCol := randInt(int64(v.numCollectors)) request := httptest.NewRequest("GET", fmt.Sprintf("/jobs/test-job-%d/targets?collector_id=collector-%d", randomJob, randomCol), nil) w := httptest.NewRecorder() s.server.Handler.ServeHTTP(w, request) @@ -217,201 +212,3 @@ func BenchmarkServerTargetsHandler(b *testing.B) { } } } - -func BenchmarkScrapeConfigsHandler(b *testing.B) { - rand.Seed(time.Now().UnixNano()) - s := &Server{ - logger: logger, - yamlMarshaller: yamlConfig, - } - - tests := []int{0, 5, 10, 50, 100, 500} - for _, n := range tests { - data := makeNScrapeConfigs(n) - b.Run(fmt.Sprintf("%d_targets", n), func(b *testing.B) { - b.ReportAllocs() - for i := 0; i < b.N; i++ { - s.compareHash = 0 - s.discoveryManager = &mockDiscoveryManager{m: data} - resp := httptest.NewRecorder() - s.ScrapeConfigsHandler(resp, nil) - } - }) - } -} - -func BenchmarkCollectorMapJSONHandler(b *testing.B) { - rand.Seed(time.Now().UnixNano()) - s := &Server{ - logger: logger, - jsonMarshaller: jsonConfig, - } - - tests := []struct { - numCollectors int - numTargets int - }{ - { - numCollectors: 0, - numTargets: 0, - }, - { - numCollectors: 5, - numTargets: 5, - }, - { - numCollectors: 5, - numTargets: 50, - }, - { - numCollectors: 5, - numTargets: 500, - }, - { - numCollectors: 50, - numTargets: 5, - }, - { - numCollectors: 50, - numTargets: 50, - }, - { - numCollectors: 50, - numTargets: 500, - }, - { - numCollectors: 50, - numTargets: 5000, - }, - } - for _, tc := range tests { - data := makeNCollectorJSON(tc.numCollectors, tc.numTargets) - b.Run(fmt.Sprintf("%d_collectors_%d_targets", tc.numCollectors, tc.numTargets), func(b *testing.B) { - b.ReportAllocs() - for i := 0; i < b.N; i++ { - resp := httptest.NewRecorder() - s.jsonHandler(resp, data) - } - }) - } -} - -func BenchmarkTargetItemsJSONHandler(b *testing.B) { - rand.Seed(time.Now().UnixNano()) - s := &Server{ - logger: logger, - jsonMarshaller: jsonConfig, - } - - tests := []struct { - numTargets int - numLabels int - }{ - { - numTargets: 0, - numLabels: 0, - }, - { - numTargets: 5, - numLabels: 5, - }, - { - numTargets: 5, - numLabels: 50, - }, - { - numTargets: 50, - numLabels: 5, - }, - { - numTargets: 50, - numLabels: 50, - }, - { - numTargets: 500, - numLabels: 50, - }, - { - numTargets: 500, - numLabels: 500, - }, - { - numTargets: 5000, - numLabels: 50, - }, - { - numTargets: 5000, - numLabels: 500, - }, - } - for _, tc := range tests { - data := makeNTargetItems(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++ { - resp := httptest.NewRecorder() - s.jsonHandler(resp, data) - } - }) - } -} - -var letters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ-_/") - -func randSeq(n int) string { - b := make([]rune, n) - for i := range b { - b[i] = letters[rand.Intn(len(letters))] //nolint:gosec - } - return string(b) -} - -func makeNScrapeConfigs(n int) map[string]*promconfig.ScrapeConfig { - items := make(map[string]*promconfig.ScrapeConfig, n) - for i := 0; i < n; i++ { - items[randSeq(20)] = &promconfig.ScrapeConfig{ - JobName: randSeq(20), - ScrapeInterval: model.Duration(30 * time.Second), - ScrapeTimeout: model.Duration(time.Minute), - MetricsPath: randSeq(50), - SampleLimit: 5, - TargetLimit: 200, - LabelLimit: 20, - LabelNameLengthLimit: 50, - LabelValueLengthLimit: 100, - } - } - return items -} - -func makeNCollectorJSON(numCollectors, numItems int) map[string]collectorJSON { - items := make(map[string]collectorJSON, numCollectors) - for i := 0; i < numCollectors; i++ { - items[randSeq(20)] = collectorJSON{ - Link: randSeq(120), - Jobs: makeNTargetItems(numItems, 50), - } - } - return items -} - -func makeNTargetItems(numItems, numLabels int) []*target.Item { - items := make([]*target.Item, 0, numItems) - for i := 0; i < numItems; i++ { - items = append(items, target.NewItem( - randSeq(80), - randSeq(150), - makeNNewLabels(numLabels), - randSeq(30), - )) - } - return items -} - -func makeNNewLabels(n int) model.LabelSet { - labels := make(map[model.LabelName]model.LabelValue, n) - for i := 0; i < n; i++ { - labels[model.LabelName(randSeq(20))] = model.LabelValue(randSeq(20)) - } - return labels -} diff --git a/go.mod b/go.mod index 3f9d7f0cd6..0860325c33 100644 --- a/go.mod +++ b/go.mod @@ -8,14 +8,12 @@ require ( github.com/Masterminds/semver/v3 v3.2.0 github.com/go-logr/logr v1.2.3 github.com/mitchellh/mapstructure v1.5.0 - github.com/openshift/api v3.9.0+incompatible github.com/prometheus/prometheus v1.8.2-0.20210621150501-ff58416a0b02 github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.8.1 go.opentelemetry.io/otel v1.11.2 gopkg.in/yaml.v2 v2.4.0 k8s.io/api v0.25.4 - k8s.io/apiextensions-apiserver v0.25.0 k8s.io/apimachinery v0.25.4 k8s.io/client-go v0.25.4 k8s.io/component-base v0.25.4 @@ -99,6 +97,7 @@ require ( github.com/mwitkow/go-conntrack v0.0.0-20190716064945-2f068394615f // indirect github.com/opencontainers/go-digest v1.0.0 // indirect github.com/opencontainers/image-spec v1.0.1 // indirect + github.com/openshift/api v3.9.0+incompatible // indirect github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/prometheus/client_golang v1.12.2 // indirect @@ -129,6 +128,7 @@ require ( gopkg.in/fsnotify/fsnotify.v1 v1.4.7 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect + k8s.io/apiextensions-apiserver v0.25.0 // indirect k8s.io/klog/v2 v2.70.1 // indirect k8s.io/kube-openapi v0.0.0-20220803162953-67bda5d908f1 // indirect k8s.io/utils v0.0.0-20220728103510-ee6ede2d64ed // indirect