[Logs+] Add Log Explorer profile deep link #161939

tonyghiani · 2023-07-14T10:04:52Z

📓 Summary

closes #161748

The implementation registers an app updater on the discover-log-explorer plugin to update the deep link list.

The deep link is accessible using the discover:log-explorer link.

elasticmachine · 2023-07-14T10:04:55Z

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

apmmachine · 2023-07-14T10:05:08Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

…fix'

weltenwort

Seems to work well 👍 I just left a question about the code organization below.

weltenwort · 2023-07-14T10:36:30Z

packages/deeplinks/analytics/deep_links.ts

-export type DeepLinkId = AppId;
+export type AppId = DiscoverApp | DashboardApp | VisualizeApp;
+
+export type DiscoverAppProfile = 'log-explorer';


Would it be possible to keep the knowledge about the existence of the specific profile confined to the discover_log_explorer plugin? Why would it have to live here?

Based on the work done for using the deep link ids instead of the href, setting this type as part of the DeepLinkId union will allow accessing this deep link when it'll be referenced on the Observability serverless project navigation sidebar.

Ah, I see. Would it make sense to move it to the observability deep links package, then?

I can do it, I added this in the analytics package since the whole definition for discover was there, but make sense to have it in the obs package.

weltenwort · 2023-07-14T13:00:29Z

packages/deeplinks/observability/deep_links.ts

@@ -27,6 +27,8 @@ export type AppId =
  | ApmApp
  | MetricsApp;

+export type DiscoverLogExplorerId = 'discover:log-explorer';


Sorry for being so picky 😇 would it make sense to still interpolate DISCOVER_APP_ID here?

…fix'

isaclfreire · 2023-07-14T16:03:39Z

Great work, nice to see we're already tackling this!

Just a few small notes to address if possible:

consider having the o11y logo in the left
consider having 'Observability' instead of Analytics
we're missing an S in the name, if I'm not mistaken it should be Logs Explorer

…kibana into 161748-log-explorer-deeplink

tonyghiani · 2023-07-14T16:40:04Z

Hey @isaclfreire, thanks for the review, I just pushed a change that allows us to customize the deep link with our preferred category (before, it was always using the parent app, in this case, Discover), I think it now looks as expected:

…-ref HEAD~1..HEAD --fix'

tonyghiani · 2023-07-19T06:31:20Z

I think I understand what you mean. The change in behaviour of the application service you propose might have an impact on many Kibana plugins, though.

Apparently, it didn't break anything (the failing tests were caused by the appUpdater not executing and updating correctly the state because the first emitted updater function was overridden by the contributeState$ one, but your suggestion combining the states fix it).
You are probably right that it still affects the behaviour of many plugins and could have some side effects we wouldn't detect if not tested properly, I switched it back to how it was before.

Might be a workable solution if the Kibana core team would rather not change the core behaviour. What do you think?

Totally, I didn't think about combining them, doing so we get the expected behaviour back, thanks for the suggestion!

weltenwort

LGTM, very nicely done 👏

weltenwort · 2023-07-19T10:12:43Z

src/plugins/discover/public/customizations/profile_registry.test.ts

      customizationCallbacks: [callback, callback2],
+      deepLinks: [],


Should we add a test to ensure the deep links are emitted correctly by the appupdater?

Sure, I added a test for the registry behaviour and also a functional test as part of the customizations functional tests.
I'll make sure to also bring them into the serverless test suite in the follow-up work for [Log Explorer] Missing test suite for DatasetSelector in log-explorer profile#160627

weltenwort · 2023-07-19T10:16:09Z

src/plugins/discover/public/customizations/profile_registry.ts

+        map((profilesList) => {
+          const mergedDeepLinks = profilesList.flatMap((profile) => profile.deepLinks ?? []);
+          return getUniqueDeepLinks(mergedDeepLinks);
+        }),


Since we're applying getUniqueDeepLinks in the next map too, do we want to skip this duplicate call?

Suggested change

map((profilesList) => {

const mergedDeepLinks = profilesList.flatMap((profile) => profile.deepLinks ?? []);

return getUniqueDeepLinks(mergedDeepLinks);

}),

map((profilesList) => profilesList.flatMap((profile) => profile.deepLinks ?? [])),

weltenwort · 2023-07-19T11:32:13Z

x-pack/test_serverless/functional/test_suites/observability/navigation.ts

@@ -37,9 +37,11 @@ export default function ({ getPageObject, getService }: FtrProviderContext) {

      // TODO: test something oblt project specific instead of generic discover


Can we consider this TODO to be completed?

weltenwort · 2023-07-19T11:32:36Z

x-pack/test_serverless/functional/test_suites/observability/navigation.ts

@@ -79,11 +81,11 @@ export default function ({ getPageObject, getService }: FtrProviderContext) {
    it('navigate using search', async () => {
      await svlCommonNavigation.search.showSearch();
      // TODO: test something oblt project specific instead of generic discover


Can we consider this TODO to be completed?

…kibana into 161748-log-explorer-deeplink

…-fix'

davismcphee

Discover changes LGTM! 👍 I left a couple of very minor comments, but otherwise this approach makes sense to me. I followed the appUpdater conversation and agree with moving the deep link logic to the customization profiles. I also appreciate that the Discover changes to make it function were fairly minimal.

I'm approving now since the code changes all look good to me, but it would be great if you could address my last couple of comments before merging.

src/plugins/discover/public/customizations/profile_registry.ts

src/plugins/discover/public/customizations/types.ts

x-pack/plugins/discover_log_explorer/public/deep_links.ts

weltenwort · 2023-07-24T14:43:29Z

@elasticmachine merge upstream

kibana-ci · 2023-07-24T15:48:14Z

💚 Build Succeeded

Buildkite Build
Commit: a86ca52

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discoverLogExplorer`	149	150	+1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`discover`	71	79	+8

Async chunks

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

id	before	after	diff
`discover`	539.6KB	539.6KB	-8.0B
`discoverLogExplorer`	197.9KB	197.8KB	-33.0B
total			-41.0B

Page load bundle

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

id	before	after	diff
`discover`	29.4KB	30.1KB	+706.0B
`discoverLogExplorer`	4.5KB	5.2KB	+683.0B
`globalSearchProviders`	4.6KB	4.8KB	+199.0B
`serverlessObservability`	26.1KB	26.2KB	+110.0B
total			+1.7KB

Unknown metric groups

API count

id	before	after	diff
`discover`	97	105	+8

History

💚 Build #143791 succeeded 78c912d
💚 Build #143601 succeeded 46c0e4f
💔 Build #143593 failed 774f061
💚 Build #143532 succeeded 5456392
💔 Build #143481 failed 174e0f9
💔 Build #143459 failed aa305c5

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

weltenwort · 2023-07-24T15:52:45Z

Thanks for the repeated review, @davismcphee 🙏 I've applied your suggestions (except for the locator usage as explained in my response).

Co-authored-by: Marco Antonio Ghiani <[email protected]> Co-authored-by: kibanamachine <[email protected]> Co-authored-by: weltenwort <[email protected]>

feat(discover-log-explorer): add log explorer deep link

d16e9e8

tonyghiani added Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes labels Jul 14, 2023

tonyghiani requested review from a team as code owners July 14, 2023 10:04

weltenwort self-requested a review July 14, 2023 10:11

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

811fd23

…fix'

weltenwort reviewed Jul 14, 2023

View reviewed changes

refactor(discover-log-explorer): move deep link type to obs package

d8563ff

tonyghiani requested a review from a team as a code owner July 14, 2023 11:06

weltenwort reviewed Jul 14, 2023

View reviewed changes

Marco Antonio Ghiani added 2 commits July 14, 2023 15:17

refactor(discover-log-explorer): use DISCOVER_APP_ID variable

de57b35

refactor(discover-log-explorer): fix test

cf08ee8

tonyghiani removed request for a team July 14, 2023 13:20

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

411d4be

…fix'

Marco Antonio Ghiani added 2 commits July 14, 2023 18:34

feat(global-search): allow deeplinks to specify a different category

d588f9d

Merge branch '161748-log-explorer-deeplink' of github.com:tonyghiani/…

019c9cc

…kibana into 161748-log-explorer-deeplink

tonyghiani requested review from a team as code owners July 14, 2023 16:35

refactor(discover-log-explorer): update deeplink nam

e435d9f

Marco Antonio Ghiani and others added 2 commits July 17, 2023 09:14

test(global-search-providers): add test for category and icon precedence

4b77017

Merge branch 'main' into 161748-log-explorer-deeplink

b125a4e

Marco Antonio Ghiani and others added 2 commits July 19, 2023 08:23

feat(discover): combine observables

b90df7e

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

d05f991

…-ref HEAD~1..HEAD --fix'

tonyghiani and others added 2 commits July 19, 2023 08:31

Merge branch 'main' into 161748-log-explorer-deeplink

8d42c9f

refactor(tests): update navigation tests serverless oblt

f50acab

tonyghiani mentioned this pull request Jul 19, 2023

[Observability] Update the link to discover in the serverless navigation. #160205

Closed

3 tasks

tonyghiani added 2 commits July 19, 2023 11:01

Merge branch 'main' into 161748-log-explorer-deeplink

aa305c5

Merge branch 'main' into 161748-log-explorer-deeplink

174e0f9

weltenwort approved these changes Jul 19, 2023

View reviewed changes

tonyghiani and others added 9 commits July 19, 2023 15:03

Merge branch 'main' into 161748-log-explorer-deeplink

5456392

feat(discover-log-explorer): request changes

2367d17

Merge branch '161748-log-explorer-deeplink' of github.com:tonyghiani/…

ef42050

…kibana into 161748-log-explorer-deeplink

Merge branch 'main' into 161748-log-explorer-deeplink

774f061

Merge branch 'main' into 161748-log-explorer-deeplink

46c0e4f

fix(discover-log-explorer): update conflicting code

470fecd

fix(discover-log-explorer): update conflicting file

a6b9580

Merge branch 'main' into 161748-log-explorer-deeplink

88f36c5

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

78c912d

…-fix'

davismcphee approved these changes Jul 24, 2023

View reviewed changes

weltenwort added 2 commits July 24, 2023 14:36

qualify the type imports

ca476a6

Export profile-related types

b0361bb

Merge branch 'main' into 161748-log-explorer-deeplink

a86ca52

weltenwort merged commit 9bae853 into elastic:main Jul 24, 2023

kibanamachine added v8.10.0 backport:skip This commit does not require backporting labels Jul 24, 2023

tonyghiani deleted the 161748-log-explorer-deeplink branch July 28, 2023 07:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Logs+] Add Log Explorer profile deep link #161939

[Logs+] Add Log Explorer profile deep link #161939

tonyghiani commented Jul 14, 2023 •

edited by weltenwort

Loading

elasticmachine commented Jul 14, 2023

apmmachine commented Jul 14, 2023

weltenwort left a comment

weltenwort Jul 14, 2023

tonyghiani Jul 14, 2023

weltenwort Jul 14, 2023

tonyghiani Jul 14, 2023

weltenwort Jul 14, 2023

isaclfreire commented Jul 14, 2023

tonyghiani commented Jul 14, 2023

tonyghiani commented Jul 19, 2023

weltenwort left a comment

weltenwort Jul 19, 2023

tonyghiani Jul 19, 2023

weltenwort Jul 19, 2023

weltenwort Jul 19, 2023

weltenwort Jul 19, 2023

weltenwort Jul 19, 2023

davismcphee left a comment

weltenwort commented Jul 24, 2023

kibana-ci commented Jul 24, 2023

API count

weltenwort commented Jul 24, 2023

		customizationCallbacks: [callback, callback2],
		deepLinks: [],

		@@ -37,9 +37,11 @@ export default function ({ getPageObject, getService }: FtrProviderContext) {

		// TODO: test something oblt project specific instead of generic discover

[Logs+] Add Log Explorer profile deep link #161939

[Logs+] Add Log Explorer profile deep link #161939

Conversation

tonyghiani commented Jul 14, 2023 • edited by weltenwort Loading

📓 Summary

elasticmachine commented Jul 14, 2023

apmmachine commented Jul 14, 2023

🤖 GitHub comments

weltenwort left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaclfreire commented Jul 14, 2023

tonyghiani commented Jul 14, 2023

tonyghiani commented Jul 19, 2023

weltenwort left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davismcphee left a comment

Choose a reason for hiding this comment

weltenwort commented Jul 24, 2023

kibana-ci commented Jul 24, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

weltenwort commented Jul 24, 2023

tonyghiani commented Jul 14, 2023 •

edited by weltenwort

Loading