Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Receive: Make head series limiting config per tenant #5685

Merged
merged 9 commits into from
Sep 26, 2022
Merged

Receive: Make head series limiting config per tenant #5685

merged 9 commits into from
Sep 26, 2022

Conversation

saswatamcode
Copy link
Member

This PR adds head/active series limiting configuration to the new per tenant limiting config added in #5565.

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • Remove flags and fields passed via receive Handler options
  • Make naming of active series limiting interfaces/structs consistent to "head series" and move to a separate file
  • Add options to limiting config file
  • Update e2e test to use new config
  • Update docs

Verification

All receive e2e tests pass locally and have tested different config scenarios.

douglascamata
douglascamata suggested changes Sep 13, 2022
View reviewed changes
Copy link
Contributor

@douglascamata douglascamata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job overall! I put a few comments, but not many. 👍

docs/components/receive.md Outdated Show resolved Hide resolved
docs/components/receive.md Outdated Show resolved Hide resolved
pkg/receive/head_series_limiter.go Outdated Show resolved Hide resolved
pkg/receive/limiter_config.go Outdated Show resolved Hide resolved
pkg/receive/request_limiter.go Outdated Show resolved Hide resolved
pkg/receive/request_limiter.go Show resolved Hide resolved
pkg/receive/head_series_limiter.go Outdated Show resolved Hide resolved
pkg/receive/head_series_limiter.go Outdated Show resolved Hide resolved
douglascamata
douglascamata previously approved these changes Sep 14, 2022
View reviewed changes
Copy link
Contributor

@douglascamata douglascamata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but would love to hear some feedback from the maintainers as well.

matej-g
matej-g reviewed Sep 15, 2022
View reviewed changes
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stellar job as always @saswatamcode ✨. I have few small-ish suggestions, mostly about organizing the code, otherwise this looks good!

pkg/httpconfig/http.go Outdated Show resolved Hide resolved
cmd/thanos/receive.go Outdated Show resolved Hide resolved
docs/components/receive.md Outdated Show resolved Hide resolved
docs/components/receive.md Outdated Show resolved Hide resolved
pkg/receive/head_series_limiter.go Outdated Show resolved Hide resolved
bwplotka
bwplotka previously approved these changes Sep 15, 2022
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM, mod @matej-g comments. Thanks!

test/e2e/e2ethanos/services.go Outdated Show resolved Hide resolved
@saswatamcode
Copy link
Member Author

Have implemented all suggestions. Also, initialized limiter in cmd/receive.go and passed it down to webHandler as suggested by @matej-g . 🙂

@matej-g
Copy link
Collaborator

matej-g commented Sep 16, 2022

Seems like test failure is related 🧐

--- FAIL: TestReceiveQuorum (0.00s)
    --- FAIL: TestReceiveQuorum/size_1_success (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0xea9352]

goroutine 64 [running]:
testing.tRunner.func1.2({0xfb8c40, 0x1b41ff0})
	/usr/local/go/src/testing/testing.go:1389 +0x24e
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1392 +0x39f
panic({0xfb8c40, 0x1b41ff0})
	/usr/local/go/src/runtime/panic.go:838 +0x207
github.com/thanos-io/thanos/pkg/receive.(*Handler).receiveHTTP.func1({0x140fd58?, 0xc0000440b0?})
	/home/circleci/project/pkg/receive/handler.go:411 +0x52
github.com/thanos-io/thanos/pkg/tracing.DoInSpan({0x140fd58?, 0xc0000440b0?}, {0x1121e8c?, 0x2?}, 0xc00028fcc0, {0x0?, 0x0?, 0xa?})
	/home/circleci/project/pkg/tracing/tracing.go:95 +0xa3
github.com/thanos-io/thanos/pkg/receive.(*Handler).receiveHTTP(0xc0001942a0, {0x140f148, 0xc00022a6c0}, 0xc000152400)
	/home/circleci/project/pkg/receive/handler.go:410 +0x2b0
github.com/thanos-io/thanos/pkg/receive.makeRequest(0xc0001942a0, {0x1105819, 0x4}, 0x1?)
	/home/circleci/project/pkg/receive/handler_test.go:1210 +0x25e
github.com/thanos-io/thanos/pkg/receive.TestReceiveQuorum.func4(0xc0002621a0)
	/home/circleci/project/pkg/receive/handler_test.go:687 +0xd2
testing.tRunner(0xc0002621a0, 0xc00023c7c0)
	/usr/local/go/src/testing/testing.go:1439 +0x102
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1486 +0x35f
FAIL	github.com/thanos-io/thanos/pkg/receive	0.053s

matej-g
matej-g previously approved these changes Sep 16, 2022
View reviewed changes
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bwplotka
bwplotka previously approved these changes Sep 22, 2022
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@saswatamcode saswatamcode dismissed stale reviews from bwplotka and matej-g via d1c1565 September 23, 2022 03:08
@saswatamcode
Receive: Make head series limiting configuration per-tenant
1bf8a93 
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
@saswatamcode
Regen docs and make naming consistent
57d4677 
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
@saswatamcode
Add changelog
a47888a 
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
@saswatamcode
Align config with exisiting implementation
8c8eefb 
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
@saswatamcode
Implement suggestions
8a286de 
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
@saswatamcode
Implement suggestions; Initialize limiter in cmd/receive.go
8ed759a 
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
@saswatamcode
Make log 'component' consistent
9b3c55d 
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
@saswatamcode
Update unit test
4ef5351 
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
@saswatamcode
Fix after rebase
cfaafd9 
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
matej-g
matej-g approved these changes Sep 26, 2022
View reviewed changes
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for rebasing the PR! Looking good.

@matej-g matej-g merged commit a31e4c5 into thanos-io:main Sep 26, 2022
@NotAFile NotAFile mentioned this pull request Apr 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matej-g matej-g matej-g approved these changes

@douglascamata douglascamata douglascamata left review comments

@bwplotka bwplotka bwplotka left review comments

Assignees
No one assigned
Labels
size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@saswatamcode @matej-g @douglascamata @bwplotka