Receive requests are required to have partitionKey label #279
Conversation
openshift-ci-robot
added
approved
pkg/receive/handler.go
Outdated
|
|
||
| var ErrRequiredLabelMissing = fmt.Errorf("a required label is missing from the metric") | ||
|
|
||
| // TODO: Make this a middleware eventually |
There was a problem hiding this comment.
can we actually make this part of this same PR? Now that we are rewriting key pieces of telemetry, it's a good time to start writing more maintainable and composable funcs. Let's make a write-request validation middleware that can take a variable amount of func(* prompb.WriteRequest) error funcs and returns a handler. This way we can wrap the forwarder easily and separate concerns
There was a problem hiding this comment.
can we still address this ^ before merging?
|
/uncc @lilic @paulfantom |
openshift-ci-robot
added
the
needs-rebase
metalmatze
force-pushed
the
receive-enforce-partition-key
branch
from
13e55fd
to
f16b48d
Compare
openshift-ci-robot
removed
the
needs-rebase
pkg/receive/handler.go
Outdated
| found := false | ||
| for _, ts := range wreq.GetTimeseries() { | ||
| for _, l := range ts.GetLabels() { | ||
| if l.Name == h.PartitionKey { |
There was a problem hiding this comment.
I think this logic is wrong. This currently checks if at least one time series on the request contains the label. We want to ensure that every time series has that label.
pkg/receive/handler.go
Outdated
| var ErrRequiredLabelMissing = fmt.Errorf("a required label is missing from the metric") | ||
|
|
||
| // ValidateLabels makes sure that the request's content contains the required partitionKey | ||
| func (h *Handler) ValidateLabels(next http.Handler) http.HandlerFunc { |
There was a problem hiding this comment.
It doesn't seem like this really needs to be a method of the handler struct. IMO it should be an function that accepts a label name and an next and returns an HandlerFunc. This would also eliminate the need for the constructor func NewHandler to accept an extra argument.
There was a problem hiding this comment.
I guess the reason I did it that way was to cut down on the amount of args being passed around on the caller side.
Happy to change it though 😊
There was a problem hiding this comment.
yes, I think it's better; these two things should be entirely separate concerns that are not coupled. It will also allow us to generalize the label requirement more: rather than enforcing _id=$token we can enforce any arbitrary $key=$label
pkg/receive/handler.go
Outdated
| @@ -61,6 +70,9 @@ func (h *Handler) Receive(w http.ResponseWriter, r *http.Request) { | |||
|
|
|||
| defer r.Body.Close() | |||
|
|
|||
| // Limit the request body size to a sane default | |||
| r.Body = http.MaxBytesReader(w, r.Body, requestLimit) | |||
There was a problem hiding this comment.
This request limit should really be configurable via a command line argument so that we don't need to PR a golang change to bump the limit.
There was a problem hiding this comment.
We should also be able to disable the limi, eg if the limit is 0
There was a problem hiding this comment.
This limiter should also be its own Middleware i think. It doesn't really need to be couped to the logic of forwarding requests.
There was a problem hiding this comment.
This limiter should also be its own Middleware i think. It doesn't really need to be couped to the logic of forwarding requests.
metalmatze
force-pushed
the
receive-enforce-partition-key
branch
from
a866630
to
8888b95
Compare
|
/test integration |
1 similar comment
|
/test integration |
pkg/receive/handler.go
Outdated
| @@ -49,6 +56,8 @@ func NewHandler(logger log.Logger, forwardURL string, reg prometheus.Registerer) | |||
| reg.MustRegister(h.forwardRequestsTotal) | |||
| } | |||
|
|
|||
| h.logger.Log("msg", "running receive handler") | |||
There was a problem hiding this comment.
this log is a bit funny to me sine we're not running a goroutine here but creating a struct
There was a problem hiding this comment.
this might have been a test of mine with the logger not logging anything at first... 😁
| @@ -91,3 +99,89 @@ func (h *Handler) Receive(w http.ResponseWriter, r *http.Request) { | |||
| h.forwardRequestsTotal.WithLabelValues("success").Inc() | |||
| w.WriteHeader(resp.StatusCode) | |||
| } | |||
|
|
|||
| func LimitBodySize(limit int64, next http.Handler) http.HandlerFunc { | |||
There was a problem hiding this comment.
could you add a short comment here
pkg/receive/handler.go
Outdated
|
|
||
| "github.com/openshift/telemeter/pkg/authorize" | ||
| ) | ||
|
|
||
| const forwardTimeout = 5 * time.Second | ||
| const RequestLimit = 15 * 1024 // based on historic Prometheus data with 6KB at most |
There was a problem hiding this comment.
can we make this DefaultRequestLimit? Eventually we'll make the request limit overridable via flags, no?
|
/lgtm super cool 😎 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: metalmatze, squat The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Let's decode the receive requests and check if the partitionKey (for us mostly
_id) is available in the request and otherwise reject the request.