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

[sabarasaba]

Fixes #111069

External links to Observability and Discover apps should now have a time-range applied to them based on the last checkpoint. I also removed a few instances of the Discover app in the plugin deps which should not be there any more.

[sabarasaba]

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.

[cjcenizal]

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}"`,
    },
  });

[sabarasaba]

👍🏼 I've created #111512 to track that

[elasticmachine]

Pinging @elastic/kibana-stack-management (Team:Stack Management)

[sabarasaba]

@elasticmachine merge upstream

[cjcenizal]

@sabarasaba The test failure is due to a flaky test that is now skipped in 7.x, FYI. Gonna merge upstream to get CI passing.

[cjcenizal]

@elasticmachine merge upstream

[cjcenizal]

Tested locally, code LGTM! I had a few suggestions on ways to improve the readability of the code. I think we can also make changes to use_locator_url so that we can use it here, but I think that's outside this PR's scope.

[cjcenizal]

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}"`,
    },
  });

[sabarasaba]

@cjcenizal Thanks a lot for the review, I've addressed your points. Let me know if last commit is good and if so we can merge this one in 🚀

[cjcenizal]

Thanks for making these changes @sabarasaba! There was one small change from my original comments that wasn't addressed. Everything else looks great so please feel free to merge without another review.

[cjcenizal]

I think this name makes more sense if the function returns a string. In #111252 (comment) I suggested moving new URLSearchParams().toString() into the function to accomplish that -- WDYT?

[sabarasaba]

Somehow I miss read the previous comment, i patched that up and will merge when ci finishes 🚀

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 8ee5104
• Storybooks Preview
• Documentation Changes

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
upgradeAssistant	138	139	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
upgradeAssistant	208.7KB	209.1KB	+488.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
upgradeAssistant	27.2KB	27.2KB	+33.0B

Unknown metric groups

References to deprecated APIs

id	before	after	diff
upgradeAssistant	4	3	-1

History

• 💚 Build #151772 succeeded e24f9df
• 💚 Build #151589 succeeded 273faa4
• 💔 Build #151201 failed 5bbda84

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @sabarasaba

[LeeDr]

External links to Observability and Discover apps should now have a time-range applied to them based on the last checkpoint.

I'm not clear on the functionality of this time-range. Is it persisted somewhere? Or only used in a link in the browser of the user who clicked the checkpoint button?
If 2 or more admin users are trying to resolve deprecations, and one of them clicks the checkpoint button, does the other one get the same view?

[sabarasaba]

It's currently only persisted in the users browser using localStorage. So the list deprecations one user is seeing could be different to another depending on the persisted timestamp that is used to filter the results.