upcoming: [DI-20357] - Changes for ACLP Dashboard with Filters component

[venkymano-akamai]

Description 📝

Added a new CloudPulseDashboardWithFilters component which has the capability to build the filters needed in service provider page as well as the dashboard. This component will be integrated in the service provider pages.

Changes 🔄

1. Added new CloudPulseDashboardWithFilters component that has the capability to build the filters needed in service provider page as well as the dashboard. This will be integrated only in the service provider pages.
2. Modified the dbaas config to introduce a new filter nodeType and changed needInServicePage prop to false

Target release date 🗓️

03-09-2024

Preview 📷

Before	After

How to test 🧪

To test this, we need to integrate the CloudPulseDashboardWithFilters in a service Provider page. The below commit shows the test integration we made in database details index page

venkymano-akamai@e0f3598

1. Copy the code in the commit in the same file mentioned in the commit
2. Start the cloud manager UI application
3. Login and enable mock user as some API endpoints are not available still.
4. Navigate to any database instance details page, and navigate to the monitor tab.
5. You will see filters needed in service provide page as well as a mock dashboard rendered.

Note - We support only linode and dbaas, so the sample integration revolves around Managed Databases

As an Author I have considered 🤔

Check all that apply

• 👀 Doing a self review
• ❔ Our contribution guidelines
• 🤏 Splitting feature into small PRs
• ➕ Adding a changeset
• 🧪 Providing/Improving test coverage
• 🔐 Removing all sensitive information from the code and PR description
• 🚩 Using a feature flag to protect the release
• 👣 Providing comprehensive reproduction steps
• 📑 Providing or updating our documentation
• 🕛 Scheduling a pair reviewing session
• 📱 Providing mobile support
• ♿ Providing accessibility support

[github-actions]

Coverage Report: ✅
Base Coverage: 86.2%
Current Coverage: 86.2%

[jaalah-akamai]

There's so many grid components here, do we need all these for some future additions or something?
This can all be done with a single Box essentially

<>
        <Paper>
          <Box
            sx={{
              justifyContent: {
                sm: 'flex-start',
                xs: 'center',
              },
            }}
            display="flex"
            paddingX={2}
            paddingY={1}
          >
            <CloudPulseTimeRangeSelect
              disabled={!dashboard}
              handleStatsChange={handleTimeRangeChange}
              savePreferences={true}
            />
          </Box>
          <Divider />
          {isFilterBuilderNeeded && (
            <CloudPulseDashboardFilterBuilder
              dashboard={dashboard}
              emitFilterChange={onFilterChange}
              isServiceAnalyticsIntegration={true}
            />
          )}
          <Divider />
        </Paper>
        {isMandatoryFiltersSelected ? (
          <CloudPulseDashboard
            {...getDashboardProperties({
              dashboardObj: dashboard,
              filterValue,
              resource,
              timeDuration,
            })}
          />
        ) : (
          renderPlaceHolder('Mandatory Filters not Selected')
        )}
      </>

[venkymano-akamai]

@jaalah-akamai , in future we might add elements like TimeRangeSelect that are common for all service providers. I also agree we don't need this much Grids. I updated the code to have a single Grid Container, and placed the TimeRangeSelect inside it, so in future if we add elements, we can easily configure the responsiveness in grid and achieve the rows and columns of our needed choice. The below looks more or less the same as suggested approach.

<>
        <Paper>
          <Grid
            justifyContent={{
              sm: 'flex-end',
              xs: 'center',
            }}
            columnSpacing={2}
            container
            display={'flex'}
            item
            maxHeight={'120px'}
            mb={1}
            overflow={'auto'}
            px={2}
            py={1}
            rowGap={2}
            xs={12}
          >
            <Grid item md={4} sm={6} xs={12}>
              <CloudPulseTimeRangeSelect
                disabled={!dashboard}
                handleStatsChange={handleTimeRangeChange}
                savePreferences={true}
              />
            </Grid>
          </Grid>
          <Divider />
          {isFilterBuilderNeeded && (
            <CloudPulseDashboardFilterBuilder
              dashboard={dashboard}
              emitFilterChange={onFilterChange}
              isServiceAnalyticsIntegration={true}
            />
          )}
          {isFilterBuilderNeeded && <Divider />}
        </Paper>
        {isMandatoryFiltersSelected ? (
          <CloudPulseDashboard
            {...getDashboardProperties({
              dashboardObj: dashboard,
              filterValue,
              resource,
              timeDuration,
            })}
          />
        ) : (
          renderPlaceHolder('Mandatory Filters not Selected')
        )}
      </>

[coliu-akamai]

I feel like there might be a way to simplify a few of the methods in ReusableDashboardFilterUtils.ts in addition to what I commented on - will think on this a bit and get back to you!

[coliu-akamai]

nitpick - this comment could be removed, since the props are already required props/not marked as optional

[coliu-akamai]

We don't need this since we're checking whether serviceTypeConfig is truthy at line 69/70. We can either keep this check or lines 69/70, but we don't need both

[venkymano-akamai]

Yeah, here kept this and removed line 69 and 70

[coliu-akamai]

do we need this check? Doesn't seem like filter will ever be undefined/null:

[venkymano-akamai]

don't need, removed

[venkymano-akamai]

@coliu-akamai , I have addressed those you have given

[coliu-akamai]

Could we also have a test for an undefined dashboard?

[coliu-akamai]

could we add a test where we check for timeDuration and resource falsy values individually? So we can keep this test (but make resource here a truthy value), and then have another test like

let result = checkMandatoryFiltersSelected({
    dashboardObj: mockDashboard,
    filterValue: { region: 'us-east' },
    timeDuration: { unit: 'min', value: 30 },
    resource: 0,
  });
expect(result).toBe(false);

that way we're checking for each of the cases individually

[venkymano-akamai]

Added as individual test cases for time duration undefined value and resource 0 value

[coliu-akamai]

Can we add tests for checkIfFilterNeededInMetricsCall?

[coliu-akamai]

I noticed at line 69 you destructured filter - you could do something similar here if you want

Suggested change

	return serviceTypeConfig.filters.some((filter) => {
	const { configuration } = filter;
	return serviceTypeConfig.filters.some(({ configuration }) => {

[coliu-akamai]

thought about simplifying this a bit more but to be honest, there could be a bit of a readability tradeback. This is 100% optional in case you prefer this, but it's definitely not necessary (and having the if (!serviceTypeConfig) check might actually be a bit more readable and straightforward). If you end up going this route, you could do something similar with checkIfFilterBuilderNeeded too

export const checkIfFilterNeededInMetricsCall = (
  filterKey: string,
  serviceType: string
): boolean => {
  const serviceTypeConfig = FILTER_CONFIG.get(serviceType);

  return (
    serviceTypeConfig?.filters.some(({ configuration }) => {
      const {
        filterKey: configFilterKey,
        isFilterable,
        neededInServicePage,
      } = configuration;

      return (
        configFilterKey === filterKey &&
        Boolean(isFilterable) &&
        neededInServicePage // Indicates if this filter should be included in the metrics call
      );
    }) ?? false
  );
};

[venkymano-akamai]

Lets, have the readability one for now

[venkymano-akamai]

@coliu-akamai , Addressed the comments, can you please check if this is good now?

[coliu-akamai]

Thanks for the changes!

✅ confirmed mocked dashboard for dbaas instances