Skip to content

[Unified observability] Refine typing for the data checks in the overview page #117994

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.

Sign up for GitHub

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

Merged
afgomez merged 10 commits into from
Nov 23, 2021
Merged

[Unified observability] Refine typing for the data checks in the overview page #117994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8b092fc
Load alerts directly from the alert component
Nov 9, 2021
9357ce3
Make the `hasAlerts` check boolean
Nov 9, 2021
0ce6266
Fix tests
Nov 10, 2021
ed86b6d
Merge branch 'main' into 117638-has-data-only-boolean
Nov 11, 2021
b0c6dc4
Simplify `hasAnyData` check
Nov 11, 2021
1923584
s/||/??
Nov 11, 2021
f082480
Merge branch 'main' into 117638-has-data-only-boolean
Nov 15, 2021
055a5e8
Merge branch 'main' into 117638-has-data-only-boolean
kibanamachine Nov 16, 2021
8494dbf
Merge branch 'main' into 117638-has-data-only-boolean
kibanamachine Nov 22, 2021
5b7c534
Merge branch 'main' into 117638-has-data-only-boolean
kibanamachine Nov 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 3 additions & 12 deletions x-pack/plugins/observability/public/components/app/empty_sections/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import { EuiFlexGrid, EuiFlexItem, EuiSpacer } from '@elastic/eui';
import React, { useContext } from 'react';
import { ThemeContext } from 'styled-components';
import { Alert } from '../../../../../alerting/common';
import { FETCH_STATUS } from '../../../hooks/use_fetcher';
import { useHasData } from '../../../hooks/use_has_data';
import { usePluginContext } from '../../../hooks/use_plugin_context';
Expand All @@ -21,17 +20,9 @@ export function EmptySections() {
const { hasDataMap } = useHasData();

const appEmptySections = getEmptySections({ core }).filter(({ id }) => {
if (id === 'alert') {
const { status, hasData: alerts } = hasDataMap.alert || {};
return (
status === FETCH_STATUS.FAILURE ||
(status === FETCH_STATUS.SUCCESS && (alerts as Alert[]).length === 0)
);
} else {
const app = hasDataMap[id];
if (app) {
return app.status === FETCH_STATUS.FAILURE || !app.hasData;
}
const app = hasDataMap[id];
if (app) {
return app.status === FETCH_STATUS.FAILURE || !app.hasData;
}
return false;
});
Expand Down
70 changes: 59 additions & 11 deletions x-pack/plugins/observability/public/components/app/section/alerts/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,18 @@ import {
EuiSpacer,
EuiTitle,
EuiButton,
EuiLoadingSpinner,
EuiCallOut,
} from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';
import moment from 'moment';
import React, { useState } from 'react';
import React, { useState, useMemo } from 'react';
import { EuiSelect } from '@elastic/eui';
import { uniqBy } from 'lodash';
import { Alert } from '../../../../../../alerting/common';
import { usePluginContext } from '../../../../hooks/use_plugin_context';
import { FETCH_STATUS, useFetcher } from '../../../../hooks/use_fetcher';
import { getObservabilityAlerts } from '../../../../services/get_observability_alerts';
import { paths } from '../../../../config';

const ALL_TYPES = 'ALL_TYPES';
Expand All @@ -34,20 +38,64 @@ const allTypes = {
}),
};

interface Props {
alerts: Alert[];
}

export function AlertsSection({ alerts }: Props) {
export function AlertsSection() {
const { config, core } = usePluginContext();
const [filter, setFilter] = useState(ALL_TYPES);
const manageLink = config.unsafe.alertingExperience.enabled
? core.http.basePath.prepend(paths.observability.alerts)
: core.http.basePath.prepend(paths.management.rules);
const filterOptions = uniqBy(alerts, (alert) => alert.consumer).map(({ consumer }) => ({
value: consumer,
text: consumer,
}));

const { data, status } = useFetcher(() => {
return getObservabilityAlerts({ core });
}, [core]);

const alerts = useMemo(() => data ?? [], [data]);

const filterOptions = useMemo(() => {
if (!alerts) {
return [];
}
return uniqBy(alerts, (alert) => alert.consumer).map(({ consumer }) => ({
value: consumer,
text: consumer,
}));
}, [alerts]);

const isError = status === FETCH_STATUS.FAILURE;
const isLoading = status !== FETCH_STATUS.SUCCESS && !isError;

if (isLoading) {
return (
<EuiFlexGroup alignItems="center" justifyContent="center" responsive={false}>
<EuiFlexItem grow={false}>
<EuiLoadingSpinner size="xl" />
</EuiFlexItem>
</EuiFlexGroup>
);
}

if (isError) {
return (
<EuiFlexGroup alignItems="center" justifyContent="spaceBetween" responsive={false}>
<EuiFlexItem>
<EuiCallOut
title={i18n.translate('xpack.observability.overview.alert.errorTitle', {
defaultMessage: "We couldn't load the alert data",
})}
Comment on lines +82 to +84
Copy link
Copy Markdown
Contributor

@claudiopro claudiopro Nov 11, 2021

Choose a reason for hiding this comment

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

Please move to a translations module

Copy link
Copy Markdown
Contributor Author

@afgomez afgomez Nov 11, 2021

Choose a reason for hiding this comment

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

I'm not sure I understand. You mean to take this out to a separate file?

Copy link
Copy Markdown
Contributor

@estermv estermv Nov 15, 2021

Choose a reason for hiding this comment

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

I think he means to move it to this file, but I'm not sure if these strings should be moved there.

What are the benefits of keeping the translations in one single place? I usually find this type of file difficult to maintain and I like to keep the translations inside the component. Not saying that I'm against it, but I would be happy to hear the reasoning behind that decision 😊

Copy link
Copy Markdown
Contributor Author

@afgomez afgomez Nov 16, 2021

Choose a reason for hiding this comment

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

I'm also not sure about the benefit of it. I feel it adds an extra layer of indirection.

@claudiopro could you elaborate on this?

Copy link
Copy Markdown
Contributor

@claudiopro claudiopro Nov 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, sorry for the lack of context! In RAC we use a layout where translation strings live in a translations module and import them where needed as constants, e.g. import * as i18n from './translations';. Not sure if choice is project specific, so feel free to ignore if your project has a different approach. @mgiota can provide more background. After a deeper look, it seems this approach was introduced recently by @ersin-erdal , even though @Zacqary had some doubts about generalizing it to the whole project #117488 (comment) . Feel free to ignore my comment 👍

color="danger"
iconType="alert"
>
<p>
<FormattedMessage
id="xpack.observability.overview.alert.errorMessage"
defaultMessage="There was an error loading the alert data. Try again later."
/>
</p>
</EuiCallOut>
</EuiFlexItem>
</EuiFlexGroup>
);
}

return (
<div>
Expand Down
23 changes: 10 additions & 13 deletions x-pack/plugins/observability/public/context/has_data_context.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ describe('HasDataContextProvider', () => {
infra_logs: { hasData: undefined, status: 'success' },
infra_metrics: { hasData: undefined, status: 'success' },
ux: { hasData: undefined, status: 'success' },
alert: { hasData: [], status: 'success' },
alert: { hasData: false, status: 'success' },
},
hasAnyData: false,
isAllRequestsComplete: true,
Expand Down Expand Up @@ -152,7 +152,7 @@ describe('HasDataContextProvider', () => {
hasData: false,
status: 'success',
},
alert: { hasData: [], status: 'success' },
alert: { hasData: false, status: 'success' },
},
hasAnyData: false,
isAllRequestsComplete: true,
Expand Down Expand Up @@ -210,7 +210,7 @@ describe('HasDataContextProvider', () => {
indices: 'apm-*',
status: 'success',
},
alert: { hasData: [], status: 'success' },
alert: { hasData: false, status: 'success' },
},
hasAnyData: true,
isAllRequestsComplete: true,
Expand Down Expand Up @@ -272,7 +272,7 @@ describe('HasDataContextProvider', () => {
indices: 'apm-*',
status: 'success',
},
alert: { hasData: [], status: 'success' },
alert: { hasData: false, status: 'success' },
},
hasAnyData: true,
isAllRequestsComplete: true,
Expand Down Expand Up @@ -315,7 +315,7 @@ describe('HasDataContextProvider', () => {
infra_logs: { hasData: undefined, status: 'success' },
infra_metrics: { hasData: undefined, status: 'success' },
ux: { hasData: undefined, status: 'success' },
alert: { hasData: [], status: 'success' },
alert: { hasData: false, status: 'success' },
},
hasAnyData: true,
isAllRequestsComplete: true,
Expand Down Expand Up @@ -364,7 +364,7 @@ describe('HasDataContextProvider', () => {
infra_logs: { hasData: undefined, status: 'success' },
infra_metrics: { hasData: undefined, status: 'success' },
ux: { hasData: undefined, status: 'success' },
alert: { hasData: [], status: 'success' },
alert: { hasData: false, status: 'success' },
},
hasAnyData: false,
isAllRequestsComplete: true,
Expand Down Expand Up @@ -429,7 +429,7 @@ describe('HasDataContextProvider', () => {
indices: 'apm-*',
status: 'success',
},
alert: { hasData: [], status: 'success' },
alert: { hasData: false, status: 'success' },
},
hasAnyData: true,
isAllRequestsComplete: true,
Expand Down Expand Up @@ -498,7 +498,7 @@ describe('HasDataContextProvider', () => {
infra_logs: { hasData: undefined, status: 'failure' },
infra_metrics: { hasData: undefined, status: 'failure' },
ux: { hasData: undefined, status: 'failure' },
alert: { hasData: [], status: 'success' },
alert: { hasData: false, status: 'success' },
},
hasAnyData: false,
isAllRequestsComplete: true,
Expand Down Expand Up @@ -527,7 +527,7 @@ describe('HasDataContextProvider', () => {
} as PluginContextValue);
});

it('returns all alerts available', async () => {
it('returns if alerts are available', async () => {
const { result, waitForNextUpdate } = renderHook(() => useHasData(), { wrapper });
expect(result.current).toEqual({
hasDataMap: {},
Expand All @@ -549,10 +549,7 @@ describe('HasDataContextProvider', () => {
infra_metrics: { hasData: undefined, status: 'success' },
ux: { hasData: undefined, status: 'success' },
alert: {
hasData: [
{ id: 2, consumer: 'apm' },
{ id: 3, consumer: 'uptime' },
],
hasData: true,
status: 'success',
},
},
Expand Down
9 changes: 3 additions & 6 deletions x-pack/plugins/observability/public/context/has_data_context.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { isEmpty, uniqueId } from 'lodash';
import React, { createContext, useEffect, useState } from 'react';
import { useRouteMatch } from 'react-router-dom';
import { asyncForEach } from '@kbn/std';
import { Alert } from '../../../alerting/common';
import { getDataHandler } from '../data_handler';
import { FETCH_STATUS } from '../hooks/use_fetcher';
import { usePluginContext } from '../hooks/use_plugin_context';
Expand All @@ -24,7 +23,7 @@ export type HasDataMap = Record<
DataContextApps,
{
status: FETCH_STATUS;
hasData?: boolean | Alert[];
hasData?: boolean;
Comment thread
afgomez marked this conversation as resolved.
indices?: string | ApmIndicesConfig;
serviceName?: string;
}
Expand Down Expand Up @@ -123,7 +122,7 @@ export function HasDataContextProvider({ children }: { children: React.ReactNode
setHasDataMap((prevState) => ({
...prevState,
alert: {
hasData: alerts,
hasData: alerts.length > 0,
status: FETCH_STATUS.SUCCESS,
},
}));
Expand All @@ -148,9 +147,7 @@ export function HasDataContextProvider({ children }: { children: React.ReactNode

const hasAnyData = (Object.keys(hasDataMap) as ObservabilityFetchDataPlugins[]).some((app) => {
const appHasData = hasDataMap[app]?.hasData;
return (
appHasData === true || (Array.isArray(appHasData) && (appHasData as Alert[])?.length > 0)
);
return appHasData === true;
});

return (
Expand Down
7 changes: 2 additions & 5 deletions x-pack/plugins/observability/public/pages/overview/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { EuiFlexGroup, EuiFlexItem, EuiSpacer, EuiPanel } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import React from 'react';
import { useTrackPageview } from '../..';
import { Alert } from '../../../../alerting/common';
import { EmptySections } from '../../components/app/empty_sections';
import { ObservabilityHeaderMenu } from '../../components/app/header';
import { NewsFeed } from '../../components/app/news_feed';
Expand Down Expand Up @@ -72,8 +71,6 @@ export function OverviewPage({ routeParams }: Props) {
docsLink: core.docLinks.links.observability.guide,
});

const alerts = (hasDataMap.alert?.hasData as Alert[]) || [];

const { refreshInterval = 10000, refreshPaused = true } = routeParams.query;

const bucketSize = calculateBucketSize({
Expand Down Expand Up @@ -118,10 +115,10 @@ export function OverviewPage({ routeParams }: Props) {
{!!newsFeed?.items?.length && <NewsFeed items={newsFeed.items.slice(0, 5)} />}
</EuiPanel>
</EuiFlexItem>
{!!alerts.length && (
{hasDataMap?.alert?.hasData && (
<EuiFlexItem>
<EuiPanel hasBorder={true}>
<AlertsSection alerts={alerts} />
<AlertsSection />
</EuiPanel>
</EuiFlexItem>
)}
Expand Down