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

Federated targets functionality #3350

Merged
merged 1 commit into from
Apr 1, 2021
Merged

Federated targets functionality #3350

merged 1 commit into from
Apr 1, 2021

Conversation

2nick
Copy link
Contributor

@2nick 2nick commented Oct 21, 2020

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

Changes

Adding federated targets functionality to query and sidecar.

Verification

There is some e2e tests.

@2nick 2nick changed the title federated targets functionality (#1375) WIP: federated targets functionality Oct 21, 2020
@2nick 2nick force-pushed the federated-targets branch 3 times, most recently from 2f338c9 to f384750 Compare October 21, 2020 15:57
@2nick 2nick mentioned this pull request Oct 21, 2020
Closed
yeya24
yeya24 reviewed Oct 21, 2020
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Amazing work!

test/e2e/targets_api_test.go Outdated Show resolved Hide resolved
pkg/targets/targetspb/rpc.proto Show resolved Hide resolved
pkg/targets/prometheus.go Outdated Show resolved Hide resolved
pkg/query/storeset.go Outdated Show resolved Hide resolved
@yeya24
Copy link
Contributor

yeya24 commented Oct 21, 2020

Why .bingo/go.mod is changed from 755 to 644, is it related to this pr?

@2nick
Copy link
Contributor Author

2nick commented Oct 21, 2020

Why .bingo/go.mod is changed from 755 to 644, is it related to this pr?

No, this file is absolutely not related to this pr.

Don't know how it happened, but 644 looks more correct for it :)

Should I change it back to 755 and push?

2nick added a commit to 2nick/thanos that referenced this pull request Oct 22, 2020
@2nick
fix comments by PR review in code of federated targets functionality (t…
86adf14 
…hanos-io#3350)

Signed-off-by: Alexander Tunik <[email protected]>
@2nick
Copy link
Contributor Author

2nick commented Oct 22, 2020

@yeya24 comments fixed

Some e2e tests failed on my laptop too but second run was succeed - is there any known bug/reason for that behaviour?

2nick added a commit to 2nick/thanos that referenced this pull request Nov 2, 2020
@2nick
federated targets functionality (thanos-io#1375)
0316d64 
Signed-off-by: Alexander Tunik <[email protected]>

fix comments by PR review in code of federated targets functionality (thanos-io#3350)

Signed-off-by: Alexander Tunik <[email protected]>

Updating pkg/promclient Client.TargetsInGRPC to use new get2xxResultWithGRPCErrors method

Signed-off-by: Alexander Tunik <[email protected]>
@2nick 2nick requested a review from yeya24 November 3, 2020 09:34
@yeya24
Copy link
Contributor

yeya24 commented Nov 3, 2020

I will test this later. But generally, this pr looks good!

The only thing I am worried about is #2600.

bwplotka
bwplotka reviewed Nov 3, 2020
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think that is very solid PR, I love it 🤗

I would be even close to approving it, but it's quite big to review in detail right now. Generally LGTM! 💪

We talked @yeya24 around #2600 and we can do this in another PR. I think this PR shows nicely what we can reuse 🤗

pkg/targets/proxy.go Show resolved Hide resolved
@yeya24
Copy link
Contributor

yeya24 commented Nov 4, 2020

Hi @2nick, is this still WIP?

@2nick 2nick changed the title WIP: federated targets functionality federated targets functionality Nov 4, 2020
@2nick 2nick requested a review from bwplotka November 13, 2020 13:21
@2nick 2nick force-pushed the federated-targets branch 2 times, most recently from d2782b4 to d9796e8 Compare November 17, 2020 21:30
bwplotka
bwplotka reviewed Nov 18, 2020
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.

This is very solid work, I love it 💪
Thanks.

LGTM, just super minor nits (:

go.mod Outdated Show resolved Hide resolved
pkg/targets/proxy.go Show resolved Hide resolved
pkg/targets/targetspb/rpc.proto Show resolved Hide resolved
pkg/targets/proxy.go Show resolved Hide resolved
GiedriusS
GiedriusS reviewed Nov 19, 2020
View reviewed changes
cmd/thanos/query.go Outdated Show resolved Hide resolved
GiedriusS
GiedriusS reviewed Nov 19, 2020
View reviewed changes
pkg/api/query/v1.go Outdated Show resolved Hide resolved
GiedriusS
GiedriusS approved these changes Nov 19, 2020
View reviewed changes
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Solid work. Just a few minor nits and @bwplotka's comments are legit. Awesome work 💪

@bwplotka bwplotka changed the title federated targets functionality Federated targets functionality Dec 2, 2020
@bwplotka
Copy link
Member

bwplotka commented Dec 2, 2020

Any chances to do the last tiny steps @2nick so we can merge this? 🤗

squat
squat reviewed Mar 15, 2021
View reviewed changes
cmd/thanos/query.go Outdated Show resolved Hide resolved
squat
squat reviewed Mar 15, 2021
View reviewed changes
Copy link
Member

@squat squat left a comment

Choose a reason for hiding this comment

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

Looks pretty good! There are some small nits we could clean up

pkg/query/storeset.go Outdated Show resolved Hide resolved
pkg/targets/proxy.go Outdated Show resolved Hide resolved
pkg/targets/proxy.go Outdated Show resolved Hide resolved
@2nick 2nick force-pushed the federated-targets branch 3 times, most recently from abdb441 to 5578a59 Compare March 16, 2021 14:54
@2nick 2nick requested review from squat and bwplotka March 16, 2021 15:08
yeya24
yeya24 reviewed Mar 20, 2021
View reviewed changes
pkg/api/query/v1.go Outdated Show resolved Hide resolved
@2nick 2nick requested a review from yeya24 March 22, 2021 16:54
yeya24
yeya24 approved these changes Mar 22, 2021
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

This pr looks good to me.
Sorry I missed this pr and just merged the exemplars one. Would you mind doing another rebase?

@2nick 2nick force-pushed the federated-targets branch 2 times, most recently from 12e2945 to 0db9145 Compare March 22, 2021 19:49
@2nick 2nick force-pushed the federated-targets branch 3 times, most recently from 8b9e78e to edee39a Compare March 30, 2021 11:51
@2nick
Copy link
Contributor Author

2nick commented Mar 30, 2021

@bwplotka any chance of being merged? 😄

yeya24
yeya24 approved these changes Mar 30, 2021
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Would you mind updating the changelog? I think it is ready to merge.

@2nick 2nick force-pushed the federated-targets branch 3 times, most recently from 7542563 to ab6f27c Compare March 31, 2021 11:18
@2nick
Copy link
Contributor Author

2nick commented Mar 31, 2021

@yeya24 added line to changelog.

Tired of those flaky e2e tests - is it possible to skip them?

@yeya24
Copy link
Contributor

yeya24 commented Mar 31, 2021

@yeya24 added line to changelog.

Tired of those flaky e2e tests - is it possible to skip them?

Yeah, it is a flaky test. Let's skip that.

@bwplotka @squat Can you please give this pr a final look?

bwplotka
bwplotka approved these changes Apr 1, 2021
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.

This SOLID work. Massive thanks for patience and high quality code. We should definitely have it merged much sooner, sorry for lag on our side. LGTM! 💪🏽

CHANGELOG.md
@@ -12,6 +12,8 @@ We use _breaking :warning:_ to mark changes that are not backward compatible (re

## Unreleased

- [#3350](https://github.com/thanos-io/thanos/pull/3350) Query/Sidecar: Added targets API support. You can now configure you Querier to fetch Prometheus targets from leaf Prometheus-es!
Copy link
Member

Choose a reason for hiding this comment

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

Should be under # Added section I suppose? 🤗

Copy link
Member

Choose a reason for hiding this comment

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

Not a bigger, we can clean up later.

@bwplotka
Copy link
Member

bwplotka commented Apr 1, 2021

I restarted e2e job, let's wait until green (:

@bwplotka bwplotka enabled auto-merge (squash) April 1, 2021 16:33
@bwplotka bwplotka merged commit ea5104a into thanos-io:main Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwplotka bwplotka bwplotka approved these changes

@GiedriusS GiedriusS GiedriusS approved these changes

@yeya24 yeya24 yeya24 approved these changes

@kakkoyun kakkoyun Awaiting requested review from kakkoyun

@squat squat Awaiting requested review from squat

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@2nick @yeya24 @bwplotka @kakkoyun @GiedriusS @squat