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: Migrate Thanos Ruler UI to React #2865

Merged
merged 4 commits into from Jul 31, 2020
Merged

Conversation

onprem
Copy link
Member

@onprem onprem commented Jul 8, 2020

Signed-off-by: Prem Kumar prmsrswt@gmail.com

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

Changes

  • Enable the new React UI for Thanos Ruler

Verification

Tested locally with a basic alert in different state.

@onprem onprem marked this pull request as ready for review July 16, 2020 22:01
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.

The code looks good but I think we could do a couple of improvements before merging:

  • Let's include a link to the new UI in the old Ruler UI?
  • If you go to /new on Thanos Ruler then the link in the bar links to /graph which leads to 404

I had to do this #2925 to make api/v1/rules work at all for me 😄

@onprem
Copy link
Member Author

onprem commented Jul 22, 2020

The code looks good but I think we could do a couple of improvements before merging:

  • Let's include a link to the new UI in the old Ruler UI?

Fixed this in the new commit. Now the Classic UI link in navbar should point to the corresponding old UI page.

  • If you go to /new on Thanos Ruler then the link in the bar links to /graph which leads to 404

I had fixed this already, I am not sure why are you still getting this. /new is redirected to /new/ by the Go HTTP server, and from /new/ to /new${defaultRoute} is done on the client-side, where defaultRoute is /graph for Query and /alerts for Ruler.

I had to do this #2925 to make api/v1/rules work at all for me 😄

I wonder why it's working fine for me without that patch 🤔

@GiedriusS
Copy link
Member

The code looks good but I think we could do a couple of improvements before merging:

  • Let's include a link to the new UI in the old Ruler UI?

Fixed this in the new commit. Now the Classic UI link in navbar should point to the corresponding old UI page.

What about the other direction? :P We want the users to discover this.

Another thing: maybe let's set some other <title> according to the component? Now Thanos Ruler also has "Thanos Expression Browser" 😄

I have filled a separate issue for the Rules API weirdness so that we could unblock this. #2938. This PR doesn't touch that at all so I think we can merge this after changing those two small things! Awesome work, Prem 👍

@onprem
Copy link
Member Author

onprem commented Jul 25, 2020

Another thing: maybe let's set some other <title> according to the component? Now Thanos Ruler also has "Thanos Expression Browser"

As we have a single index.html file in React, we either need to use a common title that fits everywhere, as the old UI has, or we can use something like React Helmet to set the meta tags. Or maybe we can just use document.title 😃

@onprem onprem force-pushed the rule-new-ui branch 2 times, most recently from 8d2722f to 6a085f3 Compare July 28, 2020 09:10
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
@onprem
Copy link
Member Author

onprem commented Jul 28, 2020

Sorry for the rebase. Forgot to signoff some old commits and then merged master on top of it. Had to rebase on master and add signoff to the old commit messages.

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
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.

Looks great! 💪 Let's see if anyone else wants to review before merging.

@@ -223,6 +224,12 @@ export const encodePanelOptionsToQueryString = (panels: PanelMeta[]) => {
export const createExpressionLink = (expr: string) => {
return `../graph?g0.expr=${encodeURIComponent(expr)}&g0.tab=1&g0.stacked=0&g0.range_input=1h`;
Copy link
Member

Choose a reason for hiding this comment

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

We could improve this even more in the future by setting range_input to $now - $when_the_alert_has_started_firing but this is OK for now 💪

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
@bwplotka bwplotka merged commit 69b8760 into thanos-io:master Jul 31, 2020
@bwplotka
Copy link
Member

Thanks!

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.

None yet

3 participants