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.

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

receiver: Split - Upstream the changes from the PR #3580 #3845

Closed
wants to merge 23 commits into from

Conversation

yashrsharma44
Copy link
Contributor

@yashrsharma44 yashrsharma44 commented Feb 26, 2021

Rebased #3580 with the main branch.

Notes:

  • There were few issues with the dynamic type checking for error, I am getting the following error -
    errs, ok := unwrappedErr.(errutil.MultiError)
    unwrappedErr (variable of type error) cannot have dynamic type errutil.MultiError (missing method Error)

Fixed it by creating another method for multierror - Error(). Let me know if the above makes sense.
Would love some reviews on this.
cc @brancz

pkg/receive/multitsdb.go Outdated Show resolved Hide resolved
pkg/receive/multitsdb.go Outdated Show resolved Hide resolved
test/e2e/e2ethanos/services.go Outdated Show resolved Hide resolved
test/e2e/receive_test.go Outdated Show resolved Hide resolved
test/e2e/e2ethanos/services.go Outdated Show resolved Hide resolved
@yashrsharma44
Copy link
Contributor Author

I have updated the changes, take a look whenever free 🤗 @yeya24

test/e2e/e2ethanos/services.go Outdated Show resolved Hide resolved
cmd/thanos/receive.go Outdated Show resolved Hide resolved
cmd/thanos/receive_route.go Show resolved Hide resolved
@kakkoyun kakkoyun changed the title Upstream the changes from the PR #3580 receiver: Split - Upstream the changes from the PR #3580 Mar 1, 2021
@yashrsharma44
Copy link
Contributor Author

I wonder if this suggestion is helpful in make lint -

/home/yash/golib/src/github.com/thanos-io/thanos/pkg/route/handler_test.go:234:31: declaration "NewCounterVec" from package "github.com/prometheus/client_golang/prometheus" shouldn't be used, suggested: "github.com/prometheus/client_golang/prometheus/promauto.{NewCounter,NewCounterVec,NewCounterVec,NewGauge,NewGaugeVec,NewGaugeFunc,NewHistorgram,NewHistogramVec,NewSummary,NewSummaryVec}"
/home/yash/golib/src/github.com/thanos-io/thanos/pkg/route/handler_test.go:240:28: declaration "NewCounterVec" from package "github.com/prometheus/client_golang/prometheus" shouldn't be used, suggested: "github.com/prometheus/client_golang/prometheus/promauto.{NewCounter,NewCounterVec,NewCounterVec,NewGauge,NewGaugeVec,NewGaugeFunc,NewHistorgram,NewHistogramVec,NewSummary,NewSummaryVec}"
/home/yash/golib/src/github.com/thanos-io/thanos/pkg/route/handler_test.go:246:33: declaration "NewGauge" from package "github.com/prometheus/client_golang/prometheus" shouldn't be used, suggested: "github.com/prometheus/client_golang/prometheus/promauto.{NewCounter,NewCounterVec,NewCounterVec,NewGauge,NewGaugeVec,NewGaugeFunc,NewHistorgram,NewHistogramVec,NewSummary,NewSummaryVec}"

because a similar issue was reported here after applying the suggestion - prometheus/client_golang#716

@yashrsharma44 yashrsharma44 force-pushed the update-receive branch 3 times, most recently from 09022da to 4361243 Compare March 2, 2021 05:21
@yashrsharma44
Copy link
Contributor Author

yashrsharma44 commented Mar 2, 2021

Any idea to get the tests fixed? 🤔 cc @brancz @kakkoyun @yeya24

@brancz
Copy link
Member

brancz commented Mar 2, 2021

I don't have time for this right now, but what I'm a bit confused about is that

  1. There seem to be commits missing compared to the other PR
  2. The e2e tests were passing on the last state of the other PR

@yeya24
Copy link
Contributor

yeya24 commented Mar 2, 2021

I will do some investigation today.

@yeya24
Copy link
Contributor

yeya24 commented Mar 3, 2021

Can you please fix the Circle CI test and linting tests?

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Either we're missing some context on this or we miss certain parts of the code 😅

cmd/thanos/receive_route.go Outdated Show resolved Hide resolved
cmd/thanos/receive_route.go Outdated Show resolved Hide resolved
cmd/thanos/main.go Outdated Show resolved Hide resolved
pkg/errutil/multierror.go Outdated Show resolved Hide resolved
pkg/route/config_test.go Outdated Show resolved Hide resolved
// determineWriteErrorCause extracts a sentinel error that has occurred more than the given threshold from a given fan-out error.
// It will inspect the error's cause if the error is a MultiError,
// It will return cause of each contained error but will not traverse any deeper.
func determineWriteErrorCause(err error, threshold int) error {
Copy link
Member

Choose a reason for hiding this comment

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

Let's make sure we have parity

func determineWriteErrorCause(err error, threshold int) error {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate on this?

pkg/route/hashring.go Outdated Show resolved Hide resolved
cmd/thanos/receive.go Outdated Show resolved Hide resolved
cmd/thanos/receive.go Outdated Show resolved Hide resolved
@kakkoyun kakkoyun marked this pull request as draft March 4, 2021 14:54
@kakkoyun kakkoyun changed the title receiver: Split - Upstream the changes from the PR #3580 WIP: receiver: Split - Upstream the changes from the PR #3580 Mar 4, 2021
@squat squat marked this pull request as ready for review March 10, 2021 10:19
cmd/thanos/main.go Outdated Show resolved Hide resolved
@yashrsharma44 yashrsharma44 force-pushed the update-receive branch 3 times, most recently from 47799f5 to d30458e Compare March 11, 2021 05:52
@yashrsharma44
Copy link
Contributor Author

Fixed the failing tests as we were using MultiError type assertion instead of NonNilMultiError assertion. Now the tests are passing.
cc @kakkoyun @squat can you review if the current changes make sense in accordance to the main branch?

Chans321 and others added 6 commits March 14, 2021 22:08
Signed-off-by: Chans321 <[email protected]>

fixes in e2e test for new receieve architecture - more fixes pending

fixes in receive and e2e test of receive

Signed-off-by: Chans321 <[email protected]>

fixed connection reset by peer error in receive e2e tests. all e2e tests pass

Signed-off-by: Chans321 <[email protected]>

pkg/route: Fix unit tests

route: Make hashring only work on labels

route: Incorporate thanos-io#2899

fixed import issues and changed the query test signature

Signed-off-by: Yash Sharma <[email protected]>

made some corrections while rebasing receiver changes

Signed-off-by: Yash Sharma <[email protected]>

Added a method Error() to implement golang error interface

Signed-off-by: Yash Sharma <[email protected]>

linting

Signed-off-by: Yash Sharma <[email protected]>

use the new promauto package

Signed-off-by: Yash Sharma <[email protected]>

removed unused params

Signed-off-by: Yash Sharma <[email protected]>

rename method and add comments

Signed-off-by: Yash Sharma <[email protected]>

removed unused flags in receiver service in e2e tests

Signed-off-by: Yash Sharma <[email protected]>

replaced promauto with prometheus, as earlier was causing unnecessary panic

Signed-off-by: Yash Sharma <[email protected]>

changes for debug

Signed-off-by: Yash Sharma <[email protected]>

removed entry for receive test for e2e

Signed-off-by: Yash Sharma <[email protected]>

added a readiness probe

Signed-off-by: Yash Sharma <[email protected]>

fix E2E tests for receive router

Signed-off-by: yeya24 <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
@yashrsharma44 yashrsharma44 changed the title WIP: receiver: Split - Upstream the changes from the PR #3580 receiver: Split - Upstream the changes from the PR #3580 Mar 14, 2021
Chans321 and others added 4 commits March 23, 2021 21:49
Signed-off-by: Chans321 <[email protected]>

fixes in e2e test for new receieve architecture - more fixes pending

fixes in receive and e2e test of receive

Signed-off-by: Chans321 <[email protected]>

fixed connection reset by peer error in receive e2e tests. all e2e tests pass

Signed-off-by: Chans321 <[email protected]>

pkg/route: Fix unit tests

route: Make hashring only work on labels

route: Incorporate thanos-io#2899

fixed import issues and changed the query test signature

Signed-off-by: Yash Sharma <[email protected]>

made some corrections while rebasing receiver changes

Signed-off-by: Yash Sharma <[email protected]>

Added a method Error() to implement golang error interface

Signed-off-by: Yash Sharma <[email protected]>

linting

Signed-off-by: Yash Sharma <[email protected]>

use the new promauto package

Signed-off-by: Yash Sharma <[email protected]>

removed unused params

Signed-off-by: Yash Sharma <[email protected]>

rename method and add comments

Signed-off-by: Yash Sharma <[email protected]>

removed unused flags in receiver service in e2e tests

Signed-off-by: Yash Sharma <[email protected]>

replaced promauto with prometheus, as earlier was causing unnecessary panic

Signed-off-by: Yash Sharma <[email protected]>

changes for debug

Signed-off-by: Yash Sharma <[email protected]>

removed entry for receive test for e2e

Signed-off-by: Yash Sharma <[email protected]>

added a readiness probe

Signed-off-by: Yash Sharma <[email protected]>

fix E2E tests for receive router

Signed-off-by: yeya24 <[email protected]>

revert changes for cmd/main

Signed-off-by: Yash Sharma <[email protected]>

nitpicks

Signed-off-by: Yash Sharma <[email protected]>

removed redundant health check

Signed-off-by: Yash Sharma <[email protected]>

Add receive router registration

Signed-off-by: Yash Sharma <[email protected]>

not to be commited

Signed-off-by: Yash Sharma <[email protected]>

modified pkg/receive/handler.go

Signed-off-by: Yash Sharma <[email protected]>

modified pkg/receive/multitsdb.go

Signed-off-by: Yash Sharma <[email protected]>

modified pkg/route

Signed-off-by: Yash Sharma <[email protected]>

modified cmd/thanos/receive_route.go

Signed-off-by: Yash Sharma <[email protected]>

replaced MultiError with NonNilMultiError

Signed-off-by: Yash Sharma <[email protected]>

changed to promauto

Signed-off-by: Yash Sharma <[email protected]>

removed unused variables

Signed-off-by: Yash Sharma <[email protected]>

changed labelpb.Zlabel to timeseries

Signed-off-by: Yash Sharma <[email protected]>

whitespace removal and revert the labels

Signed-off-by: Yash Sharma <[email protected]>

use the main branch's implementation of MultiError

Signed-off-by: Yash Sharma <[email protected]>
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

I have just skimmed again and pointed out the major things. I'll review it in depth when I have time.

We should be super considered about merging this. @yashrsharma44 Please check all the subsequent PRs after this branched is created. And also we should have all the necessary facilities for the new subcommand. Moreover I don't see any changes on alerts or dashboard don't we have any changes regarding those parts?

I understand this is just a rebase effort but this could break receiver on master. If there are things missing in the previous PR, these should be documented by an issue or a document so that we can fix all those before the next major release. cc @Chans321

pkg/receive_route/hashring.go Outdated Show resolved Hide resolved
@@ -108,29 +108,6 @@ Flags:
TLS CA to verify clients against. If no client
CA is specified, there is no client
verification on server side. (tls.NoClientCert)
--remote-write.address="0.0.0.0:19291"
Copy link
Member

Choose a reason for hiding this comment

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

Yes, we remove these however, where are we adding those? We need to generate documentation for router as well.

@yashrsharma44
Copy link
Contributor Author

TODO:

  • Traverse PRs and add anything missing to this PR. - Need to be taken care in this PR
  • Make sure documentation for the new command exists. - Enable autogeneration of documentation for receive-router
  • Make sure dashboards and alerts work with the new command. - We can create an issue to track the shortcomings related to dashboards and alerts

Signed-off-by: Yash Sharma <[email protected]>
@bwplotka
Copy link
Member

bwplotka commented Apr 9, 2021

As promised, the modification of the proposal is ready: #4036 🤗 PTAL before moving with this PR.

@kakkoyun
Copy link
Member

Deprecated by #4231

@kakkoyun kakkoyun closed this May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants