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
Add RGW Support in Object Service Dashboard #6070
Conversation
5753225
to
20ab6fe
Compare
curentDropdown, | ||
const parser = React.useMemo( | ||
() => | ||
// Todo (bipuladh): Fix data consumption utils to work with getInstantVectorStats |
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.
Won't be doing as part of 4.6.
setSelectedBreakdown={setBreakdownBy} | ||
selectedMetric={metric} | ||
setSelectedMetric={setMetric} | ||
isRgwSupported |
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.
@anmolsachan we need to determine where we are going to support it and where not.
name: _.truncate(`${r.x}`, { length: 12 }), | ||
link: `${r.x}`, | ||
color: Colors.LINK, | ||
name: labelNames ? labelNames[i] : _.truncate(`${r.x}`, { length: 12 }), |
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.
A question here - Shouldn't you be checking labelNames[i]
too here, before applying it in case labelNames[i]
is undefined?
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.
No need as there a few cases where we pass label names and for those cases, we can put the correct number of items, in the array. I have created a map for those special cases.
|
||
return results; | ||
}; | ||
|
||
type UsePrometheusQuery = (query: string, humanize: Humanize) => [HumanizeResult, any, number]; | ||
type UsePrometheusQueries = (queries: string[], metric?: string) => UsePrometheusQueriesResult; | ||
type UsePrometheusQueriesResult = [DataPoint[], boolean, any][]; | ||
// [data, loading, loadError] |
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.
Why this comment here?
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.
To make people aware of difference between loading and loaded. I see some interfaces using loading and some using loaded.
...ackages/noobaa-storage-plugin/src/components/capacity-breakdown/capacity-breakdown-card.scss
Show resolved
Hide resolved
queryKeys.forEach((q) => stopWatchPrometheusQuery(queries[q])); | ||
}; | ||
}, [watchPrometheus, stopWatchPrometheusQuery, metricType, queryKeys, queries]); | ||
const getDisablableSelectOptions = (dropdownItems: DropdownItems) => { |
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.
typo getDisableableSelectOptions
selections={[serviceType]} | ||
isGrouped | ||
placeholderText={`Type: ${serviceType}`} | ||
isCheckboxSelectionBadgeHidden |
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.
Please add id
isOpen={isOpenBreakdownSelect} | ||
selections={[metricType]} | ||
isGrouped | ||
placeholderText={`By: ${serviceType}`} |
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.
Please add id
setSelectedBreakdown={setBreakdownBy} | ||
selectedMetric={metric} | ||
setSelectedMetric={setMetric} | ||
isRgwSupported |
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.
isRgwSupported
-> isRGWSupported
might be better
componentsHealth: SubsystemHealth[], | ||
): { state: HealthState; message: string; count: number } => { | ||
const withPriority = componentsHealth.map((h) => healthPriority[h.state]); | ||
const withPriority = componentsHealth.map((h) => healthPriority?.[h.state] ?? {}); |
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.
no need for healthPriority?.[h.state]
const withPriority = componentsHealth.map((h) => healthPriority?.[h.state] ?? {}); | |
const withPriority = componentsHealth.map((h) => healthPriority[h.state] ?? {}); |
componentsHealth: SubsystemHealth[], | ||
): { state: HealthState; message: string; count: number } => { | ||
const withPriority = componentsHealth.map((h) => healthPriority[h.state]); | ||
const withPriority = componentsHealth.map((h) => healthPriority?.[h.state] ?? {}); | ||
const mostImportantState = Math.max(...withPriority.map(({ priority }) => priority)); |
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.
Maybe even better and less error prone would be to make sure that healthPriority
contains all Health States. Currently its lacking Loading state. Once we add it I think there's no reason to have separate healthStateMapping
and healthPriority
. We just include the priority into healthStateMapping
directly.
export const healthStateMapping: { [key in HealthState]: PriorityHealthState } = {
[HealthState.OK]: {
priority: 0,
health: HealthState.OK,
icon: <GreenCheckCircleIcon />,
},
[HealthState.UNKNOWN]: {
priority: 1,
health: HealthState.UNKNOWN,
icon: <GrayUnknownIcon />,
message: 'Unknown',
},
[HealthState.PROGRESS]: {
priority: 2,
health: HealthState.PROGRESS,
icon: <InProgressIcon />,
message: 'Pending',
},
[HealthState.UPDATING]: {
priority: 3,
health: HealthState.UPDATING,
icon: <BlueSyncIcon />,
message: 'Updating',
},
[HealthState.WARNING]: {
priority: 4,
health: HealthState.WARNING,
icon: <YellowExclamationTriangleIcon />,
message: 'Degraded',
},
[HealthState.ERROR]: {
priority: 5,
health: HealthState.ERROR,
icon: <RedExclamationCircleIcon />,
message: 'Degraded',
},
[HealthState.LOADING]: {
priority: 6,
health: HealthState.LOADING,
icon: <div className="skeleton-health" />,
},
[HealthState.NOT_AVAILABLE]: {
priority: 7,
health: HealthState.NOT_AVAILABLE,
icon: <GrayUnknownIcon />,
message: 'Not available',
},
};
Can you try that ?
Just looking at the screenshot - 'RADOS' is a terminology / details, we wish to expose to the customer? What's the benefit? |
|
import { getDataConsumptionChartData, numberInWords } from './data-consumption-card-utils'; | ||
import { DATA_CONSUMPTION_QUERIES } from '../../queries'; | ||
import './data-consumption-card.scss'; | ||
import { PrometheusResponse } from '@console/internal/components/graphs'; |
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.
This import should be moved before the relative ones.
9d3cb12
to
b604551
Compare
}, | ||
}; | ||
|
||
type HealthStateMappingKeys = keyof typeof HealthState; | ||
export type PriorityHealthState = { |
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.
seems like we dont need this one anymore as its the same as HealthStateMappingValues
/approve |
clearInterval(id); | ||
} | ||
}); | ||
id = setInterval(logicHandler, 10000); |
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.
nit: create variable for SECOND
id = setInterval(logicHandler, 10000); | |
id = setInterval(logicHandler, 10* SECOND); |
const provisioners = data.map((sc) => sc.provisioner); | ||
if (provisioners.includes('openshift-storage.ceph.rook.io/bucket')) { |
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.
const cephRGWProvisioner = 'openshift-storage.ceph.rook.io/bucket';
const checkRGWSCIsPresent = () => data.some((sc) => sc.provisioner === cephRGWProvisioner);
if(checkRGWSCIsPresent) {
...
}
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.
Probably use const file to define variable & checkRGWSCIsPresent
in utils & can be used later for checks.
/lgtm |
Add Support for Performance Card Added Dashboard upgrade for RGW
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bipuladh, cloudbehl, rawagner The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
Add Support for RGW in Top Consumers Card
Add Support for Performance Card
Added Dashboard upgrade for RGW