[Drilldowns] import dashboard url generator from plugin contract #64628

Dosant · 2020-04-28T09:29:30Z

Summary

Following up on #63108 (comment)
Following recommendation from: #62815

Instead of importing url generator using ID and registry, import it via plugin contract.

@stacey-gammon, should I create a ticket to clean url generator registry and to use the same approach in ML?

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-04-28T09:29:33Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

stacey-gammon · 2020-04-28T19:18:16Z

...dashboard_enhanced/public/services/drilldowns/dashboard_to_dashboard_drilldown/drilldown.tsx

@@ -153,7 +150,7 @@ export class DashboardToDashboardDrilldown
      }
    }

-    return getUrlGenerator(DASHBOARD_APP_URL_GENERATOR).createUrl({
+    return this.params.getDashboardUrlGenerator().createUrl!({


huh, having a hard time following this without pulling down the code... why would this possibly be undefined?

I just realised a difference...
When I started exporting directly and using directly it is UrlGeneratorsDefinition
But when we access from share plugin registry it is UrlGeneratorContract.
So url generator registry wrap the definition and adds additional logic. Which means this "refactor" is not 1 to 1 functionality match.

Not sure how to proceed from here in this case, but this pr is incorrect then right now...

ahhhh. Yea this is the problem discussed in that issue #62815. However, this PR has a solution for it: #62998 that may work. You'd have to change it to registerUrlGenerator returns the getter that will return the contract in the start life cycle.

Thanks!

share plugin is optional in dashboard. So I guess this also means that dashboardUrlGenerator will be optional on dashboard plugin contract?

somewhat of a bummer, but that share plugin contains the code for handling migrations automatically, which is why the Definition becomes a Contract. This may change when Persistable state registries becomes a formal, generic service. I would take the path of least resistance for now (require share plugin for this functionality), and we can revisit when we know more about how the persistable state registries will work.

@stacey-gammon please take a look 🙏
hope I followed the proposed solution correctly.

This needs further clean up: like removing global mappings and fixing how ML uses it, but I don't want to do this in this pr, because it points to drilldowns branch

stacey-gammon · 2020-04-30T14:13:44Z

x-pack/plugins/dashboard_enhanced/public/services/drilldowns/dashboard_drilldowns_services.ts

    const actionFlyoutCreateDrilldown = new FlyoutCreateDrilldownAction({ start });
    uiActions.addTriggerAction(CONTEXT_MENU_TRIGGER, actionFlyoutCreateDrilldown);

    const actionFlyoutEditDrilldown = new FlyoutEditDrilldownAction({ start });
    uiActions.addTriggerAction(CONTEXT_MENU_TRIGGER, actionFlyoutEditDrilldown);

    const dashboardToDashboardDrilldown = new DashboardToDashboardDrilldown({
-      getSavedObjectsClient,
+      start,


I think this is a move in the wrong direction. While it's very easy to just pass down start it encourages tight coupling to core. See https://www.notion.so/Tight-Coupling-in-Kibana-fcee0454319140d39aa5e83052bba901

Had a similar comment in a PR from Vadim, we can move this discussion outside this PR. Don't let it block you.

stacey-gammon

code review only

Dosant · 2020-05-06T08:28:01Z

@elasticmachine merge upstream

streamich · 2020-05-06T08:46:38Z

x-pack/plugins/dashboard_enhanced/public/services/drilldowns/dashboard_drilldowns_services.ts

+    const getDashboardUrlGenerator = () => {
+      const urlGenerator = start().plugins.dashboard.dashboardUrlGenerator;
+      if (!urlGenerator)
+        throw new Error('dashboardUrlGenerator is required for dashboard to dashboard drilldown');


This message contains error message specific for drilldowns.

... required for dashboard to dashboard drilldown

When somebody else in this plugin will start using the dashboard URL generator, this error message will not make sense for them.

But it is inside /dashboard_drilldowns_services.ts 🤔

Dosant · 2020-05-07T09:10:17Z

@elasticmachine merge upstream

Dosant · 2020-05-07T11:17:22Z

@majagrubic or @ThomThomson might taking a code owner look please? 🙏

ThomThomson

Tested locally on chrome and safari. Everything dashboard / Kibana App related still works as expected. I also took the opportunity to understand the broader context of this change, and that makes sense as well.

LGTM!

ThomThomson · 2020-05-07T20:00:52Z

...dashboard_enhanced/public/services/drilldowns/dashboard_to_dashboard_drilldown/drilldown.tsx

-    const { plugins } = this.params.start();
-
-    return plugins.share.urlGenerators.getUrlGenerator(DASHBOARD_APP_URL_GENERATOR).createUrl({
+    return this.params.getDashboardUrlGenerator().createUrl({


This looks much cleaner!

Dosant · 2020-05-08T13:13:10Z

@elasticmachine merge upstream

Dosant · 2020-05-11T05:32:51Z

@elasticmachine merge upstream

kibanamachine · 2020-05-11T07:40:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 46193dd

History

💔 Build #46611 failed 834c38c
💚 Build #46220 succeeded e27c67c
💔 Build #45877 failed 23516f2
💚 Build #45063 succeeded 18935b8
💚 Build #44351 succeeded a26e409

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

…ct (elastic#64628) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: (58 commits) [Drilldowns][chore] import dashboard url generator from plugin contract (elastic#64628) fix double flyouts in add panel flow (elastic#65861) Point 7.x to 7.9.0 in .backportrc.json Mount ui/new_platform applications in same div structure as Core (elastic#63930) [Uptime] Settings threshold validation (elastic#65454) Fix edit alert flyout to update initialAlert after edit (elastic#65359) Fix anomalies display on focused APM service map (elastic#65882) [SIEM][Detection Engine] Increases the template limit for ECS mappings [SIEM][CASE] Moves functional tests from "legacyEs" to "Es" (elastic#65851) [Metrics UI] Fix p95/p99 charts and alerting error (elastic#65579) [ML] Add job timing stats to anomaly jobs (elastic#65696) restore index pattern management data-test-subj's (elastic#64697) [Discover] Prevent whitespace wrapping of doc table header (elastic#52861) [SIEM] Fixes a CSS issue with Timeline field truncation (elastic#65789) Skipping failing tests. elastic#65867 elastic#65866 elastic#65865 [Discover] Deangularize the hits counter and create a react component (elastic#65631) Tsvb less update (elastic#65467) [TSVB] Remove remaining lodash.set usage (elastic#65846) [Uptime] Add `a11y` tests (elastic#65514) [Uptime] Enable loading on monitor list (elastic#65670) ...

…ct (#64628) (#65972) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Dosant added Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.8.0 labels Apr 28, 2020

Dosant requested review from streamich, stacey-gammon and a team April 28, 2020 09:29

Dosant mentioned this pull request Apr 28, 2020

[Drilldowns] Dashboard to dashboard drilldown #63108

Merged

7 tasks

stacey-gammon reviewed Apr 28, 2020

View reviewed changes

Dosant marked this pull request as draft April 29, 2020 09:35

stacey-gammon reviewed Apr 30, 2020

View reviewed changes

stacey-gammon approved these changes Apr 30, 2020

View reviewed changes

reset

18935b8

Dosant force-pushed the dev/import-dashboard-url-generator-directly branch from a26e409 to 18935b8 Compare May 4, 2020 15:34

Dosant changed the base branch from drilldowns to master May 4, 2020 15:35

Dosant marked this pull request as ready for review May 4, 2020 15:35

Dosant requested a review from a team as a code owner May 4, 2020 15:35

Merge branch 'master' into dev/import-dashboard-url-generator-directly

23516f2

botelastic bot added the Feature:Drilldowns Embeddable panel Drilldowns label May 6, 2020

streamich approved these changes May 6, 2020

View reviewed changes

Dosant requested review from majagrubic and ThomThomson May 7, 2020 09:09

Dosant added v7.9.0 and removed v7.8.0 labels May 7, 2020

Merge branch 'master' into dev/import-dashboard-url-generator-directly

e27c67c

ThomThomson approved these changes May 7, 2020

View reviewed changes

Merge branch 'master' into dev/import-dashboard-url-generator-directly

834c38c

Merge branch 'master' into dev/import-dashboard-url-generator-directly

46193dd

Dosant merged commit 387e28a into elastic:master May 11, 2020

Dosant deleted the dev/import-dashboard-url-generator-directly branch May 11, 2020 08:13

Dosant added a commit to Dosant/kibana that referenced this pull request May 11, 2020

[Drilldowns][chore] import dashboard url generator from plugin contra…

479ebc3

…ct (elastic#64628) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Dosant mentioned this pull request May 11, 2020

[7.x] [Drilldowns][chore] import dashboard url generator from plugin contract (#64628) #65972

Merged

Dosant mentioned this pull request Jun 16, 2020

[ML] Add Anomaly Swimlane Embeddable to the dashboard from the Anomaly Explorer page #68784

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Drilldowns] import dashboard url generator from plugin contract #64628

[Drilldowns] import dashboard url generator from plugin contract #64628

Dosant commented Apr 28, 2020

elasticmachine commented Apr 28, 2020

stacey-gammon Apr 28, 2020

Dosant Apr 29, 2020 •

edited

Loading

stacey-gammon Apr 29, 2020

Dosant Apr 29, 2020

stacey-gammon Apr 29, 2020 •

edited

Loading

Dosant Apr 29, 2020

stacey-gammon Apr 30, 2020

stacey-gammon Apr 30, 2020

stacey-gammon left a comment

Dosant commented May 6, 2020

streamich May 6, 2020

Dosant May 7, 2020

Dosant commented May 7, 2020

Dosant commented May 7, 2020

ThomThomson left a comment

ThomThomson May 7, 2020

Dosant commented May 8, 2020

Dosant commented May 11, 2020

kibanamachine commented May 11, 2020

[Drilldowns] import dashboard url generator from plugin contract #64628

[Drilldowns] import dashboard url generator from plugin contract #64628

Conversation

Dosant commented Apr 28, 2020

Summary

Checklist

For maintainers

elasticmachine commented Apr 28, 2020

Choose a reason for hiding this comment

Dosant Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon left a comment

Choose a reason for hiding this comment

Dosant commented May 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dosant commented May 7, 2020

Dosant commented May 7, 2020

ThomThomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dosant commented May 8, 2020

Dosant commented May 11, 2020

kibanamachine commented May 11, 2020

💚 Build Succeeded

History

Dosant Apr 29, 2020 •

edited

Loading

stacey-gammon Apr 29, 2020 •

edited

Loading