Query-frontend: use activity tracker for requests #3561

colega · 2022-11-29T14:07:25Z

What this PR does

Query-frontend uses the activity-tracker for the PromQL engine, but that is just for the sharded queries execution. Query-frontend does much more than that, and we should also track that.

This adds every HTTP request received by the Handler to the activity tracker.

Which issue(s) this PR fixes or relates to

Fixes (none)

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

replay · 2022-11-29T14:47:21Z

The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG document. Thanks!

colega · 2022-11-29T14:48:58Z

Rebased and force-pushed ✅

pstibrany

Thanks. I'd suggest to simplify the PR: ignore errors when generating activity description, and use real activity tracker for testing.

pkg/frontend/transport/handler.go

pkg/frontend/transport/handler_test.go

pstibrany

Thanks for adding this!

CHANGELOG.md

When setting the default "time" param, if the form was already parsed (by some previous middleware) we didn't propagate that change into r.Form. In order to solve that, we're now replacing the r.Body with a readCloseSeeker, that can be seeked to the beginning again and again. This change isn't so important on itself, but it's needed to make #3561 work properly, as there we're going to parse the request form at the beginning of the request in order to generate the activity. Additionally, though not important, note that we were never checking the POST request body, in case the "time" was specified there, and that's where Prometheus API client is sending it, so we were always setting the default "time" param on the Query, and it worked just because if it was also sent in the body, that would take precedence when parsing the form (but it was a waste of resources since we had to parse and render the query string again all the times). Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

pracucci · 2022-12-01T09:33:43Z

@colega Please ping me once this PR is rebased on top of #3588 and ready to be reviewed. Thanks!

pkg/frontend/transport/handler.go

colega · 2022-12-01T16:26:41Z

@pracucci @pstibrany rebased, force-pushed. Ready for review.

pstibrany

I've left some comments, but this should work. Thank you!

pstibrany · 2022-12-02T13:54:07Z

pkg/frontend/transport/handler.go

+	params := copyValues(r.Form)
+
+	// Seek the body back to the beginning, so it can be read again if it's used to forward the request through a roundtripper.
+	if _, err := seeker.Seek(0, io.SeekStart); err != nil {


If we read the body into bytes buffer (which we do, but then wrap it), we wouldn't need to call Seek here, but instead set body to r.Body = io.NopCloser(bytes.NewReader(buf)).

"seeker" abstraction seems unnecessary here.

Seeker allows next user of r.Body to just seek it back instead of having to read it again to a new buffer in order to replace it.

I don't think there is another user of r.Body doing that (except http -> httpgrpc conversion, which reads body into new buffer again).

It would also be very strange if there was code checking if r.Body implements Seeker. That's very unusual interface for HTTP request body to implement, given that it's typically streamed over network.

I don't see the point in hiding functionality that saves memory allocation. io.NopCloser just hides the Seek method, and what I'm doing here is not hiding it. I can only see positive effects of that.

I think once again we make things little bit more complicated and generic than they need to be. But my intention is not to block the PR on this, feel free to merge it as-is.

pstibrany · 2022-12-02T13:55:06Z

pkg/frontend/transport/handler.go

+	// Parse the form, as it's needed to build the activity for the activity-tracker.
+	if err := r.ParseForm(); err != nil {
+		writeError(w, apierror.New(apierror.TypeBadData, err.Error()))
+		return
+	}


If activity tracker is disabled, this is unnecessary. Although it may be better to keep it for consistent handling of all requests.

Given the stats are enabled by default, and tracker too, I'd just keep it here. If it's unnecessary, it's not a big deal.

pstibrany · 2022-12-02T13:58:08Z

pkg/frontend/transport/handler.go

+	// Store the r.Form contents copy, as middlewares may modify it (like setting the default values).
+	params := copyValues(r.Form)
+
+	// Seek the body back to the beginning, so it can be read again if it's used to forward the request through a roundtripper.


The fact that we reset the Body, but keep r.Form non-nil is a red flag. While it is probably OK, it leaves Request in the state which handler or middlewares may not expect, and is different than before this PR.

As an alternative, we could make a copy of request (with the same buffered body), and only parse form in the copy, and use that for activity tracking and logging.

I can't see why it's a red flag.

it leaves Request in the state which handler or middlewares may not expect

Neither handler or middlewares should expect undocumented states. There's no docs saying that a non-nil r.Form indicates an empty body.

I don't think that we need to parse twice just because someone takes a wrong assumption somewhere else.

There's no docs saying that a non-nil r.Form indicates an empty body.

non-nil r.Form implies that ParseForm was called (based on Form field documentation), and ParseForm is documented to read the body for POST, PUT or PATCH request with Content-Type: application/x-www-form-urlencoded.

Since our middlewares don't currently parse the form, we wouldn't parse it twice.

Anyway, well behaved middlewares (ie. all in Mimir) are not making any such assumptions, so we don't need to worry about it anymore.

CHANGELOG.md

Query-frontend uses the activity-tracker for the PromQL engine, but that is just for the sharded queries execution. Query-frontend does much more than that, and we should also track that. This adds every HTTP request received by the Handler to the activity tracker. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Appliedi Peter's comments. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Query-frontend: use activity tracker for requests Query-frontend uses the activity-tracker for the PromQL engine, but that is just for the sharded queries execution. Query-frontend does much more than that, and we should also track that. This adds every HTTP request received by the Handler to the activity tracker. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Update CHANGELOG.md Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Store a copy of params Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Restore the original request state Appliedi Peter's comments. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Fix a comment Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega requested a review from a team as a code owner November 29, 2022 14:07

colega requested a review from pstibrany November 29, 2022 14:08

replay added the release/notified-changelog-cut label Nov 29, 2022

colega force-pushed the query-frontend-request-activity-tracker branch from e35460a to 2d1c630 Compare November 29, 2022 14:48

pstibrany reviewed Nov 29, 2022

View reviewed changes

pkg/frontend/transport/handler.go Outdated Show resolved Hide resolved

pkg/frontend/transport/handler.go Outdated Show resolved Hide resolved

pkg/frontend/transport/handler_test.go Outdated Show resolved Hide resolved

pstibrany approved these changes Nov 29, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

colega marked this pull request as draft November 30, 2022 11:44

colega mentioned this pull request Nov 30, 2022

Parse req form again after setting default time #3588

Closed

3 tasks

pracucci self-requested a review December 1, 2022 09:11

pstibrany self-requested a review December 1, 2022 10:23

pstibrany reviewed Dec 1, 2022

View reviewed changes

pkg/frontend/transport/handler.go Outdated Show resolved Hide resolved

pstibrany self-requested a review December 1, 2022 10:37

colega mentioned this pull request Dec 1, 2022

Add default "time" param to parsed form (if any) #3599

Merged

3 tasks

colega force-pushed the query-frontend-request-activity-tracker branch 2 times, most recently from bad5876 to 2a6678c Compare December 1, 2022 15:55

colega marked this pull request as ready for review December 1, 2022 16:25

pstibrany approved these changes Dec 2, 2022

View reviewed changes

colega force-pushed the query-frontend-request-activity-tracker branch from 1039d35 to 052f56a Compare December 2, 2022 16:33

colega added 4 commits December 7, 2022 09:50

Update CHANGELOG.md

023d2f9

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Store a copy of params

44b788c

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Restore the original request state

32f2b00

Appliedi Peter's comments. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega force-pushed the query-frontend-request-activity-tracker branch from 052f56a to 32f2b00 Compare December 7, 2022 08:58

Fix a comment

27049ca

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega merged commit 0a94f26 into main Dec 7, 2022

colega deleted the query-frontend-request-activity-tracker branch December 7, 2022 09:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query-frontend: use activity tracker for requests #3561

Query-frontend: use activity tracker for requests #3561

colega commented Nov 29, 2022 •

edited

Loading

replay commented Nov 29, 2022

colega commented Nov 29, 2022

pstibrany left a comment

pstibrany left a comment

pracucci commented Dec 1, 2022

colega commented Dec 1, 2022

pstibrany left a comment

pstibrany Dec 2, 2022

colega Dec 2, 2022

pstibrany Dec 2, 2022

pstibrany Dec 2, 2022

colega Dec 2, 2022

pstibrany Dec 2, 2022

pstibrany Dec 2, 2022

colega Dec 2, 2022

pstibrany Dec 2, 2022

colega Dec 2, 2022

pstibrany Dec 2, 2022

Query-frontend: use activity tracker for requests #3561

Query-frontend: use activity tracker for requests #3561

Conversation

colega commented Nov 29, 2022 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

replay commented Nov 29, 2022

colega commented Nov 29, 2022

pstibrany left a comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

pracucci commented Dec 1, 2022

colega commented Dec 1, 2022

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colega commented Nov 29, 2022 •

edited

Loading