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

Adds support for rules screen in react-ui #6503

Merged
merged 13 commits into from Jan 27, 2020

Conversation

Harkishen-Singh
Copy link
Contributor

@Harkishen-Singh Harkishen-Singh commented Dec 21, 2019

Fixes: #6385
Fixes: #6302
Initial-version

preview-rules-page-gif

Update-1:
preview-rules-update-1-gif

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>
Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>
Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>
Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>
@Harkishen-Singh
Copy link
Contributor Author

@juliusv @boyskila please have a look.

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>
@Harkishen-Singh Harkishen-Singh changed the title [WIP] Adds support for rules screen in react-ui Adds support for rules screen in react-ui Dec 27, 2019
Copy link
Member

@juliusv juliusv left a comment

Choose a reason for hiding this comment

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

Thanks! This is a first round of comments. Currently the page doesn't load when alerting rules are defined, so let's fix these comments first before doing a second review.

web/api/v1/api.go Outdated Show resolved Hide resolved
web/api/v1/api.go Outdated Show resolved Hide resolved
web/api/v1/api.go Outdated Show resolved Hide resolved
web/web.go Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/Rules.tsx Outdated Show resolved Hide resolved
web/api/v1/api.go Outdated Show resolved Hide resolved
web/api/v1/api.go Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/Rules.tsx Outdated Show resolved Hide resolved
Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>
@Harkishen-Singh
Copy link
Contributor Author

@juliusv I have implemented the suggestions. Please have a look.

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>
Copy link
Contributor

@boyskila boyskila left a comment

Choose a reason for hiding this comment

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

@Harkishen-Singh Added some comments and ideas

@@ -1,15 +1,147 @@
import React, { FC } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you align response handling with the pattern used in other pages i.e check how withStatusIndicator HOC is used

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you align response handling with the pattern used in other pages i.e check how withStatusIndicator HOC is used

Better to merge this one first #6629

web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
@@ -1,15 +1,147 @@
import React, { FC } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you align response handling with the pattern used in other pages i.e check how withStatusIndicator HOC is used

Better to merge this one first #6629

@boyskila
Copy link
Contributor

boyskila commented Jan 18, 2020

@Harkishen-Singh On top of all the suggestions, I would say that there is inconsistency with the old UI in terms of font-size, font-family and text formatting. You can check CSS class rule_cell in the old UI - it will give you all you need to align both UI's.
Also, the back button is not working once you navigate to the graph page

Update: It turns out that the back button is not working once you navigate to the graph page via the expression link. This bug is already fixed with this PR #6659

Copy link
Member

@juliusv juliusv left a comment

Choose a reason for hiding this comment

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

Thanks, I gave this one more thorough review, added more comments to be addressed. Seems there are also still quite a few comments by @boyskila that would be good to fix.

};

const Rules: FC<RouteComponentProps & PathPrefixProps> = ({ pathPrefix }) => {
const { response, error } = useFetch<RulesMap>(`${pathPrefix}/api/v1/rules`);
Copy link
Member

Choose a reason for hiding this comment

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

#6629 is merged now, so it would be good to convert this to the same fetching pattern, as @boyskila said.

web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
<tr key={i}>
{r.alerts ? (
<td>
<strong>alert:</strong>
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if we didn't use CSS/HTML spacing (margin-left: 3%, <br/>, etc.) in this field and instead kept it all preformatted like in the old UI and use existing normal whitespace for the formatting, so it's properly copyable YAML (at least for the most part).

In the old UI the styling was achieved via:

.rule_cell {
    white-space: pre-wrap;
    background-color: #F5F5F5;
    display: block;
    font-family: monospace;
}

web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
web/ui/react-app/src/pages/rules/Rules.tsx Outdated Show resolved Hide resolved
Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>
Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>
Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>
@Harkishen-Singh
Copy link
Contributor Author

@juliusv @boyskila I have implemented the suggestions. Please have a look. Thank you.

@@ -237,3 +237,14 @@ button.execute-btn {
margin-right: 5px;
max-height: 20px;
}

.rules-head {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove rulse-head - it's not used anywhere

groups: RuleGroup[];
}

const GraphExpressionLink: FC<{ expr: string; title: string }> = props => {
Copy link
Contributor

@boyskila boyskila Jan 25, 2020

Choose a reason for hiding this comment

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

This component should look like:

export const GraphExpressionLink: FC<{ expr: string; title: string; name: string  & PathPrefixProps}> = memo(({ name, title, expr, pathPrefix }) => {
  return (
    <div>
      {title}:
      <Link className="ml-4" to={createExpressionLink(expr, pathPrefix)}>
        {name}
      </Link>
    </div>
  );
});

name and expr may have different content, so we can't rely on expr to represent the name
Also, wrapping the Link and title in div will remove the need of <br/>. IMO it's better.
Comparing with the old UI titles here are wrapped in <strong>, maybe you should remove the extra tag

Another, more important thing is to pas the pathPrefix prop down to createExpressionLink. IMO the ../ in createExpressionLink should be passed from outside and the function just to return
${prefix}graph?g0.expr=${encodeURIComponent(expr)}&g0.tab=1&g0.stacked=0&g0.range_input=1h
The usage should looks like createExpressionLink(expr, pathPrefix + '/../')

Suggestion: GraphExpressionLink can be moved to component folder and can replace both expressions here

<div>
name: <Link to={createExpressionLink(`ALERTS{alertname="${rule.name}"}`)}>{rule.name}</Link>
</div>
<div>
expr: <Link to={createExpressionLink(rule.query)}>{rule.query}</Link>
</div>
<div>
as well

{g.rules.map((r, i) => {
return (
<tr key={i}>
{r.alerts ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

<td> content can be reduced to

<td className="rule_cell">
      <GraphExpressionLink name={r.name} title={r.alerts ? 'alert' : 'record'} expr={`ALERTS{alertname="${r.name}"}`}/>
      <GraphExpressionLink title="expr" expr={r.query} name={r.query} />
         {r.alerts && (
              <>
                 labels:
                 {Object.entries(r.labels).map(([key, value]) => (
                     <div className="ml-4" key={key}>
                         {key}: {value}
                      </div>
                   ))}
                    annotations:
                    {Object.entries(r.annotations).map(([key, value]) => (
                        <div className="ml-4" key={key}>
                            {key}: {value}
                         </div>
                      ))}
                  </>
             )}
 </td>

Some of the divs are redundant and.
Alse, note that first GraphExpressionLink has as an expression ALERTS{alertname="${r.name}"}. Passing just r.name as expr is not enough and its not working

@@ -197,3 +197,7 @@ export const toQueryString = ({ key, options }: PanelMeta) => {
export const encodePanelOptionsToQueryString = (panels: PanelMeta[]) => {
return `?${panels.map(toQueryString).join('&')}`;
};

export const createExpressionLink = (expr: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please pass as second argument the pathPrefix prop and remove ../ - this should be passed from outside as well

@juliusv
Copy link
Member

juliusv commented Jan 27, 2020

Thanks! :) There are still a couple of new (and old, #6503 (comment)) outstanding comments, but I'll merge this for now, and then we can individually refactor/address the remaining few bits.

@juliusv juliusv merged commit c1e49d5 into prometheus:master Jan 27, 2020
boyskila pushed a commit to boyskila/prometheus that referenced this pull request Jan 29, 2020
* base

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

* base of rules page

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

* initial version

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

* removed unused function

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

* version 1

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

* implemented suggestions

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

* implemented suggestions

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

* implemented suggestions.

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

* new fetching pattern

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

* implemented suggestions

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>
Signed-off-by: blalov <boiskila@gmail.com>
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.

React UI: Create /rules page React UI: Figure out how to handle "Xs ago" duration displays
3 participants