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

Jump to bottom

[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 5 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
17 changes: 13 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,22 @@ const idToUrlMap = {
SNAPSHOT_RESTORE_LOCATOR: 'snapshotAndRestoreUrl',
DISCOVER_APP_LOCATOR: 'discoverUrl',
};
type IdKey = keyof typeof idToUrlMap;

const convertObjectValuesToString = (params: Record<string, any>) => {
sabarasaba marked this conversation as resolved.
Show resolved Hide resolved
return Object.keys(params).reduce((list, key) => {
const value = typeof params[key] === 'object' ? JSON.stringify(params[key]) : params[key];

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

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]}?${new URLSearchParams(convertObjectValuesToString(params)).toString()}`,
});

export const getAppContextMock = () => ({
Expand Down
20 changes: 18 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,17 @@

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

const MOCKED_TIME = '2021-09-05T10:49:01.805Z';
sabarasaba marked this conversation as resolved.
Show resolved Hide resolved
jest.mock('../../../../public/application/lib/utils', () => {
const originalModule = jest.requireActual('../../../../public/application/lib/utils');

return {
__esModule: true,
...originalModule,
getLastCheckpointFromLS: jest.fn().mockReturnValue('2021-09-05T10:49:01.805Z'),
sabarasaba marked this conversation as resolved.
Show resolved Hide resolved
};
});

import { DeprecationLoggingStatus } from '../../../../common/types';
import { DEPRECATION_LOGS_SOURCE_ID } from '../../../../common/constants';
import { setupEnvironment } from '../../helpers';
Expand Down Expand Up @@ -145,7 +156,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 @@ -159,7 +170,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
36 changes: 12 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 } from 'react';
import React, { FunctionComponent } 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,26 +47,20 @@ 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);
interface Props {
lastCheckpoint: string;
resetLastCheckpoint: (value: string) => void;
sabarasaba marked this conversation as resolved.
Show resolved Hide resolved
}

return now;
};

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

const warningsCount = data?.count || 0;
Expand All @@ -80,9 +70,7 @@ export const DeprecationsCountCheckpoint: FunctionComponent = () => {

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

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

if (isInitialRequest && isLoading) {
Expand All @@ -109,7 +97,7 @@ export const DeprecationsCountCheckpoint: FunctionComponent = () => {

return (
<EuiCallOut
title={i18nTexts.calloutTitle(warningsCount, previousCheck)}
title={i18nTexts.calloutTitle(warningsCount, lastCheckpoint)}
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 {
lastCheckpoint: 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> = ({ lastCheckpoint }) => {
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 > "${lastCheckpoint}"`,
},
});

setDiscoveryUrl(url);
};

getDiscoveryUrl();
}, [dataService, share.url.locators]);
}, [dataService, lastCheckpoint, 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> = ({ lastCheckpoint }) => {
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
lastCheckpoint
)})`
);

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

export const ExternalLinks: FunctionComponent = () => {
export const ExternalLinks: FunctionComponent<Props> = ({ lastCheckpoint }) => {
return (
<EuiFlexGroup>
<EuiFlexItem>
Expand All @@ -106,7 +118,7 @@ export const ExternalLinks: FunctionComponent = () => {
</p>
</EuiText>
<EuiSpacer size="m" />
<ObservabilityAppLink />
<ObservabilityAppLink lastCheckpoint={lastCheckpoint} />
</EuiPanel>
</EuiFlexItem>
<EuiFlexItem>
Expand All @@ -120,7 +132,7 @@ export const ExternalLinks: FunctionComponent = () => {
</p>
</EuiText>
<EuiSpacer size="m" />
<DiscoverAppLink />
<DiscoverAppLink lastCheckpoint={lastCheckpoint} />
</EuiPanel>
</EuiFlexItem>
</EuiFlexGroup>
Expand Down
18 changes: 14 additions & 4 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, useCallback } 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 { getLastCheckpointFromLS, setLastCheckpointToLS } from '../../../lib/utils';

const i18nTexts = {
identifyStepTitle: i18n.translate('xpack.upgradeAssistant.overview.identifyStepTitle', {
Expand Down Expand Up @@ -49,6 +50,12 @@ const i18nTexts = {

const FixLogsStep: FunctionComponent = () => {
const state = useDeprecationLogging();
const [lastCheckpoint, setLastCheckpoint] = useState(getLastCheckpointFromLS());

const resetLastCheckpoint = useCallback((newValue: string) => {
sabarasaba marked this conversation as resolved.
Show resolved Hide resolved
setLastCheckpoint(newValue);
setLastCheckpointToLS(newValue);
}, []);

return (
<>
Expand All @@ -57,7 +64,7 @@ const FixLogsStep: FunctionComponent = () => {
</EuiText>
<EuiSpacer size="m" />
<EuiPanel>
<DeprecationLoggingToggle {...state} />
<DeprecationLoggingToggle {...state} />{' '}
</EuiPanel>

{state.onlyDeprecationLogWritingEnabled && (
Expand All @@ -81,14 +88,17 @@ const FixLogsStep: FunctionComponent = () => {
<h4>{i18nTexts.analyzeTitle}</h4>
</EuiText>
<EuiSpacer size="m" />
<ExternalLinks />
<ExternalLinks lastCheckpoint={lastCheckpoint} />

<EuiSpacer size="xl" />
<EuiText data-test-subj="deprecationsCountTitle">
<h4>{i18nTexts.deprecationsCountCheckpointTitle}</h4>
</EuiText>
<EuiSpacer size="m" />
<DeprecationsCountCheckpoint />
<DeprecationsCountCheckpoint
lastCheckpoint={lastCheckpoint}
resetLastCheckpoint={resetLastCheckpoint}
/>
</>
)}
</>
Expand Down
22 changes: 22 additions & 0 deletions x-pack/plugins/upgrade_assistant/public/application/lib/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@
* 2.0.
*/

import moment from 'moment-timezone';
import { pipe } from 'fp-ts/lib/pipeable';
import { tryCatch, fold } from 'fp-ts/lib/Either';

import { DEPRECATION_WARNING_UPPER_LIMIT } from '../../../common/constants';
import { Storage } from '../../shared_imports';

export const validateRegExpString = (s: string) =>
pipe(
Expand All @@ -34,3 +36,23 @@ export const getDeprecationsUpperLimit = (count: number) => {

return count.toString();
};

const LS_SETTING_ID = 'kibana.upgradeAssistant.lastCheckpoint';
const localStorage = new Storage(window.localStorage);
sabarasaba marked this conversation as resolved.
Show resolved Hide resolved

export const getLastCheckpointFromLS = () => {
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;
};

export const setLastCheckpointToLS = (value: string) => {
sabarasaba marked this conversation as resolved.
Show resolved Hide resolved
localStorage.set(LS_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