Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[chore] change JSON marshaler to goccy-json #3660

Merged
merged 1 commit into from
Feb 5, 2025
Merged

[chore] change JSON marshaler to goccy-json #3660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions cmd/otel-allocator/server/bench_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,7 @@ func BenchmarkScrapeConfigsHandler(b *testing.B) {
func BenchmarkCollectorMapJSONHandler(b *testing.B) {
random := rand.New(rand.NewSource(time.Now().UnixNano())) // nolint: gosec
s := &Server{
logger: logger,
jsonMarshaller: jsonConfig,
logger: logger,
}

tests := []struct {
Expand Down Expand Up @@ -153,8 +152,7 @@ func BenchmarkCollectorMapJSONHandler(b *testing.B) {
func BenchmarkTargetItemsJSONHandler(b *testing.B) {
random := rand.New(rand.NewSource(time.Now().UnixNano())) // nolint: gosec
s := &Server{
logger: logger,
jsonMarshaller: jsonConfig,
logger: logger,
}

tests := []struct {
Expand Down
27 changes: 8 additions & 19 deletions cmd/otel-allocator/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ package server
import (
"context"
"crypto/tls"
"encoding/json"
"fmt"
"net/http"
"net/http/pprof"
Expand All @@ -29,7 +28,7 @@ import (
yaml2 "github.com/ghodss/yaml"
"github.com/gin-gonic/gin"
"github.com/go-logr/logr"
jsoniter "github.com/json-iterator/go"
"github.com/goccy/go-json"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
"github.com/prometheus/client_golang/prometheus/promhttp"
Expand All @@ -49,14 +48,6 @@ var (
}, []string{"path"})
)

var (
jsonConfig = jsoniter.Config{
EscapeHTML: false,
MarshalFloatWith6Digits: true,
ObjectFieldMustBeSimpleString: true,
Comment on lines -54 to -56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know how go-json behaves when it comes to these settings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it follows the same best practices as encoding/json. It doesn't cut any corners.

I assume test coverage would catch any regressions related to this change, is that an incorrect assumption?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should, I'm just a bit wary of Hyrum's Law. But looking through jsoniter's documentation, these seem to be performance optimizations, so we should be fine switching.

On that note, could you run the benchmark in https://github.com/open-telemetry/opentelemetry-operator/blob/main/cmd/otel-allocator/server/bench_test.go before and after this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, jsoniter was trying for higher performance by cutting corners, so if you have those flags off, there is no performance improvement from it. I have attached before/after/benchstat results below.

FWIW the collector project as a whole has moved over to goccy-json a little while back.

}.Froze()
)

type collectorJSON struct {
Link string `json:"_link"`
Jobs []*targetJSON `json:"targets"`
Expand All @@ -72,11 +63,10 @@ type targetJSON struct {
}

type Server struct {
logger logr.Logger
allocator allocation.Allocator
server *http.Server
httpsServer *http.Server
jsonMarshaller jsoniter.API
logger logr.Logger
allocator allocation.Allocator
server *http.Server
httpsServer *http.Server

// Use RWMutex to protect scrapeConfigResponse, since it
// will be predominantly read and only written when config
Expand Down Expand Up @@ -116,9 +106,8 @@ func (s *Server) setRouter(router *gin.Engine) {

func NewServer(log logr.Logger, allocator allocation.Allocator, listenAddr string, options ...Option) *Server {
s := &Server{
logger: log,
allocator: allocator,
jsonMarshaller: jsonConfig,
logger: log,
allocator: allocator,
}

gin.SetMode(gin.ReleaseMode)
Expand Down Expand Up @@ -323,7 +312,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")
}
Expand Down
Loading