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

UI: Enable Targets page in Querier UI #4045

Merged
merged 3 commits into from
Apr 21, 2021
Merged

Conversation

onprem
Copy link
Member

@onprem onprem commented Apr 11, 2021

Signed-off-by: Prem Saraswat [email protected]

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

Changes

  • Added the missing field globalUrl in proto, which was used in the UI
  • Added Targets page to the Querier nav bar

Verification

The json response format of Querier API matches with the response from Prometheus API.

Querier Targets page

yeya24
yeya24 previously approved these changes Apr 11, 2021
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.

LGTM!

pkg/targets/targetspb/rpc.proto Show resolved Hide resolved
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.

LGTM, just have one question: maybe it's time to make --target & other flags not hidden anymore? Because I think with this link in the UI we are going to inevitably get questions about what the link is for if it's nowhere to be found :s

@onprem
Copy link
Member Author

onprem commented Apr 11, 2021

maybe it's time to make --target & other flags not hidden anymore?

Oh, I was not aware that it is hidden. I also agree to make this not hidden.

@GiedriusS
Copy link
Member

maybe it's time to make --target & other flags not hidden anymore?

Oh, I was not aware that it is hidden. I also agree to make this not hidden.

Should we unhide them in a separate PR before merging this one? :P

@squat
Copy link
Member

squat commented Apr 12, 2021

maybe it's time to make --target & other flags not hidden anymore?

Oh, I was not aware that it is hidden. I also agree to make this not hidden.

Should we unhide them in a separate PR before merging this one? :P

👍

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

I would propose to not UNHIDE them until @hitanshu-mehta project of unificating those into single endpoint flag as planned in https://thanos.io/tip/proposals/202101_endpoint_discovery.md/ WDYT? 🤗

Otherwise LGTM!

@GiedriusS
Copy link
Member

I would propose to not UNHIDE them until @hitanshu-mehta project of unificating those into single endpoint flag as planned in https://thanos.io/tip/proposals/202101_endpoint_discovery.md/ WDYT?

I think it would lead to lots of questions about what is that page for and why one cannot any instructions about how to use it 🤔

@onprem
Copy link
Member Author

onprem commented Apr 16, 2021

So I guess one solution is to make targets page working but leave it out from the nav bar (similar to a hidden flag, a hidden page). So, whenever we decide to make the flag non-hidden (or we move to the --endpoint flag), we can make the page non-hidden too. WDYT?

This way, if anyone wants to really use it, they can use the hidden flag to point to sidecars and go directly to <querier-url>/new/targets.

@onprem onprem dismissed stale reviews from bwplotka and yeya24 via 18008eb April 20, 2021 19:53
onprem added 2 commits April 21, 2021 01:23
Signed-off-by: Prem Saraswat <[email protected]>
Signed-off-by: Prem Saraswat <[email protected]>
@onprem
Copy link
Member Author

onprem commented Apr 20, 2021

I have rebased this and made the Targets page hidden from Navbar (one can manually go to /new/targets though). If we want to unhide the --target flag and the Targets page, we can do that in a separate PR.

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.

LGTM 🚀

@onprem onprem merged commit f5174e0 into thanos-io:main Apr 21, 2021
@onprem onprem deleted the ui-targets branch April 21, 2021 10:31
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.

6 participants