Distributor: /all_user_stats now show API and Rule Ingest Rate correctly #2457
Conversation
…Rate correctly. Signed-off-by: Wing924 <weihe924stephen@gmail.com>
pkg/util/push/push.go
Outdated
| _, err := util.ParseProtoReader(r.Context(), r.Body, int(r.ContentLength), cfg.MaxRecvMsgSize, &req, compressionType) | ||
| logger := util.WithContext(r.Context(), util.Logger) | ||
| if err != nil { | ||
| level.Error(logger).Log("err", err.Error()) | ||
| http.Error(w, err.Error(), http.StatusBadRequest) | ||
| return | ||
| } | ||
| req.Source = client.API |
There was a problem hiding this comment.
_, err := util.ParseProtoReader(r.Context(), r.Body, int(r.ContentLength), cfg.MaxRecvMsgSize, &req, compressionType) overwrite that value.
There was a problem hiding this comment.
I believe that was deliberate: if the caller supplies a value then we use that, otherwise we fall back to API.
There was a problem hiding this comment.
I tested here and I'm sure util.ParseProtoReader will overwrite req.Source to 0.
Because client.API equals to 0, you may not notice it.
reproduce
diff --git a/pkg/ingester/client/cortex.proto b/pkg/ingester/client/cortex.proto
index 6a341b59..a9dfcfee 100644
--- a/pkg/ingester/client/cortex.proto
+++ b/pkg/ingester/client/cortex.proto
@@ -30,7 +30,8 @@ service Ingester {
message WriteRequest {
repeated TimeSeries timeseries = 1 [(gogoproto.nullable) = false, (gogoproto.customtype) = "PreallocTimeseries"];
enum SourceEnum {
- API = 0;
+ UNKNOWN_SOURCE = 0;
+ API = 2;
RULE = 1;
}
SourceEnum Source = 2;and print req.Source before and afterutil.ParseProtoReader.
result:
- before is
API - after is
UNKNOWN_SOURCE
There was a problem hiding this comment.
Did you test it with and without the caller supplying a value in the protobuf?
There was a problem hiding this comment.
I just test it with real remote write from distributor.
I don't think remote write protobuf have Source field.
There was a problem hiding this comment.
remote write protobuf:
https://github.com/prometheus/prometheus/blob/master/prompb/remote.proto#L22-L24
There was a problem hiding this comment.
I agree on @Wing924 on this. The source is not part of the remote write protocol, so we need to inject it after the WriteRequest has been deseralized, otherwise the deserialization will overwrite it with the default value.
There was a problem hiding this comment.
I added test to cover both prometheus remote write and cortex writerequest.
Wing924
changed the title
/all_user_stats now show API and Rule Ingest Rate correctly./all_user_stats now show API and Rule Ingest Rate correctly; reuse []Timeseries in ruler to reduce memory allocation.
Wing924
changed the title
/all_user_stats now show API and Rule Ingest Rate correctly; reuse []Timeseries in ruler to reduce memory allocation.
Wing924
changed the title
pkg/util/push/push.go
Outdated
| _, err := util.ParseProtoReader(r.Context(), r.Body, int(r.ContentLength), cfg.MaxRecvMsgSize, &req, compressionType) | ||
| logger := util.WithContext(r.Context(), util.Logger) | ||
| if err != nil { | ||
| level.Error(logger).Log("err", err.Error()) | ||
| http.Error(w, err.Error(), http.StatusBadRequest) | ||
| return | ||
| } | ||
| req.Source = client.API |
There was a problem hiding this comment.
I agree on @Wing924 on this. The source is not part of the remote write protocol, so we need to inject it after the WriteRequest has been deseralized, otherwise the deserialization will overwrite it with the default value.
| if req.Source == 0 { | ||
| req.Source = client.API | ||
| } |
There was a problem hiding this comment.
As per our instructions, every commit must explain the changes made. This is an extraordinary change, so needs a very good explanation.
There was a problem hiding this comment.
I'll send another PR to add comment here.
| if req.Source == 0 { | ||
| req.Source = client.API | ||
| } |
There was a problem hiding this comment.
Getting back to this change, @bboreham as right and I was wrong. The req.Source is not overridden by the unmarshalling if not included in the request, so we should have fixed the issue to the root: ensuring Source is always set within Cortex.
https://github.com/cortexproject/cortex/blob/master/pkg/ingester/client/cortex.pb.go#L5502-L5520
@Wing924 May you re-iterate on this?
There was a problem hiding this comment.
The req.Source is not overridden by the unmarshalling if not included in the request
It's not true.
Let's change the code like this:
+ req.Source = 2
_, err := util.ParseProtoReader(r.Context(), r.Body, int(r.ContentLength), cfg.MaxRecvMsgSize, &req, compressionType)
logger := util.WithContext(r.Context(), util.Logger)
if err != nil {
level.Error(logger).Log("err", err.Error())
http.Error(w, err.Error(), http.StatusBadRequest)
return
}
- if req.Source == 0 {
- req.Source = client.API
- }if Source is not included in the request, we expect req.Source is 2, right?
But actually it's 0.
You can check my test.
There was a problem hiding this comment.
https://github.com/cortexproject/cortex/blob/master/pkg/util/http.go#L107
this line reset req.Source to 0
There was a problem hiding this comment.
How about you explain your change, where I have substituted one const value with its definition for clarity:
if req.Source == 0 {
req.Source = 0
}
There was a problem hiding this comment.
I know client.API is equals to 0 so that you think these code is useless.
This code is explicitly set default value if req.Source is not set.
There was a problem hiding this comment.
Physically, client.API equals to 0, but logically, they are different things.
There was a problem hiding this comment.
OK, the Reset() is indeed the important line, thanks.
So the original code didn't do anything (it set to 0 which was then overridden to 0) and the new code doesn't do anything.
The meaning of the code as-was is that we initialise to `API` but allow the unmarshal code to override. Tests were added in #2457 to confirm this works as expected. Signed-off-by: Bryan Boreham <bryan@weave.works>
Signed-off-by: Wing924 weihe924stephen@gmail.com
What this PR does:
/all_user_statsnow show API and Rule Ingest Rate correctly.Which issue(s) this PR fixes:
Fixes #2171
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]