Add direct access link registry and dashboard impl and use in ML #57496
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stacey-gammon
commented
Feb 12, 2020
| // template to inject the time parameters. | ||
| useHash: false, | ||
| }) | ||
| .then(urlValue => { |
There was a problem hiding this comment.
@peteharverson - would a service like this benefit ML? Unfortunately I had to strip the base path and the app and I manually re-added kibana prefix, since that's what you show users in the input box.
This would buffer the ml team at least from changes to the hash that dashboard app team may make. It looks like though you store the relative urls in saved objects? Have you thought about how to deal with migrations? in #25247 I discuss some issues with storing urls as string in saved objects. Ideally if we had this service you could store the generator id and the state and generate the url string on the fly, but since you expose this url string to the user, that may not be an option for you.
There was a problem hiding this comment.
@stacey-gammon yes ML would definitely benefit from having some sort of service to generate links to Discover and Dashboard, and indeed to any other apps such as APM, SIEM, Logs UI. The current process by which we build the URLs to Discover / Dashboard is cumbersome, and if the format of those URLs change (which to be fair hasn't happened over previous releases) then we need to edit the URL building code inside ML and all the custom URLs of ML jobs inside our pre-built modules.
When generating drilldowns from ML jobs, the properties we would need to store in the state are:
- dashboard ID / discover index pattern ID
- time range to set in the target page
- entities to pass to Discover / Dashboard to set in the 'filter', for example 'user: jane' and 'host: server1'. Currently we add these to the query part of the URL, as it is much easier to add them to a query than to the filter part. Not sure if we would want to allow the user to specify whether these entities should be set in the query or the filter - it might make more sense for the target app to decide how to use these filter entities?
The main reason we display the actual URL to the user in the edit job flyout / jobs list is historical. If we switched to this new service, we could just display controls for the state (saved object ID, time range, entities etc) rather than the full URL. This would also make it easier for the user to edit the custom URL, rather than the current method where they have to directly edit the long URL string, which is error-prone. We would then generate the actual URL on the fly, when the user clicked on the action link in the anomalies table for example. If a user for whatever reason couldn't use one of these new format URLs, then they could always use the other type of custom URL which allows them to enter the URL directly.
Note the custom URL links are stored within ML jobs (documents in ES, stored in the .ml-config index and accessed via GET _ml/anomaly_detectors), and not in Kibana saved objects. As part of the upcoming work to make ML jobs space-aware, we are planning on adding saved object 'wrappers' around ML jobs, so in theory, we could move the custom URLs into this new 'ML job saved object'.
We haven't had to deal with migrating custom URLs on upgrade to date - the format of the URLs seems to have fairly stable over 6.x and 7.x. But if we were to switch to this new service, and storing the generator ID and state on the job, we'd need to add custom upgrade functionality to check all the job custom URLs and convert any old style full URL ones to the new generator ID / state format.
There was a problem hiding this comment.
Thanks for the details @peteharverson. All that being said, is this specific change, in this PR, something you would like to see merged? There is much room for improvement with more refactoring/more registration of url generators, like discover, but for this initial PR, is this something you see as an improvement over the old code? Or would you prefer to wait until you can implement a larger refactoring yourself?
stacey-gammon
force-pushed
the
2020-02-11-url-service
branch
2 times, most recently
from
4e5f69d
to
87c03f1
Compare
stacey-gammon
force-pushed
the
2020-02-11-url-service
branch
3 times, most recently
from
a51df32
to
de2b278
Compare
stacey-gammon
force-pushed
the
2020-02-11-url-service
branch
8 times, most recently
from
eba1516
to
1c1c9f1
Compare
stacey-gammon
force-pushed
the
2020-02-11-url-service
branch
from
1c1c9f1
to
62962cd
Compare
7 tasks
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
stacey-gammon
added
the
release_note:skip
stacey-gammon
force-pushed
the
2020-02-11-url-service
branch
from
b1525b8
to
0da8260
Compare
| ); | ||
|
|
||
| return generator | ||
| .createUrl({ |
There was a problem hiding this comment.
@stacey-gammon testing your latest commits, I can confirm that the backslash / is now going in correctly.
src/plugins/dashboard_embeddable_container/public/url_generator.ts
Outdated
Show resolved
Hide resolved
timroes
reviewed
Mar 3, 2020
timroes
reviewed
Mar 3, 2020
src/plugins/dashboard_embeddable_container/public/url_generator.ts
Outdated
Show resolved
Hide resolved
timroes
reviewed
Mar 3, 2020
src/plugins/dashboard_embeddable_container/public/url_generator.ts
Outdated
Show resolved
Hide resolved
timroes
approved these changes
Mar 4, 2020
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / X-Pack Saved Object API Integration Tests -- security_and_spaces.x-pack/test/saved_object_api_integration/security_and_spaces/apis/find·ts.saved objects security and spaces enabled find rbac user with all globally within the default space "before all" hook for "finding a hiddentype should return 403 with forbidden find hiddentype message"Standard OutStack TraceKibana Pipeline / kibana-xpack-agent / X-Pack Saved Object API Integration Tests -- security_and_spaces.x-pack/test/saved_object_api_integration/security_and_spaces/apis/find·ts.saved objects security and spaces enabled find rbac user with all globally within the default space "after all" hook for "finding a hiddentype should return 403 with forbidden find hiddentype message"Standard OutStack TraceKibana Pipeline / kibana-xpack-agent / X-Pack Saved Object API Integration Tests -- security_and_spaces.x-pack/test/saved_object_api_integration/security_and_spaces/apis/find·ts.saved objects security and spaces enabled find rbac user with all globally within the default space "before all" hook for "finding a hiddentype should return 403 with forbidden find hiddentype message"Standard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
stacey-gammon
added a commit
to stacey-gammon/kibana
that referenced
this pull request
Mar 5, 2020
…stic#57496) * Add direct access link registry and dashboard impl and use in ML * Add example plugin with migration example * address code review comments * Fixes, more code review updates * Readme clean up * add tests * remove else * Rename everything from DirectAccessLinkGenerator to the much short UrlGenerator. also fix the ml # thing and return a relative link from dashboard genrator * add important text in bold * Move url generators into share plugin * add correct i18n prefix * Fix timeRange url name * make share plugin optional for dashboard * fix code owners * Use base UrlGeneratorState type, add comments * Fix hash bug and add test that would have caught it
stacey-gammon
added a commit
that referenced
this pull request
Mar 5, 2020
) (#59433) * Add direct access link registry and dashboard impl and use in ML * Add example plugin with migration example * address code review comments * Fixes, more code review updates * Readme clean up * add tests * remove else * Rename everything from DirectAccessLinkGenerator to the much short UrlGenerator. also fix the ml # thing and return a relative link from dashboard genrator * add important text in bold * Move url generators into share plugin * add correct i18n prefix * Fix timeRange url name * make share plugin optional for dashboard * fix code owners * Use base UrlGeneratorState type, add comments * Fix hash bug and add test that would have caught it
Closed
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Basic implementation and usage of #25247
cc @streamich - do you think this would help with drilldown link implementation?
@timroes @majagrubic - what do you think about app team owning the dashboard implementation of this?
TODO: This functionality should be exposed server side as well.
Example plugin added:
Modified the code so users can still call
.createUrlon a deprecated generator, as the base class handles getting the migrated version and callingcreateUrlon that one.link
@rudolf and @tylersmalley - this is similar to my thinking of how a generic registry would work with BWC.
This is how it's baked into the base implementation of
createDirectAccessLinkGenerator .createUrl: https://github.com/elastic/kibana/pull/57496/files#diff-400c5ae9c43c646080f80639e728d6edR131