Add search deep links for APM, Metrics, Logs, and Dev Tools #96135
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
joshdover
added
release_note:enhancement
Team:Core
sorenlouv
reviewed
Apr 6, 2021
sorenlouv
reviewed
Apr 6, 2021
Comment on lines
+145
to
+165
| { | ||
| id: 'services', | ||
| title: i18n.translate('xpack.apm.breadcrumb.servicesTitle', { | ||
| defaultMessage: 'Services', | ||
| }), | ||
| path: '/services', | ||
| }, | ||
| { | ||
| id: 'traces', | ||
| title: i18n.translate('xpack.apm.breadcrumb.tracesTitle', { | ||
| defaultMessage: 'Traces', | ||
| }), | ||
| path: '/traces', | ||
| }, | ||
| { | ||
| id: 'service-map', | ||
| title: i18n.translate('xpack.apm.breadcrumb.serviceMapTitle', { | ||
| defaultMessage: 'Service Map', | ||
| }), | ||
| path: '/service-map', | ||
| }, |
There was a problem hiding this comment.
If we implement a getDeepLink that pulls the necessary info from the routes obejct, we can probably replace this with something like:
Suggested change
| { | |
| id: 'services', | |
| title: i18n.translate('xpack.apm.breadcrumb.servicesTitle', { | |
| defaultMessage: 'Services', | |
| }), | |
| path: '/services', | |
| }, | |
| { | |
| id: 'traces', | |
| title: i18n.translate('xpack.apm.breadcrumb.tracesTitle', { | |
| defaultMessage: 'Traces', | |
| }), | |
| path: '/traces', | |
| }, | |
| { | |
| id: 'service-map', | |
| title: i18n.translate('xpack.apm.breadcrumb.serviceMapTitle', { | |
| defaultMessage: 'Service Map', | |
| }), | |
| path: '/service-map', | |
| }, | |
| getDeepLink('services'), | |
| getDeepLink('traces'), | |
| getDeepLink('service-map'), | |
Not needed for this PR though.
sorenlouv
approved these changes
Apr 6, 2021
joshdover
force-pushed
the
add-solution-deep-links
branch
from
f469fc5
to
9fb9c4d
Compare
|
Pinging @elastic/kibana-core (Team:Core) |
sorenlouv
approved these changes
Apr 12, 2021
pgayvallet
reviewed
Apr 14, 2021
Comment on lines
+96
to
+97
| title: tool.title as string, | ||
| path: `#/${tool.id}`, |
There was a problem hiding this comment.
Why did you change DevToolApp.title from string to ReactNode to then forcecast it to string here?
There was a problem hiding this comment.
Yeah that wasn't super clear was it. It's because there is a dev tool (Painless Lab) that is passing a component here to be rendered in the tabs UI. So I fixed the type on DevToolApp to be accurate and then filtered that dev tool out (for now) and casted this to a string.
|
@elasticmachine merge upstream |
pgayvallet
approved these changes
Apr 16, 2021
💚 Build Succeeded
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
kibanamachine
added a commit
to kibanamachine/kibana
that referenced
this pull request
Apr 19, 2021
…96135) * Add searchDeepLinks for APM * Add searchDeepLinks for Metrics and Logs * Add searchDeepLinks for Dev Tools Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
kibanamachine
added a commit
that referenced
this pull request
Apr 19, 2021
…97511) * Add searchDeepLinks for APM * Add searchDeepLinks for Metrics and Logs * Add searchDeepLinks for Dev Tools Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Josh Dover <1813008+joshdover@users.noreply.github.com>
cauemarcondes
added
the
apm:test-plan-done
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.
Summary
Partially addresses #92153
Right now this PR adds deep links to Kibana's search for the APM, Metrics, Logs, and Dev Tools apps.
For APM, Metrics, and Logs I accomplished this by copying the route paths and translation strings. I've left comments explaining that these need to be kept in sync across files. Of course we should probably extract these into a common file, but that should be done by the individual teams and we'd like to get this feature rolled out more widely in the meantime.
Dev Tools should be fine as-is without needing any changes by the team. The only caveat is it does not currently add a link for the "Painless Lab" tool because that tool does not provide a string title, only a React component, so I was unable to add that one. It should be addressed separately.
This PR does not currently include any tests. I'm not inclined to add any except for the Dev Tools app since it actually contains logic changes, but it would be good for test coverage to be added in the future to be sure that these URLs are correct. I am electing to delegate that to the individual solution teams. I'm opening this PR now to get feedback on this approach before adding the tests for Dev Tools.
I was unable to add deep links for the Monitoring and Fleet apps at this time. Fleet is using a hash-based router which is not actually supported by the platform, so in-app navigations using search are currently broken. I've opened an issue to discuss this: #96134. Monitoring faces a similar problem, but it is using Angular which is also not 100% compatible with the Platform's router. They will need to migrate away from Angular before I think we can solve this.
@elastic/apm-ui @elastic/observability-ui @elastic/es-ui Feedback would be appreciated before I continue forward. Thanks all!
Checklist
Delete any items that are not applicable to this PR.
For maintainers