-
Notifications
You must be signed in to change notification settings - Fork 44
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: Fetch alert history #3501
UI: Fetch alert history #3501
Conversation
Hello jbwatenbergscality,My role is to assist you with the merge of this Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
e10319e
to
d25dbdb
Compare
ConflictThere is a conflict between your branch Please resolve the conflict on the feature branch ( git fetch && \
git checkout origin/improvement/ui-alert-history && \
git merge origin/development/2.11 Resolve merge conflicts and commit git push origin HEAD:improvement/ui-alert-history |
45c1ab9
to
bb64655
Compare
25eb470
to
4fc7988
Compare
2936a76
to
eb78221
Compare
c495b8e
to
611469e
Compare
c503a5b
to
7a3aeff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have some requests to write some comments to help us to remember the issue we faced...
And thanks for rebasing to my branch again, it was not very clean before :/
ui/src/containers/Layout.js
Outdated
exact | ||
path="/" | ||
component={() => <Redirect to="/nodes" />} | ||
<AlertHistoryProvider> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am just wondering should we wrap this AlertHisotryProvider in App.js?
@@ -176,5 +176,6 @@ | |||
"core": "Core", | |||
"view_details": "View details", | |||
"start": "Start", | |||
"no_data_available_for_metrics": "No data available for displaying the Metrics" | |||
"no_data_available_for_metrics": "No data available for displaying the Metrics", | |||
"global_health_explanation": "The Global Health is the overall status of your Platform over a specific period.\nThe statuses of the Volumes and Nodes, the Network and the Services are monitored.\n(circle with StatusHealthy) OK, the Platform is healthy.\n(circle with StatusWarning) Warning, the Platform is degraded but not at risk.\n(circle with StatusCritical) Critical status, the Platform is degraded and at risk.\nHover or click on an alert segment on the Global Health bar for more details." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we don't have a french translation for this long text yet?
@@ -46,7 +48,7 @@ export function getAlertsLoki(start: string, end: string): Promise<Alert[]> { | |||
|
|||
return lokiApiClient | |||
.get( | |||
`/loki/api/v1/query_range?start=${start}&end=${end}&query=${METALK8S_HISTORY_ALERTS_QUERY}`, | |||
`/loki/api/v1/query_range?start=${start}&end=${end}&limit=1000&query=${METALK8S_HISTORY_ALERTS_QUERY}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we write down the reason why we set the limit to 1000?
ui/src/containers/DashboardPage.js
Outdated
@@ -122,11 +113,15 @@ const DashboardPage = (props: {}) => { | |||
onClick: () => { | |||
writeUrlTimeSpan(option); | |||
}, | |||
selected: queryTimeSpansCodes.find(timespan => timespan.duration === metricsTimeSpan)?.label === option, | |||
selected: | |||
queryTimeSpansCodes.find((item) => item.label === label)?.label === |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI:
I exposed the queryTimeSpansCodes
from core-ui
as well ;) To make sure we have a single source of truth.
const newCurrentDate = new Date(); | ||
setCurrentTime(newCurrentDate); | ||
const newCurrentDate = new Date().getTime(); | ||
const newCurrentTime = newCurrentDate - (newCurrentDate % (frequency * 1000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here it worth writiing down why we did this change (what problem we were trying to solve)
611469e
to
d184ffa
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
afd040f
to
de57104
Compare
ConflictThere is a conflict between your branch Please resolve the conflict on the feature branch ( git fetch && \
git checkout origin/improvement/ui-alert-history && \
git merge origin/development/2.11 Resolve merge conflicts and commit git push origin HEAD:improvement/ui-alert-history |
9bb6a9e
to
2a2e3ea
Compare
… avoid whole displayed charts to change over time and simulate an effect where new entries move the charts to the right
…is smaller than 12 hours
Remove the ServiceItem from DashboardServices and create the HealthItem component with it Add DashboardPlane component containing 2 HealthItems Add tests for the new DashbordPlane component history.push replaced by history.replace in the HealthItem component Ref: #3511
2a2e3ea
to
797706f
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
/approve |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following options are set: approve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue None. Goodbye jbwatenbergscality. |
Component:
ui
Context:
In order to display the Global health component we have to provide a way for the UI to retrieve alert history.
Summary:
This PR improve the behavior of
useHistoryAlert
to take in account platform downTimes.Acceptance criteria:
Unit testing