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.

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

[Upgrade Assistant] External links with checkpoint time-range applied #111252

Merged
merged 11 commits into from
Sep 9, 2021
Merged

[Upgrade Assistant] External links with checkpoint time-range applied #111252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
061c777
Bound query around last checkpoint date
sabarasaba Sep 5, 2021
91d95df
Fix tests
sabarasaba Sep 5, 2021
f4c14d5
Also test discover url contains search params
sabarasaba Sep 6, 2021
b148eb2
Small refactor
sabarasaba Sep 6, 2021
c51fa30
Keep state about lastCheckpoint in parent component
sabarasaba Sep 6, 2021
ff6c599
Remove space
sabarasaba Sep 6, 2021
5bbda84
Merge branch '7.x' into ua/deep_linking_to_external_apps_with_last_ch…
kibanamachine Sep 6, 2021
273faa4
Merge branch '7.x' into ua/deep_linking_to_external_apps_with_last_ch…
kibanamachine Sep 7, 2021
e24f9df
Address CR changes
sabarasaba Sep 8, 2021
18d507c
Merge branch '7.x' into ua/deep_linking_to_external_apps_with_last_ch…
sabarasaba Sep 9, 2021
8ee5104
Address CR changes
sabarasaba Sep 9, 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
19 changes: 15 additions & 4 deletions x-pack/plugins/upgrade_assistant/__jest__/client_integration/helpers/app_context.mock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,24 @@ const idToUrlMap = {
SNAPSHOT_RESTORE_LOCATOR: 'snapshotAndRestoreUrl',
DISCOVER_APP_LOCATOR: 'discoverUrl',
};
type IdKey = keyof typeof idToUrlMap;

const stringifySearchParams = (params: Record<string, any>) => {
const stringifiedParams = Object.keys(params).reduce((list, key) => {
const value = typeof params[key] === 'object' ? JSON.stringify(params[key]) : params[key];

return { ...list, [key]: value };
}, {});

return new URLSearchParams(stringifiedParams).toString();
};

const shareMock = sharePluginMock.createSetupContract();
shareMock.url.locators.get = (id) => ({
// @ts-expect-error This object is missing some properties that we're not using in the UI
// @ts-expect-error This object is missing some properties that we're not using in the UI
shareMock.url.locators.get = (id: IdKey) => ({
useUrl: (): string | undefined => idToUrlMap[id],
// @ts-expect-error This object is missing some properties that we're not using in the UI
getUrl: (): string | undefined => idToUrlMap[id],
getUrl: (params: Record<string, any>): string | undefined =>
`${idToUrlMap[id]}?${stringifySearchParams(params)}`,
});

export const getAppContextMock = () => ({
Expand Down
23 changes: 21 additions & 2 deletions ...grade_assistant/__jest__/client_integration/overview/fix_logs_step/fix_logs_step.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,20 @@

import { act } from 'react-dom/test-utils';

// Once the logs team register the kibana locators in their app, we should be able
// to remove this mock and follow a similar approach to how discover link is tested.
// See: https://github.com/elastic/kibana/issues/104855
const MOCKED_TIME = '2021-09-05T10:49:01.805Z';
sabarasaba marked this conversation as resolved.
Show resolved Hide resolved
jest.mock('../../../../public/application/lib/logs_checkpoint', () => {
const originalModule = jest.requireActual('../../../../public/application/lib/logs_checkpoint');

return {
__esModule: true,
...originalModule,
loadLogsCheckpoint: jest.fn().mockReturnValue('2021-09-05T10:49:01.805Z'),
};
});

import { DeprecationLoggingStatus } from '../../../../common/types';
import { DEPRECATION_LOGS_SOURCE_ID } from '../../../../common/constants';
import { setupEnvironment } from '../../helpers';
Expand Down Expand Up @@ -180,7 +194,7 @@ describe('Overview - Fix deprecation logs step', () => {

expect(exists('viewObserveLogs')).toBe(true);
expect(find('viewObserveLogs').props().href).toBe(
`/app/logs/stream?sourceId=${DEPRECATION_LOGS_SOURCE_ID}`
`/app/logs/stream?sourceId=${DEPRECATION_LOGS_SOURCE_ID}&logPosition=(end:now,start:'${MOCKED_TIME}')`
);
});

Expand All @@ -194,7 +208,12 @@ describe('Overview - Fix deprecation logs step', () => {
component.update();

expect(exists('viewDiscoverLogs')).toBe(true);
expect(find('viewDiscoverLogs').props().href).toBe('discoverUrl');

const decodedUrl = decodeURIComponent(find('viewDiscoverLogs').props().href);
expect(decodedUrl).toContain('discoverUrl');
['"language":"kuery"', '"query":"@timestamp+>'].forEach((param) => {
expect(decodedUrl).toContain(param);
});
});
});

Expand Down
32 changes: 8 additions & 24 deletions ...ts/overview/fix_logs_step/deprecations_count_checkpoint/deprecations_count_checkpoint.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,13 @@
* 2.0.
*/

import React, { FunctionComponent, useState, useEffect } from 'react';
import React, { FunctionComponent, useEffect } from 'react';
import moment from 'moment-timezone';
import { FormattedDate, FormattedTime, FormattedMessage } from '@kbn/i18n/react';

import { i18n } from '@kbn/i18n';
import { EuiCallOut, EuiButton, EuiLoadingContent } from '@elastic/eui';
import { useAppContext } from '../../../../app_context';
import { Storage } from '../../../../../shared_imports';

const LS_SETTING_ID = 'kibana.upgradeAssistant.lastCheckpoint';
const localStorage = new Storage(window.localStorage);

const i18nTexts = {
calloutTitle: (warningsCount: number, previousCheck: string) => (
Expand Down Expand Up @@ -51,32 +47,22 @@ const i18nTexts = {
),
};

const getPreviousCheckpointDate = () => {
const storedValue = moment(localStorage.get(LS_SETTING_ID));

if (storedValue.isValid()) {
return storedValue.toISOString();
}

const now = moment().toISOString();
localStorage.set(LS_SETTING_ID, now);

return now;
};

interface Props {
checkpoint: string;
setCheckpoint: (value: string) => void;
setHasNoDeprecationLogs: (hasNoLogs: boolean) => void;
}

export const DeprecationsCountCheckpoint: FunctionComponent<Props> = ({
checkpoint,
setCheckpoint,
setHasNoDeprecationLogs,
}) => {
const {
services: { api },
} = useAppContext();
const [previousCheck, setPreviousCheck] = useState(getPreviousCheckpointDate());
const { data, error, isLoading, resendRequest, isInitialRequest } = api.getDeprecationLogsCount(
previousCheck
checkpoint
);

const logsCount = data?.count || 0;
Expand All @@ -87,9 +73,7 @@ export const DeprecationsCountCheckpoint: FunctionComponent<Props> = ({

const onResetClick = () => {
const now = moment().toISOString();

setPreviousCheck(now);
localStorage.set(LS_SETTING_ID, now);
setCheckpoint(now);
};

useEffect(() => {
Expand Down Expand Up @@ -126,7 +110,7 @@ export const DeprecationsCountCheckpoint: FunctionComponent<Props> = ({

return (
<EuiCallOut
title={i18nTexts.calloutTitle(logsCount, previousCheck)}
title={i18nTexts.calloutTitle(logsCount, checkpoint)}
color={calloutTint}
iconType={calloutIcon}
data-test-subj={calloutTestId}
Expand Down
36 changes: 24 additions & 12 deletions ...upgrade_assistant/public/application/components/overview/fix_logs_step/external_links.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { encode } from 'rison-node';
import React, { FunctionComponent, useState, useEffect } from 'react';

import { FormattedMessage } from '@kbn/i18n/react';
Expand All @@ -17,10 +18,12 @@ import {
DEPRECATION_LOGS_SOURCE_ID,
} from '../../../../../common/constants';

const getDeprecationIndexPatternId = async (dataService: DataPublicPluginStart) => {
const { indexPatterns: indexPatternService } = dataService;
interface Props {
checkpoint: string;
}

const results = await indexPatternService.find(DEPRECATION_LOGS_INDEX_PATTERN);
const getDeprecationIndexPatternId = async (dataService: DataPublicPluginStart) => {
const results = await dataService.dataViews.find(DEPRECATION_LOGS_INDEX_PATTERN);
sabarasaba marked this conversation as resolved.
Show resolved Hide resolved
// Since the find might return also results with wildcard matchers we need to find the
// index pattern that has an exact match with our title.
const deprecationIndexPattern = results.find(
Expand All @@ -30,15 +33,15 @@ const getDeprecationIndexPatternId = async (dataService: DataPublicPluginStart)
if (deprecationIndexPattern) {
return deprecationIndexPattern.id;
} else {
const newIndexPattern = await indexPatternService.createAndSave({
const newIndexPattern = await dataService.dataViews.createAndSave({
title: DEPRECATION_LOGS_INDEX_PATTERN,
allowNoIndex: true,
});
return newIndexPattern.id;
}
};

const DiscoverAppLink: FunctionComponent = () => {
const DiscoverAppLink: FunctionComponent<Props> = ({ checkpoint }) => {
const {
services: { data: dataService },
plugins: { share },
Expand All @@ -55,12 +58,19 @@ const DiscoverAppLink: FunctionComponent = () => {
return;
}

const url = await locator.getUrl({ indexPatternId });
const url = await locator.getUrl({
Copy link
Member Author

Choose a reason for hiding this comment

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

I dug a bit into useUrl and its implemented as a hook, which we cannot call from within the useEffect we have here. I'm leaving the getUrl instead because it will be more straightforward to read compared to having another top level hook that depends on the indexPatternId generated by this useEffect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps in a separate PR we can change https://github.com/elastic/kibana/blob/master/src/plugins/share/common/url_service/locators/use_locator_url.ts#L42 to define the dependencies more comprehensively:

  // Convert params object to string to be able to compare its value,
  // allowing the consumer to freely pass new objects to the hook on each
  // render without requiring it to be memoized.
  const paramsStringified = params ? JSON.stringify(params) : undefined;

  /* eslint-disable react-hooks/exhaustive-deps */
  useEffect(() => {
    if (!locator) {
      setUrl('');
      return;
    }

    locator
      .getUrl(params, getUrlParams)
      .then((result: string) => {
        if (!isMounted()) return;
        setUrl(result);
      })
      .catch((error) => {
        if (!isMounted()) return;
        // eslint-disable-next-line no-console
        console.error('useLocatorUrl', error);
        setUrl('');
      });
  }, [locator, paramsStringified, ...deps]);
  /* eslint-enable react-hooks/exhaustive-deps */

This would allow us to consume the hook, since it will update once the indexPatternId dependency has resolved:

  /* 
   * All this code replaces the current `useEffect`
   */

  const [indexPatternId, setIndexPatternId] = useState<string | undefined>();

  useEffect(() => {
    async function fetchIndexPatternId() {
      setIndexPatternId(await getDeprecationIndexPatternId(dataService));
    }
    fetchIndexPatternId();
  }, []);

  const locator = share.url.locators.get('DISCOVER_APP_LOCATOR');
  const discoverUrl = locator?.useUrl({
    indexPatternId,
    query: {
      language: 'kuery',
      query: `@timestamp > "${lastCheckpoint}"`,
    },
  });

Copy link
Member Author

Choose a reason for hiding this comment

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

👍🏼 I've created #111512 to track that

indexPatternId,
query: {
language: 'kuery',
query: `@timestamp > "${checkpoint}"`,
},
});

setDiscoveryUrl(url);
};

getDiscoveryUrl();
}, [dataService, share.url.locators]);
}, [dataService, checkpoint, share.url.locators]);

return (
<EuiLink href={discoveryUrl} data-test-subj="viewDiscoverLogs">
Expand All @@ -72,14 +82,16 @@ const DiscoverAppLink: FunctionComponent = () => {
);
};

const ObservabilityAppLink: FunctionComponent = () => {
const ObservabilityAppLink: FunctionComponent<Props> = ({ checkpoint }) => {
const {
services: {
core: { http },
},
} = useAppContext();
const logStreamUrl = http?.basePath?.prepend(
`/app/logs/stream?sourceId=${DEPRECATION_LOGS_SOURCE_ID}`
`/app/logs/stream?sourceId=${DEPRECATION_LOGS_SOURCE_ID}&logPosition=(end:now,start:${encode(
sabarasaba marked this conversation as resolved.
Show resolved Hide resolved
checkpoint
)})`
);

return (
Expand All @@ -92,7 +104,7 @@ const ObservabilityAppLink: FunctionComponent = () => {
);
};

export const ExternalLinks: FunctionComponent = () => {
export const ExternalLinks: FunctionComponent<Props> = ({ checkpoint }) => {
return (
<EuiFlexGroup>
<EuiFlexItem>
Expand All @@ -106,7 +118,7 @@ export const ExternalLinks: FunctionComponent = () => {
</p>
</EuiText>
<EuiSpacer size="m" />
<ObservabilityAppLink />
<ObservabilityAppLink checkpoint={checkpoint} />
</EuiPanel>
</EuiFlexItem>
<EuiFlexItem>
Expand All @@ -120,7 +132,7 @@ export const ExternalLinks: FunctionComponent = () => {
</p>
</EuiText>
<EuiSpacer size="m" />
<DiscoverAppLink />
<DiscoverAppLink checkpoint={checkpoint} />
</EuiPanel>
</EuiFlexItem>
</EuiFlexGroup>
Expand Down
16 changes: 13 additions & 3 deletions .../upgrade_assistant/public/application/components/overview/fix_logs_step/fix_logs_step.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import React, { FunctionComponent } from 'react';
import React, { FunctionComponent, useState, useEffect } from 'react';

import { i18n } from '@kbn/i18n';
import { EuiText, EuiSpacer, EuiPanel, EuiCallOut } from '@elastic/eui';
Expand All @@ -15,6 +15,7 @@ import { ExternalLinks } from './external_links';
import { DeprecationsCountCheckpoint } from './deprecations_count_checkpoint';
import { useDeprecationLogging } from './use_deprecation_logging';
import { DeprecationLoggingToggle } from './deprecation_logging_toggle';
import { loadLogsCheckpoint, saveLogsCheckpoint } from '../../../lib/logs_checkpoint';
import type { OverviewStepProps } from '../../types';

const i18nTexts = {
Expand Down Expand Up @@ -54,6 +55,11 @@ interface Props {

const FixLogsStep: FunctionComponent<Props> = ({ setIsComplete }) => {
const state = useDeprecationLogging();
const [checkpoint, setCheckpoint] = useState(loadLogsCheckpoint());

useEffect(() => {
saveLogsCheckpoint(checkpoint);
}, [checkpoint]);

return (
<>
Expand Down Expand Up @@ -86,14 +92,18 @@ const FixLogsStep: FunctionComponent<Props> = ({ setIsComplete }) => {
<h4>{i18nTexts.analyzeTitle}</h4>
</EuiText>
<EuiSpacer size="m" />
<ExternalLinks />
<ExternalLinks checkpoint={checkpoint} />

<EuiSpacer size="xl" />
<EuiText data-test-subj="deprecationsCountTitle">
<h4>{i18nTexts.deprecationsCountCheckpointTitle}</h4>
</EuiText>
<EuiSpacer size="m" />
<DeprecationsCountCheckpoint setHasNoDeprecationLogs={setIsComplete} />
<DeprecationsCountCheckpoint
checkpoint={checkpoint}
setCheckpoint={setCheckpoint}
setHasNoDeprecationLogs={setIsComplete}
/>
</>
)}
</>
Expand Down
30 changes: 30 additions & 0 deletions x-pack/plugins/upgrade_assistant/public/application/lib/logs_checkpoint.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import moment from 'moment-timezone';

import { Storage } from '../../shared_imports';

const SETTING_ID = 'kibana.upgradeAssistant.lastCheckpoint';
const localStorage = new Storage(window.localStorage);

export const loadLogsCheckpoint = () => {
const storedValue = moment(localStorage.get(SETTING_ID));

if (storedValue.isValid()) {
return storedValue.toISOString();
}

const now = moment().toISOString();
localStorage.set(SETTING_ID, now);

return now;
};

export const saveLogsCheckpoint = (value: string) => {
localStorage.set(SETTING_ID, value);
};
3 changes: 1 addition & 2 deletions x-pack/plugins/upgrade_assistant/public/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export class UpgradeAssistantUIPlugin
title: pluginName,
order: 1,
async mount(params) {
const [coreStart, { discover, data }] = await coreSetup.getStartServices();
const [coreStart, { data }] = await coreSetup.getStartServices();

const {
chrome: { docTitle },
Expand All @@ -61,7 +61,6 @@ export class UpgradeAssistantUIPlugin
core: coreStart,
data,
history: params.history,
discover,
api: apiService,
breadcrumbs: breadcrumbService,
},
Expand Down
3 changes: 0 additions & 3 deletions x-pack/plugins/upgrade_assistant/public/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
*/

import { ScopedHistory } from 'kibana/public';
import { DiscoverStart } from 'src/plugins/discover/public';
import { ManagementSetup } from 'src/plugins/management/public';
import { DataPublicPluginStart } from 'src/plugins/data/public';
import { SharePluginSetup } from 'src/plugins/share/public';
Expand All @@ -30,7 +29,6 @@ export interface SetupDependencies {

export interface StartDependencies {
licensing: LicensingPluginStart;
discover: DiscoverStart;
data: DataPublicPluginStart;
}

Expand All @@ -43,7 +41,6 @@ export interface AppDependencies {
};
services: {
core: CoreStart;
discover: DiscoverStart;
data: DataPublicPluginStart;
breadcrumbs: BreadcrumbService;
history: ScopedHistory;
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/upgrade_assistant/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"declarationMap": true
},
"include": [
"../../../typings/**/*",
sabarasaba marked this conversation as resolved.
Show resolved Hide resolved
"__jest__/**/*",
"common/**/*",
"public/**/*",
Expand Down