-
Notifications
You must be signed in to change notification settings - Fork 820
Enhance the performance of the frontend JSON codec #6816
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
Conversation
101064f
to
6c36bd4
Compare
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.
Thanks. I think this change makes sense!
Maybe @alanprot can also take a look
pkg/querier/tripperware/query.go
Outdated
@@ -25,6 +25,7 @@ import ( | |||
"github.com/prometheus/prometheus/util/jsonutil" | |||
"github.com/weaveworks/common/httpgrpc" | |||
|
|||
_ "github.com/cortexproject/cortex/pkg/chunk" // Register jsoniter type: labels.Labels. |
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 am wondering if we have a better package to import? In case we remove some code from that chunk package.
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.
maybe we can just something like this?
var _ = labels.Labels{}
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.
maybe we can reimplement the codec for cortexpb.LabelAdapter
and use it directly
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 am worried that at sometime we just remove that code path from the package without noticing and we have the regression.
We don't have to do it in this PR but let's at least add some TODO or notes for re-implementing the codec or something similar. Creating an issue also works
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.
We can export the legacy functions directly and use them without relying on global registration.
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.
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.
Thanks!
pkg/querier/tripperware/query.go
Outdated
@@ -25,6 +25,7 @@ import ( | |||
"github.com/prometheus/prometheus/util/jsonutil" | |||
"github.com/weaveworks/common/httpgrpc" | |||
|
|||
_ "github.com/cortexproject/cortex/pkg/chunk" // Register jsoniter type: labels.Labels. |
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.
maybe we can just something like this?
var _ = labels.Labels{}
Just out of curiosity, can you also show the impact in memory on this? |
Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>
6c36bd4
to
5840e00
Compare
Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>
08c7800
to
da7343d
Compare
@damnever For the graph you shared, when was the change deployed and how much it impacted memory? |
@yeya24 the middle of 06/13 12:00 - 06/14 00:00, peak memory decreased significantly, possibly by 70% |
I think this PR broke the integration tests.
Looks like it also failed in your tasks, but the metrics is different |
What this PR does:
Stream json encoding using existing functions, reducing CPU usage ~50%.
Which issue(s) this PR fixes:
After upgrading from v1.16 to v1.19, the frontend's CPU usage increased significantly. The primary cause appears to be related to JSON codec, the use of
encoding/json
and the read-and-parse process. This patch addresses both issues. However, despite applying the patch, CPU usage remains higher than it was prior to the upgrade. The exact reason for this is unclear, maybe it is due to Go 1.24 or something else, but profiling results still show that decoding labels continues to consume a substantial amount of processing time (profile below).Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]