-
Notifications
You must be signed in to change notification settings - Fork 486
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.
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
Conversation
c6c6626
to
928c757
Compare
EscapeHTML: false, | ||
MarshalFloatWith6Digits: true, | ||
ObjectFieldMustBeSimpleString: true, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Before:
After:
Benchmark:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with this, but I'd like another maintainer approval. @open-telemetry/operator-maintainers
Description:
jsoniter is no longer maintained; this PR switches over to github.com/goccy/go-json