[Discover] Add support for contextual awareness functional tests #185905

davismcphee · 2024-06-10T22:42:42Z

Summary

This PR adds functional test support for the Discover contextual awareness framework, and adds tests for the initial getCellRenderers extension point using example profiles.

To support this, this PR introduces a new YAML setting called discover.experimental.enabledProfiles which can be used to selectively enable profiles both for functional testing and demoing WIP profiles that aren't yet ready for GA.

Example usage:

discover.experimental.enabledProfiles: ['example-root-profile', 'example-data-source-profile', 'example-document-profile']

Flaky test runs:

Stateful x50: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6304
Serverless Observability x50: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6305
Serverless Search x50: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6306
Serverless Security x50: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6307

Resolves #184699.

Checklist

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
Flaky Test Runner was used on any tests changed
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

davismcphee · 2024-06-11T01:12:57Z

/ci

…mentation

davismcphee · 2024-06-13T00:02:06Z

/ci

davismcphee · 2024-06-13T00:08:38Z

/ci

davismcphee · 2024-06-13T01:00:37Z

/ci

kibanamachine · 2024-06-13T01:39:54Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6304

[✅] test/functional/apps/discover/context_awareness/config.ts: 50/50 tests passed.

see run history

kibanamachine · 2024-06-13T01:43:30Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6307

[✅] x-pack/test_serverless/functional/test_suites/security/config.context_awareness.ts: 50/50 tests passed.

see run history

kibanamachine · 2024-06-13T01:45:02Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6305

[✅] x-pack/test_serverless/functional/test_suites/observability/config.context_awareness.ts: 50/50 tests passed.

see run history

kibanamachine · 2024-06-13T01:46:54Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6306

[✅] x-pack/test_serverless/functional/test_suites/search/config.context_awareness.ts: 50/50 tests passed.

see run history

davismcphee · 2024-06-13T03:07:20Z

/ci

elasticmachine · 2024-06-13T03:08:01Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

jughosta

LGTM 👍

src/plugins/discover/public/context_awareness/__mocks__/index.ts

tonyghiani · 2024-06-18T11:16:19Z

@davismcphee I just finished a first pass on the changes at a high-level, here some thoughts:

Working with esArchiver for the FT makes it more difficult to debug and update test data as mock data are gzipped and there is no explicit data to look over while debugging.
I believe switching to synthtrace will make more manageable the fake data and avoid the configuration of additional esArchives, here you can find a quick example of ad-hoc logs ingested for an FT.
The new discover.experimental.enabledProfiles setting requires every developer to know about all the available profile ids and set them manually to develop the contextual awareness architecture. I'm not sure if this is easily maintainable as many devs might stay out of sync if they don't set/update this setting.
Is there any particular benefit I am missing here for why we need this setting first? If the matter is keeping it disabled while developing, we can just not pass it on the list of registered providers, otherwise, I can't think of a reason why a ready-to-production profile should be disabled.
The example profiles are useful to understand the capabilities of each profile type, although I wonder if makes sens to register them on the main system where we also add the real profiles. If it's for testing/documentation purpose only, shall we move them out and register them on a test env only?
The profile manager init and profiles registration start having more logic around it (singleton profiles registration step, depends on config, need services injection, dynamic loading, ...) and the plugin entry point has already knowledge of more and more of it (this.profileProvidersRegistered).
Should we consider extracting these parts into a more encapsulated service, so that a client is instantiated lazily on demand, and all these implementation details don't pertain to the main plugin? I'm not leaving examples but I have some to provide, happy to talk more about this 👌

davismcphee · 2024-06-18T20:29:45Z

Thanks for the review @tonyghiani, and some responses to your feedback:

Working with esArchiver for the FT makes it more difficult to debug and update test data as mock data are gzipped and there is no explicit data to look over while debugging.
I believe switching to synthtrace will make more manageable the fake data and avoid the configuration of additional esArchives, here you can find a quick example of ad-hoc logs ingested for an FT.

Neat, I actually didn't realize you could use synthtrace as a service in functional tests since it's not something I've seen used in Platform tests. That said, I'd prefer to keep using the esArchiver approach for now to avoid blocking this PR to refactor how the example data is ingested.

The example data should be very simple and predictable, not reflective of real world data, to make it easy to assert against specific index names, field contents, etc. I'm sure we could do this with synthtrace too, but that would basically just be another way of hardcoding it through a different API. I'd rather use synthtrace for cases where we want realistic data like when testing the actual logs profiles. But if you feel strongly about this, I'd accept a followup PR that migrates the example data to be ingested through synthtrace instead.

The new discover.experimental.enabledProfiles setting requires every developer to know about all the available profile ids and set them manually to develop the contextual awareness architecture. I'm not sure if this is easily maintainable as many devs might stay out of sync if they don't set/update this setting.
Is there any particular benefit I am missing here for why we need this setting first? If the matter is keeping it disabled while developing, we can just not pass it on the list of registered providers, otherwise, I can't think of a reason why a ready-to-production profile should be disabled.

The intent isn't to have all profiles enabled through discover.experimental.enabledProfiles, only ones that we don't want exposed to users by default. Other ones such as the logs profiles will be hardcoded to be enabled, which is what I've done here now that they've been merged: 1aff791.

The reason we want this setting other than for example profiles is so that we can merge incomplete or experimental profiles to main without releasing them to users, allowing product folks to set up demo environments to test different combinations of unreleased profiles, or rolling out experimental profiles in serverless before going GA in a stateful release.

The example profiles are useful to understand the capabilities of each profile type, although I wonder if makes sens to register them on the main system where we also add the real profiles. If it's for testing/documentation purpose only, shall we move them out and register them on a test env only?

We discussed just hardcoding them to be enabled when passing --run-examples like we do with most other example functionality, but decided it against it since we don't want them to interfere with the default Discover behaviour in dev environments where folks often run Kibana with --run-examples, so we went with a YAML setting instead. I don't really see an issue with them being registered through the same mechanism as other profiles since we won't be enabling them unless specified.

The profile manager init and profiles registration start having more logic around it (singleton profiles registration step, depends on config, need services injection, dynamic loading, ...) and the plugin entry point has already knowledge of more and more of it (this.profileProvidersRegistered).
Should we consider extracting these parts into a more encapsulated service, so that a client is instantiated lazily on demand, and all these implementation details don't pertain to the main plugin? I'm not leaving examples but I have some to provide, happy to talk more about this 👌

Personally I think a dedicated service for this would be overkill, but I agree that the handling of profiles was starting to get messy. I revisited this and realized we actually don't need this.profileProvidersRegistered or class fields for the services. In the interest of moving quickly and keeping things simple, I went with a straightforward approach of just memoizing the creation of services and profile registrations here: 07a26a8.

tonyghiani · 2024-06-19T07:23:18Z

The reason we want this setting other than for example profiles is so that we can merge incomplete or experimental profiles to main without releasing them to users, allowing product folks to set up demo environments to test different combinations of unreleased profiles, or rolling out experimental profiles in serverless before going GA in a stateful release.

@davismcphee I see your point, I guess it would be useful to then have some sort of guidance on how to use this (I think you mentioned you had a follow-up PR for documentation).

I'll continue the PR review and leave the previous points to your judgment, I feel synthtrace should be the way to go for new tests as it makes it much easier to iterate the tests and update the mock data, but it's not necessarily a must-have here if you want to unblock this PR 👌

tonyghiani

LGTM 👌

logeekal

small questions/comments

logeekal · 2024-06-19T10:51:06Z

test/functional/apps/discover/context_awareness/_data_source_profile.ts

+        it('should render custom @timestamp but not custom log.level', async () => {
+          const state = kbnRison.encode({
+            dataSource: { type: 'esql' },
+            query: { esql: 'from my-example-* | sort @timestamp desc' },


I have question for my clarity. Even though my-example-* includes my-example-logs as an index, why log.level is not customized here?

Good question, it's just due to the way the resolution logic for the example profile that customizes log.level is implemented here:

kibana/src/plugins/discover/public/context_awareness/profile_providers/example_data_source_profile/profile.tsx

Lines 62 to 64 in 07a26a8

if (indexPattern !== 'my-example-logs') {

return { isMatch: false };

}

We're only matching on this profile if the index pattern exactly equals my-example-logs, although it could have been implemented to match on any index pattern covered by my-example-* if I had decided to implement it that way. Real resolution implementations like the logs one are more sophisticated, this one is pretty dumb just for example purposes.

Ahh... I went though these example profile multiple time but missed this line. Sorry for that and thank you very much for pointing it out.

Real resolution implementations like the logs one are more sophisticated, this one is pretty dumb just for example purposes.

Makes sense to me now. This demonstrates the profile resolutions well enough. Thanks.

logeekal · 2024-06-19T10:51:49Z

.../discover/public/context_awareness/profile_providers/example_data_source_profile/profile.tsx

+        const level = getFieldValue(props.row, 'log.level');
+
+        if (!level) {
+          return <span css={{ color: euiThemeVars.euiTextSubduedColor }}>(None)</span>;


I suggest to add data-test-subj here as well, since this is also a customized renderer and not the original one.

Makes sense to me, added here: de21734

davismcphee

@davismcphee I see your point, I guess it would be useful to then have some sort of guidance on how to use this (I think you mentioned you had a follow-up PR for documentation).

Definitely it would be, and I do intend to add documentation and will make sure to cover this since I understand it's currently confusing for others. Just a matter of finding some time between trying to get things ready for next week's demo 😅 It's ON Week for us next week and I'm on PTO the following week, but I will aim to address this ASAP.

I feel synthtrace should be the way to go for new tests as it makes it much easier to iterate the tests and update the mock data

Fully agree, it's a better way for sure, and I appreciate the understanding. Now that I know how to use it for FTs, we should absolutely use it at least for testing real profile implementations, and I will see if I can find some time to come back and update these example tests to use it too.

davismcphee · 2024-06-19T15:01:31Z

.../discover/public/context_awareness/profile_providers/example_data_source_profile/profile.tsx

+        const level = getFieldValue(props.row, 'log.level');
+
+        if (!level) {
+          return <span css={{ color: euiThemeVars.euiTextSubduedColor }}>(None)</span>;


Makes sense to me, added here: de21734

davismcphee · 2024-06-19T15:09:24Z

test/functional/apps/discover/context_awareness/_data_source_profile.ts

+        it('should render custom @timestamp but not custom log.level', async () => {
+          const state = kbnRison.encode({
+            dataSource: { type: 'esql' },
+            query: { esql: 'from my-example-* | sort @timestamp desc' },


Good question, it's just due to the way the resolution logic for the example profile that customizes log.level is implemented here:

kibana/src/plugins/discover/public/context_awareness/profile_providers/example_data_source_profile/profile.tsx

Lines 62 to 64 in 07a26a8

if (indexPattern !== 'my-example-logs') {

return { isMatch: false };

}

We're only matching on this profile if the index pattern exactly equals my-example-logs, although it could have been implemented to match on any index pattern covered by my-example-* if I had decided to implement it that way. Real resolution implementations like the logs one are more sophisticated, this one is pretty dumb just for example purposes.

kibana-ci · 2024-06-19T16:36:29Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: de21734

Failed CI Steps

Jest Tests #17

Test Failures

[job] [logs] Jest Tests #17 / Fleet integration policy endpoint security event filters card should show the card when no permissions but results

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	930	938	+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`	804.2KB	808.7KB	+4.5KB

Page load bundle

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

id	before	after	diff
`discover`	44.0KB	42.6KB	-1.4KB

Unknown metric groups

async chunk count

id	before	after	diff
`discover`	24	25	+1

History

💛 Build #216515 was flaky 07a26a8
💛 Build #216227 was flaky 574d67f
💚 Build #215955 succeeded 237d7be
💚 Build #215954 succeeded 1abc91b
💛 Build #215639 was flaky eaf6d04
💔 Build #215630 failed c50090b

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

cc @davismcphee

davismcphee added release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. Project:OneDiscover Enrich Discover with contextual awareness labels Jun 10, 2024

davismcphee self-assigned this Jun 10, 2024

davismcphee added 2 commits June 12, 2024 16:11

Clean up profile service types and add initial enabled profiles imple…

ad83b20

…mentation

Add registerProfileProviders and new example profiles

831f324

davismcphee force-pushed the one-discover-functional-tests branch from ec06e14 to 831f324 Compare June 12, 2024 19:14

Add Discover context awareness functional tests

15147b5

Fix registerProfileProviders async import

9403b16

Add registerProfileProviders Jest tests

c50090b

Add discover.experimental.enabledProfiles to rendering service test

eaf6d04

davismcphee marked this pull request as ready for review June 13, 2024 03:07

davismcphee requested review from a team as code owners June 13, 2024 03:08

jughosta approved these changes Jun 13, 2024

View reviewed changes

src/plugins/discover/public/context_awareness/__mocks__/index.ts Outdated Show resolved Hide resolved

sphilipse approved these changes Jun 13, 2024

View reviewed changes

Ikuni17 approved these changes Jun 14, 2024

View reviewed changes

Merge branch 'main' into one-discover-functional-tests

1abc91b

This was referenced Jun 16, 2024

[Discover][Extension] Document Viewer #185940

Merged

[Discover] Add logs source and document contexts #184601

Merged

Update registerProviders to shouldRegisterProviders

237d7be

jughosta mentioned this pull request Jun 17, 2024

[Discover] Add functional tests for DocViewer extention #186265

Closed

Merge branch 'main' into one-discover-functional-tests

574d67f

SiddharthMantri approved these changes Jun 18, 2024

View reviewed changes

davismcphee added 3 commits June 18, 2024 13:54

Merge branch 'main' into one-discover-functional-tests

bfde82e

Move logs profile registrations to registerProfileProviders

1aff791

Simplify profile service handling in plugin.tsx

07a26a8

tonyghiani approved these changes Jun 19, 2024

View reviewed changes

logeekal reviewed Jun 19, 2024

View reviewed changes

davismcphee added 2 commits June 19, 2024 11:49

Merge branch 'main' into one-discover-functional-tests

4fff149

Add missing data-test-subj

de21734

davismcphee commented Jun 19, 2024

View reviewed changes

davismcphee merged commit 57891ff into elastic:main Jun 19, 2024
22 checks passed

davismcphee deleted the one-discover-functional-tests branch June 19, 2024 17:17

kibanamachine added v8.15.0 backport:skip This commit does not require backporting labels Jun 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Add support for contextual awareness functional tests #185905

[Discover] Add support for contextual awareness functional tests #185905

davismcphee commented Jun 10, 2024 •

edited

Loading

davismcphee commented Jun 11, 2024

davismcphee commented Jun 13, 2024

davismcphee commented Jun 13, 2024

davismcphee commented Jun 13, 2024

kibanamachine commented Jun 13, 2024

kibanamachine commented Jun 13, 2024

kibanamachine commented Jun 13, 2024

kibanamachine commented Jun 13, 2024

davismcphee commented Jun 13, 2024

elasticmachine commented Jun 13, 2024

jughosta left a comment

tonyghiani commented Jun 18, 2024

davismcphee commented Jun 18, 2024

tonyghiani commented Jun 19, 2024

tonyghiani left a comment

logeekal left a comment

logeekal Jun 19, 2024

davismcphee Jun 19, 2024

logeekal Jun 19, 2024

logeekal Jun 19, 2024

davismcphee Jun 19, 2024

davismcphee left a comment

davismcphee Jun 19, 2024

davismcphee Jun 19, 2024

kibana-ci commented Jun 19, 2024

async chunk count

	if (indexPattern !== 'my-example-logs') {
	return { isMatch: false };
	}

[Discover] Add support for contextual awareness functional tests #185905

[Discover] Add support for contextual awareness functional tests #185905

Conversation

davismcphee commented Jun 10, 2024 • edited Loading

Summary

Checklist

For maintainers

davismcphee commented Jun 11, 2024

davismcphee commented Jun 13, 2024

davismcphee commented Jun 13, 2024

davismcphee commented Jun 13, 2024

kibanamachine commented Jun 13, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6304

kibanamachine commented Jun 13, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6307

kibanamachine commented Jun 13, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6305

kibanamachine commented Jun 13, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6306

davismcphee commented Jun 13, 2024

elasticmachine commented Jun 13, 2024

jughosta left a comment

Choose a reason for hiding this comment

tonyghiani commented Jun 18, 2024

davismcphee commented Jun 18, 2024

tonyghiani commented Jun 19, 2024

tonyghiani left a comment

Choose a reason for hiding this comment

logeekal left a comment

Choose a reason for hiding this comment

logeekal Jun 19, 2024

Choose a reason for hiding this comment

davismcphee Jun 19, 2024

Choose a reason for hiding this comment

logeekal Jun 19, 2024

Choose a reason for hiding this comment

logeekal Jun 19, 2024

Choose a reason for hiding this comment

davismcphee Jun 19, 2024

Choose a reason for hiding this comment

davismcphee left a comment

Choose a reason for hiding this comment

davismcphee Jun 19, 2024

Choose a reason for hiding this comment

davismcphee Jun 19, 2024

Choose a reason for hiding this comment

kibana-ci commented Jun 19, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

History

davismcphee commented Jun 10, 2024 •

edited

Loading