[Discover] One Discover context awareness #183797

davismcphee · 2024-05-19T04:07:59Z

Summary

This PR includes the initial implementation of the Discover contextual awareness framework and composable profiles:

Context

We currently support three levels of context in Discover:

Root context:
- Based on the current solution type, navigational parameters, etc.
- Resolved at application initialization and on parameter changes.
- Runs synchronously or asynchronously.
Data source context:
- Based on the current ES|QL query or data view.
- Resolved on ES|QL query or data view change, before data fetching occurs.
- Runs synchronously or asynchronously.
Document context:
- Based on individual ES|QL records or ES documents.
- Resolved individually for each ES|QL record or ES document after data fetching runs.
- Runs synchronously only.

Composable profiles

To support application extensibility based on context, we've introduced the concept of "composable profiles". Composable profiles are implementations of a core Profile interface (or a subset of it) containing all of the available extension points Discover supports. A composable profile can be implemented at any context level through a "profile provider", responsible for defining the composable profile and its associated context resolution method. The context resolution method, named resolve, determines if its composable profile is a match for the current Discover context, and returns related metadata in a context object.

Merged accessors

Composable profiles operate similarly to middleware in that each of their extension point implementations are passed a prev argument, which can be called to access the results from profiles at previous context levels, and allows overwriting or composing a final result from the previous results. The method Discover calls to trigger the extension point merging process and obtain a final result from the combined profiles is referred to as a "merged accessor".

The following diagram illustrates the extension point merging process:

Supporting services

The contextual awareness framework is driven by two main supporting services called ProfileService and ProfilesManager.

Each context level has a dedicated profile service, e.g. RootProfileService, which is responsible for accepting profile provider registrations and running through each provider in order during context resolution to identify a matching profile.

A single ProfilesManager is instantiated on Discover load, or one per saved search panel in a dashboard. The profiles manager is responsible for the following:

Managing state associated with the current Discover context.
Coordinating profile services and exposing resolution methods for each context level.
Providing access to the combined set of resolved profiles.
Deduplicating profile resolution attempts with identical parameters.
Error handling and fallback behaviour on profile resolution failure.

Bringing it all together

The following diagram models the overall Discover contextual awareness framework and how each of the above concepts come together:

Followup work

We'll want to add developer documentation as a followup, which I've created an issue for here: [Discover] Add contextual awareness framework developer documentation #184698. The summary for this PR can be used as the basis for the documentation.
Since we currently have no profile or extension point implementations, this PR does not include any functional tests. We should create example implementations for functional testing and ensure they're only enabled when running the test suite or when developers want them enabled. I've created a followup issue for this work here: [Discover] Add functional tests for Discover contextual awareness framework #184699.

Testing notes

Testing the framework is tricky since we have no actual profile or extension point implementations yet. However, I previously added example implementations that I was using for testing while building the framework. I've removed the example implementations so they don't get merged, but they can be temporarily restored for testing by reverting the commit where I removed them: git revert 5752651f474d99dfbdecfe9d869377b9edaf7c62.

You'll also need to uncomment the following lines in src/plugins/discover/public/plugin.tsx:

kibana/src/plugins/discover/public/plugin.tsx

Lines 458 to 463 in ce85a6a

    
           private registerProfiles() { 
        
             // TODO: Conditionally register example profiles for functional testing in a follow up PR 
        
             // this.rootProfileService.registerProvider(o11yRootProfileProvider); 
        
             // this.dataSourceProfileService.registerProvider(logsDataSourceProfileProvider); 
        
             // this.documentProfileService.registerProvider(logDocumentProfileProvider); 
        
           }

To test the root profile resolution based on solution type, I'd recommend enabling the solution nav locally by adding the following to kibana.dev.yml:

xpack.cloud_integrations.experiments.enabled: true
xpack.cloud_integrations.experiments.flag_overrides:
  "solutionNavEnabled": true

xpack.cloud.id: "ftr_fake_cloud_id:aGVsbG8uY29tOjQ0MyRFUzEyM2FiYyRrYm4xMjNhYmM="
xpack.cloud.base_url: "https://cloud.elastic.co"
xpack.cloud.deployment_url: "/deployments/deploymentId"

In order to change the active solution type, modify the mockSpaceState in src/plugins/navigation/public/plugin.tsx:

kibana/src/plugins/navigation/public/plugin.tsx

Lines 159 to 162 in 79e51d6

    
           // Here we will read the space state and decide if we are in classic or project style 
        
           const mockSpaceState: { solutionView?: 'classic' | 'es' | 'oblt' | 'security' } = { 
        
             solutionView: 'security', // Change this value to test different solution views 
        
           };

For test data, I'd recommend running the following commands to generate sample ECS compliant logs and metrics data:

node scripts/synthtrace.js --target "http://elastic:changeme@localhost:9200/" --kibana "http://elastic:changeme@localhost:5601/" --live --logLevel debug simple_logs.ts
node scripts/synthtrace.js --target "http://elastic:changeme@localhost:9200/" --kibana "http://elastic:changeme@localhost:5601/" --live --logLevel debug simple_trace.ts

And lastly a couple of the ES|QL queries I used for testing:

// resolves to the example logs data source context
from logs-synth-default

// mixed dataset that falls back to vanilla Discover
// helpful for testing document context in the doc viewer flyout
from logs-synth-default,metrics-*

Resolves #181962.

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-05-22T02:37:16Z

/ci

davismcphee · 2024-05-22T03:14:57Z

/ci

src/plugins/discover/public/application/main/components/layout/discover_documents.tsx

src/plugins/discover/public/context_awareness/profiles_manager.ts

src/plugins/discover/public/context_awareness/profile_service.ts

jughosta · 2024-05-23T12:11:39Z

src/plugins/discover/public/application/main/data_fetching/fetch_documents.ts

+        return buildDataTableRecordList(res.rawResponse.hits.hits as EsHitRecord[], dataView, {
+          processRecord: (record) => {
+            profilesCollector.collect({ record });
+            return record;
+          },
+        });


I was experimenting with optimizing records flattening by using a "lazy" getter function and I think it can become in handy for document context resolution too. We could add a "lazy" getter for this logic to the new DataTableRecord class too. This way we would process records "on demand" and not all at once which should make things faster.

https://github.com/jughosta/kibana/pull/11/files#diff-eebdb51e43f4e4e7083c39bddb64144d3e5da051d56d2fa68efb2d0a87479dc4R75

Good idea! Document context resolution is synchronous anyway, so a lazy getter should do the trick. I wouldn't expect the resolution process to be heavy in general, but it seems reasonable to avoid running it until necessary. I was able to implement it already, but it could certainly be improved if we switch to a class later: d8351ca#diff-8501e1b7df4eeb0b27b10309dd2ffbe366964c264334485dee5a1d404e51a583R104-R108.

davismcphee · 2024-05-28T04:05:17Z

/ci

tonyghiani · 2024-05-28T08:35:02Z

src/plugins/discover/public/context_awareness/hooks/use_root_profile.ts

+
+    setRootProfileLoading(true);
+
+    profilesManager.resolveRootProfile({ solutionNavId }).then(() => {


question: as we have async resolution of the profiles, should we handle errors here in case a profile resolution triggers one? Also the resolveRootProfile method and the other resolution functions don't encapsulate the error, would be nice to also wrap the error there to something like ProfileResolutionError than can be easily recognized and handled on this custom hook or where might be necessary.

Yes we should definitely handle errors in some way to avoid breaking the UI if profile resolution fails, although I lean toward handling this internally within ProfilesManager, logging it, and resolving to the default profile on error to avoid having to handle it elsewhere in code. We could also surface resolution failures in the inspector when adding support for debugging context awareness. WDYT, or do you feel there's value in throwing an error and letting consuming code handle it instead?

I'm good with having the error handling centralized in ProfilesManager as far as there are no surprising behaviours for the user when the profile resolution fails 😄

Sounds good! Added initial error handling and fallback logic here: faa68c2.

tonyghiani · 2024-05-28T09:04:37Z

src/plugins/discover/public/context_awareness/profile_service.ts

+  public registerProvider(provider: ProfileProvider<TProfile, TParams, TContext, TMode>) {
+    this.providers.push(provider);
+    this.providers.sort((a, b) => a.order - b.order);
+  }
+
+  public getProfile(context: ContextWithProfileId<TContext>): ComposableProfile<Profile> {
+    const provider = this.providers.find((current) => current.profileId === context.profileId);
+    return provider?.profile ?? EMPTY_PROFILE;
+  }


doubt: I might be missing something here, but could you explain a use case of where/why we need an order property for the registered profiles? The only case I can think of is in case we have the exact same resolved value in a specific part of the page (e.g. top nav items), but in this case shouldn't we enforce a constrained set of profileId values to register a profile?

Taken from the example profile, where are we potentially going to register a profile with id logs-data-source-profile more than once?

The order prop determines the order for providers to run in during context resolution since the first match is the one that gets used. So for example if we have a logsDataSourceProfileProvider and a metricsDataSourceProfileProvider, the order will determine which one tries to resolve first.

But this was done originally under the assumption we would expose the registration function externally, which may not be necessary if we just do all registrations internally in Discover. In that case, I think it would make sense to just run the providers in whichever order they were registered in code. WDYT?

The order prop determines the order for providers to run in during context resolution since the first match is the one that gets used. So for example if we have a logsDataSourceProfileProvider and a metricsDataSourceProfileProvider, the order will determine which one tries to resolve first.

This is the part where I have doubts, for the order value to have a role in the resolution step, we are assuming that there might be multiple registration of profiles with id logsDataSourceProfileProvider or anything else, what is the use case where we need to register multiple profiles for logs data source? Or is it just an example id and I am misleading the change that we can have profiles with same ids for other features?

Bad explanation on my part, let me try again 😄

The order property was used to determine the resolution order for different providers of the same type (e.g. RootProfileProvider, DataSourceProfileProvider, DocumentProfileProvider), unrelated to profileId, which as you would imagine should be unique. For example:

// Set order in providers const logsDataSourceProfileProvider: DataSourceProfileProvider = { order: 0, profileId: 'logs-data-source-profile', ... }; const metricsDataSourceProfileProvider: DataSourceProfileProvider = { order: 100, profileId: 'metrics-data-source-profile', ... }; // `resolve` uses `order` to decide provider resolution order when looking for a matching profile const context = await dataSourceProfileProvider.resolve({ ... });

The profileId is used to map a resolved context to its corresponding profile so we can look it up later, e.g. to get the DocumentProfile associated with a data table record:

// `getProfile` uses the `profileId` property of `record.context` to look up its profile const documentProfile = documentProfileService.getProfile(record.context);

That said, the conversations I've had about it made me realize that order is confusing and unnecessary assuming we intend to register providers from within the Discover codebase, in which case we can just rely on registration order, so I removed it here: e953b41.

davismcphee · 2024-05-30T02:40:02Z

/ci

davismcphee · 2024-05-31T03:43:42Z

/ci

davismcphee · 2024-06-03T04:25:40Z

/ci

jughosta

Great foundation! LGTM 👍

weltenwort · 2024-06-05T10:23:48Z

src/plugins/discover/public/context_awareness/profiles_manager.ts

+
+  public resolveDocumentProfile(params: DocumentProfileProviderParams) {
+    Object.defineProperty(params.record, 'context', {
+      get: memoize(() => {


Can you help me understand why we're using a (memoized) property here instead of just storing a reference to the resolved document context?

This was an optimization suggested in #183797 (comment) to limit the impact of context resolution on rendering until the document context is actually needed. In general I don't expect it to have much impact, but we could be handling up to 10,000 records here, and if any of the resolution methods do inner looping, the time complexity could grow quite quickly and become noticeable.

But when switching over to Proxy, I ended up removing memoize and just storing context directly instead: 113f66f.

weltenwort · 2024-06-05T10:25:16Z

src/plugins/discover/public/application/main/data_fetching/fetch_documents.ts

-        return buildDataTableRecordList(res.rawResponse.hits.hits as EsHitRecord[], dataView);
+        return buildDataTableRecordList(res.rawResponse.hits.hits as EsHitRecord[], dataView, {
+          processRecord: (record) => {
+            services.profilesManager.resolveDocumentProfile({ record });


Modifying the value in-place feels like a potential maintenance hazard. What's the argument for doing this instead of returning a wrapper or proxy of the record?

In general I agree, but it felt less problematic here since we were modifying the record at creation time before anything else had access to it. I was mainly thinking about performance and allocations since this is running in a loop, but maybe that was premature and a Proxy likely isn't too much overhead. I switched over to a Proxy here, which also simplifies the ProfilesManager a bit since there's no need to keep track of the latest record ID anymore: 113f66f.

I'm still not 100% on how I feel about this though. @jughosta it would be great to get your opinion on this too.

Let's go with Proxy solution 👍

weltenwort · 2024-06-05T10:41:10Z

src/plugins/discover/public/application/main/data_fetching/fetch_esql.ts

                id: String(idx),
-                raw: row,
+                raw: row as EsHitRecord,


Could this be a dangerous type assertion? If, for example, the document context resolution relies on accessing one of the meta fields guaranteed by the EsHitRecord (such as _index), it would fail without the compiler recognizing the problem. One instance of this happening is the planned resolution heuristic in #184601.

Does this imply that we need to differentiate between hits and esql records in the context awareness mechanism?

This is a dangerous type assertion since ES|QL records only contain _id and _index if explicitly requested through the metadata directive, but it's how ES|QL is implemented in Discover currently. It's mainly an issue of using EsHitRecord in DataTableRecord, which is used for both data views and ES|QL. We also do a similar cast for data view responses too, but it's safer there since we know _id and _index will be defined.

I think the best we can do for now without heavy refactoring is to ensure runtime safety by setting undefined _id and _index values to empty strings for ES|QL results, which I added here: 6607524.

According to the failing functional tests, this change affects the functionality. Maybe we should rather update the type of raw in DataTableRecord.

Yup, that didn't work as intended 😅 I've reverted that commit now. I agree that the right way is to update the type, but I attempted that in this PR and it caused a cascade of type errors. Since this is an existing issue, I'd like to leave this as it is for this PR, but I will open a followup one that addresses the actual type issue. I just don't want to block other PRs that depend on this one.

I opened a PR based on this one for the type changes here: #184975. Let's see if it passes 🤞

weltenwort · 2024-06-05T11:39:42Z

...s/observability_solution/logs_shared/public/components/log_ai_assistant/log_ai_assistant.tsx

@@ -51,7 +51,8 @@ export const LogAIAssistant = ({
      return undefined;
    }

-    const message = doc.fields.find((field) => field.field === 'message')?.value[0];
+    const messageValue = doc.fields.find((field) => field.field === 'message')?.value;
+    const message = Array.isArray(messageValue) ? messageValue[0] : messageValue;


Does this mean messageValue can be something else instead of a JsonArray? If so, can we somehow fix the type?

I made this change because the logs overview tab was breaking for ES|QL records which may not have array values. But looking into it a bit deeper, the issue was actually in createLogsAIAssistantRenderer, which was doing an unsafe cast to LogAIAssistantDocument['fields']. I reverted this change and updated createLogsAIAssistantRenderer to properly handle DataTableRecord instead: 0bfbe8a.

tonyghiani · 2024-06-05T12:01:07Z

packages/kbn-discover-utils/src/utils/build_data_record.ts

+export function buildDataTableRecordList<T extends DataTableRecord = DataTableRecord>(
  docs: EsHitRecord[],
-  dataView?: DataView
+  dataView?: DataView,
+  { processRecord }: { processRecord?: (record: DataTableRecord) => T } = {}


nit: as dataView previously was the last optional argument, this function signature made sense. But if we add a parameter and invoke it without having a data view, we then need to force passing a second argument to pass then a third one, and it'll start looking something like

buildDataTableRecordList(docs, undefined, {processRecord})

Shall we refactor the function to get named parameters as we are adding the processRecord now?

buildDataTableRecordList({docs, dataView, processRecord})

I fully agree with this, but buildDataTableRecordList is used outside of Discover and I'd like to avoid pinging other teams for review on this PR. I can make this change in a followup though to avoid blocking this one.

I opened a PR based on this one for the signature change here: #184976.

tonyghiani · 2024-06-05T12:11:04Z

src/plugins/discover/public/__mocks__/services.ts

@@ -236,6 +242,11 @@ export function createDiscoverServicesMock(): DiscoverServices {
    contextLocator: { getRedirectUrl: jest.fn(() => '') },
    singleDocLocator: { getRedirectUrl: jest.fn(() => '') },
    urlTracker: urlTrackerMock,
+    profilesManager: new ProfilesManager(


doubt: should this be the same as the mock in https://github.com/elastic/kibana/pull/183797/files#diff-66364df80226d194a5368e2cc7208a0f29538806fb2865d46a82e8f97e1bf3ddR86 ?

It should be! Updated here: f5fd842.

tonyghiani · 2024-06-05T12:18:16Z

src/plugins/discover/public/application/main/data_fetching/fetch_esql.test.ts

+      await fetchEsql(
+        { esql: 'from *' },
+        dataViewWithTimefieldMock,
+        discoverServiceMock.data,
+        discoverServiceMock.expressions,
+        { requests: new RequestAdapter() },
+        discoverServiceMock.profilesManager
+      )


Warning
This function is quickly growing and becomes more difficult to maintain and use, it might be a good time for a refactor.

Yeah I guess it's about time we clean up this function's signature. Updated here: 3f1ba48.

src/plugins/discover/public/application/main/state_management/discover_data_state_container.ts

src/plugins/discover/public/context_awareness/profiles/data_source_profile.ts

weltenwort · 2024-06-05T12:55:22Z

src/plugins/discover/public/embeddable/view_saved_search_action.test.ts

@@ -34,6 +35,8 @@ const embeddableConfig = {
  executeTriggerActions,
 };

+services.core.chrome.getActiveSolutionNavId$ = () => new BehaviorSubject('test');


This seems to be a jest mock, so how about using .mockReturnValue() to modify its behavior?

It's not typed as one, so instead I just used jest.spyOn: 2b8f6bf.

weltenwort · 2024-06-05T13:04:26Z

src/plugins/discover/public/embeddable/saved_search_embeddable.test.ts

+    const { profilesManagerMock } = createContextAwarenessMocks();
+
+    servicesMock.profilesManager = profilesManagerMock;
+    servicesMock.core.chrome.getActiveSolutionNavId$ = () => new BehaviorSubject('test');


This seems to be a jest mock, so how about using .mockReturnValue() to modify its behavior?

weltenwort · 2024-06-05T18:22:07Z

src/plugins/discover/public/application/main/components/layout/discover_documents.tsx

+  const getCellRenderersAccessor = useProfileAccessor('getCellRenderers');
+  const cellRenderers = useMemo(() => {
+    const getCellRenderers = getCellRenderersAccessor(() => customCellRenderer ?? {});
+    return getCellRenderers();


I might be misunderstanding, but isn't it necessary to get the cell renderer for each row with the corresponding record instead of just once?

Unified Data Table cell renders are column based currently, so they don't work on individual records and can't be supported by DocumentProfile right now. I'm not sure if we need cell renderers on a per-record basis yet, but maybe there are some good use cases for it.

If we decide we need it, I'd recommend we add support for them and do the necessary Unified Data Table refactoring in a followup.

Ah, sorry, I didn't know this was an intentional omission. Could this indicate that narrowing the composable profile interface by context type might be good? Because I would also have made that mistake when trying to implement a custom cell renderer.

Yes, that would make sense and it's actually already supported, I just forgot to omit getCellRenderers from the DocumentProfile type when I reverted the example extension points. Updated it here for now so it will be enforced in providers, and we can revisit this later if we find a need for document specific cell renderers: 8bab582.

weltenwort · 2024-06-05T18:24:05Z

src/plugins/discover/public/application/main/discover_main_route.tsx

@@ -338,11 +339,14 @@ export function DiscoverMainRoute({
    stateContainer,
  ]);

+  const { solutionNavId } = customizationContext;
+  const { rootProfileLoading } = useRootProfile({ solutionNavId });


The Logs Explorer doesn't seem to pick up the root context from the solution nav. I guess we need to call the root resolution step separately there? This would be true for every consumer of the <DiscoverContainer>, right?

Yeah that's a good point, although I think I'd like to avoid supporting profiles in DiscoverContainer anyway if possible. I could change useRootProfile to listen for core.chrome.getActiveSolutionNavId$ instead, but what if we just pass an empty ProfilesManager for now?

although I think I'd like to avoid supporting profiles in DiscoverContainer anyway if possible.

Does this mean those apps embedding the DiscoverContainer (such as LogsExplorer) won't benefit from the resolution logic built-in in Discover with the new data source and document profile providers?
Isn't it the purpose of making Discover aware of the data so that the other solutions embedding it get the ad-hoc behaviours out of the box?
Let me know if I misunderstood this and what is the intended support here please.

I'd agree with @tonyghiani here. For any future addition to the Logs Explorer we'd all benefit if we could instead add it to a matching Discover context.

What would be the downside of having the Logs Explorer explicitly resolve the root context (always to an obs root context)?

Does this mean those apps embedding the DiscoverContainer (such as LogsExplorer) won't benefit from the resolution logic built-in in Discover with the new data source and document profile providers?
Isn't it the purpose of making Discover aware of the data so that the other solutions embedding it get the ad-hoc behaviours out of the box?

It does mean that, but I've been under the impression the purpose of the contextual experience in Discover was to avoid the need for embedding Discover into solution apps, and instead have a single Discover app that can be linked to from within solutions. We discussed this in the engineering sync earlier today and I understand it's contentious, but I really think this is something that needs alignment by product across platform and solutions so engineering has a clear understanding of the product intentions.

I have a meeting tomorrow with my PM and director and hopefully will have a better understanding of the situation after. I personally have no problem supporting this if it's aligned with the vision since I don't think it's technically difficult, but if we don't have clarity on it after tomorrow's meeting, I'd really prefer if we merge this PR without supporting the embedded experience to avoid blocking progress until we are aligned at the product level.

Thanks for the clarity and managing the communication with PM/Director Davis!

I have clear the vision for this and the future of One Discover, and your reasoning makes sense.
On the other hand, it's even true there are still many differences between Discover and the embedded version, which means we have to maintain the same features in different parts of the codebase if we don't rely on a single mechanism.

For instance, we did some work to bring the Logs overview into the flyout so that every log user could use it based on the resolved context. What feels counter-productive is having vanilla Discover as the only app taking decisions based on context (enabling the flyout overview tab in this case), while also have in parallel the embedded version manually enable the same features (logs explorer should keep manually allowing that flyout to tab until we have feature parity and Logs Explorer can be removed).

The above would apply to other features and implementation could quickly diverge in how things are done until we have a good level of feature parity and we keep a One Discover.

If is a minimal effort, I would strongly opt for a single source of truth to resolve specific customizations, which would be the profile resolution we have worked on so far.

Let me know your thoughts, I'm happy to talk more about this if needed (I missed the engineering sync yesterday, not sure how in depth this was already covered, I'll check the recording)

Thanks for the additional context @tonyghiani. I discussed this with my PM and director Friday, and we'd like to avoid supporting the profiles system in DiscoverContainer, at least for the time being until product teams across Platform and O11y are aligned.

The main concern is that the current Discover logs experience PRD diverges significantly from what exists in Logs Explorer, and it's not clear if these will actually converge at some point. Depending on product decisions currently being discussed, parity between the Discover logs experience and Logs Explorer may not actually be a goal.

If we support the profiles system for DiscoverContainer, it means Logs Explorer will automatically inherit the Discover logs profile enhancements, which isn't something we want currently. We also don't want to migrate current Logs Explorer features to the Discover logs profile unless it's decided to include them as part of the PRD.

I also don't want to maintain parallel systems with the customization framework and the contextual awareness framework since it creates tech debt, but I think this is the right way to move forward for now until we have more clarity.

This isn't me saying no it won't happen (especially since it's not my decision to make), just that I'd like to move forward with this PR without DiscoverContainer support, and work with our product teams to reach alignment on if it should be supported. I think this would be a good topic for our weekly One Discover sync, and we know it's technically easy to change if needed.

…ogAIAssistant

logeekal · 2024-06-06T10:31:35Z

src/plugins/discover/public/context_awareness/profiles_manager.ts

+    if (isEqual(this.prevRootProfileParams, serializedParams)) {
+      return;
+    }


I am curious about the need of storing the previous reference to params and diffing the params here?

Imo this resolve function should be dumb and should simply resolve the profile. And since this function will always be called from react ( current me if assumption is wrong ), we should make use of that reactivity to call resolveRootProfile only when params have changed. What do you think?

I definitely get where you're coming from, but there are two reasons I did it this way:

The comparison logic is pretty simple right now and could probably just use a React deps array, but it could become more complex over time. E.g. we're currently re-resolving data source context whenever the ES|QL query changes, but we may later decide it's only necessary when the actual index pattern changes, so I think having a central place for this logic outside of React can be helpful.

The resolve methods are actually mostly called from outside of React, in the state container and the saved search embeddable, so keeping it all in React would be tricky.

But the resolve methods in the actual profile services are dumb and just run through the list of providers looking for a match.

logeekal · 2024-06-06T12:58:27Z

src/plugins/discover/public/context_awareness/composable_profile.ts

+  [TKey in keyof TProfile]?: ComposableAccessor<TProfile[TKey]>;
+};
+
+export const getMergedAccessor = <TKey extends keyof Profile>(


I think it might be worth adding a couple of lines of JSDoc as it is not 100% clear when this utilities do exactly from the code.

I totally agree, but I'd prefer to do this as part of the dev documentation issue I have as a followup here: #184698.

logeekal · 2024-06-06T12:59:02Z

src/plugins/discover/public/context_awareness/hooks/use_profile_accessor.ts

+import { useProfiles } from './use_profiles';
+import type { Profile } from '../types';
+
+export const useProfileAccessor = <TKey extends keyof Profile>(


Same comment as before.

I think it might be worth adding a couple of lines of JSDoc as it is not 100% clear when this utilities do exactly from the code.

This reverts commit 6607524.

weltenwort

Given the limitations that we don't have real profile properties to test it with, this looks ok to me. Good job and thanks for the thorough description.

Maybe the follow-up documentation could point out the mismatch between how Hits and ES|QL rows work in the document context resolution - and provide some guidance on how to handle it.

src/plugins/discover/public/context_awareness/hooks/use_root_profile.ts

weltenwort · 2024-06-07T18:21:37Z

src/plugins/discover/public/context_awareness/profiles_manager.test.ts

+  });
+
+  it('should log an error and fall back to the default profile if root profile resolution fails', async () => {
+    await mocks.profilesManagerMock.resolveRootProfile({ solutionNavId: 'solutionNavId' });


Not saying this needs to change now, but I found these tests quite difficult to read. Contributing factors might have been:

The error is provoked by monkey-patching a method instead of using a profile provider that fails "naturally".

The addLog is an unfortunate side-effect on its own, but also using it to test formatted error messages doubles the smell.

A cleaner solution could be to decompose it into layers responsible for the cache, the async side effects and the reactivity, which could be tested separately. But I recognize that would be disproporionate effort it this point.

Thanks for the feedback, and I'm certainly open to followup improvements (although as you mentioned, I'd prefer to limit changes at this point for the sake of unblocking dependent work). This feedback makes sense to me overall, but I'm a bit confused on this point:

The addLog is an unfortunate side-effect on its own, but also using it to test formatted error messages doubles the smell.

Is your concern here the use of the static addLog function for logging, or that ProfilesManager handles this logging itself? The way I interpreted this is that you feel it would be cleaner to do something along the lines of returning or throwing an error on resolution failure, and logging it externally, rather than logging as a side effect of resolution failure, but I'm not sure if that's what you meant.

Yes, that's exactly what I meant. But if we want to keep it in there for convenience, maybe an improvement would be to supply the logging side-effect to the profile manager as a constructor argument? 🤔

weltenwort · 2024-06-07T18:39:36Z

GitHub counts my review as sufficient for the Obs UX Logs team, but I'd appreciate it if @tonyghiani got a chance to follow up on his comments too. 😇

kibana-ci · 2024-06-10T03:09:52Z

💚 Build Succeeded

Buildkite Build
Commit: 965b97f
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-183797-965b97f78a1a
Observability Deployment

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	911	924	+13

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
`@kbn/discover-utils`	81	84	+3

Async chunks

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

id	before	after	diff
`discover`	808.4KB	809.1KB	+708.0B
`slo`	860.4KB	860.5KB	+47.0B
total			+755.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`	35.5KB	41.6KB	+6.0KB
`logsShared`	172.5KB	172.5KB	+21.0B
total			+6.1KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/discover-utils`	109	112	+3

ESLint disabled in files

id	before	after	diff
`discover`	0	2	+2

Total ESLint disabled count

id	before	after	diff
`discover`	25	27	+2

History

💚 Build #214595 succeeded 643e98d
💚 Build #214579 succeeded 8bab582
💔 Build #214565 failed aa78635
💔 Build #214330 failed 2b8f6bf

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

cc @davismcphee

tonyghiani

@davismcphee Thanks for this piece of work and for patiently addressing all my doubts/suggestions, this is a solid foundation to handle profiles in Discover from now on! 👏

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 May 19, 2024

davismcphee self-assigned this May 19, 2024

davismcphee mentioned this pull request May 19, 2024

[PoC] One Discover context awareness davismcphee/kibana#8

Closed

10 tasks

davismcphee force-pushed the one-discover-context-awareness branch from 674a80c to a8f1b15 Compare May 22, 2024 02:00

weltenwort reviewed May 22, 2024

View reviewed changes

src/plugins/discover/public/application/main/components/layout/discover_documents.tsx Outdated Show resolved Hide resolved

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

weltenwort reviewed May 22, 2024

View reviewed changes

src/plugins/discover/public/context_awareness/profile_service.ts Show resolved Hide resolved

jughosta reviewed May 23, 2024

View reviewed changes

davismcphee force-pushed the one-discover-context-awareness branch from 484bb35 to 3a35c00 Compare May 28, 2024 04:05

tonyghiani reviewed May 28, 2024

View reviewed changes

davismcphee force-pushed the one-discover-context-awareness branch from 3a35c00 to a5f2de8 Compare May 30, 2024 02:22

weltenwort mentioned this pull request May 31, 2024

[Discover] Add logs source and document contexts #184601

Merged

davismcphee force-pushed the one-discover-context-awareness branch from 5fb7971 to 5d7cdc3 Compare June 3, 2024 04:25

davismcphee added 9 commits June 3, 2024 14:50

Initial pass at context provider services

a0785b9

Initial context awareness implementation

386a18c

Add profile service

89e2a62

Add use_profile_accessor

3cd00c5

Add custom cell renderers and grid flyout

b9519d4

Attach document profiles to data table records

ac6f2e5

Make root profile service async and add profiles provider

de7c276

Make data source profile service async and set it via profiles provider

00669f7

Move example profiles to separate file

b19bd7e

botelastic bot added the ci:project-deploy-observability Create an Observability project label Jun 4, 2024

jughosta approved these changes Jun 5, 2024

View reviewed changes

weltenwort reviewed Jun 5, 2024

View reviewed changes

tonyghiani reviewed Jun 5, 2024

View reviewed changes

src/plugins/discover/public/application/main/state_management/discover_data_state_container.ts Show resolved Hide resolved

tonyghiani reviewed Jun 5, 2024

View reviewed changes

src/plugins/discover/public/context_awareness/profiles/data_source_profile.ts Show resolved Hide resolved

weltenwort reviewed Jun 5, 2024

View reviewed changes

davismcphee added 6 commits June 5, 2024 20:41

Update resolveDocumentProfile to return a Proxy for attaching context

113f66f

Ensure _id and _index are defined in EsHitRecord

6607524

Fix mappedDoc in createLogsAIAssistantRenderer and revert change to L…

0bfbe8a

…ogAIAssistant

Use createContextAwarenessMocks in mock Discover services

f5fd842

Updated fetchEsql params

3f1ba48

Use mockReturnValue instead of overwriting getActiveSolutionNavId$

2b8f6bf

logeekal reviewed Jun 6, 2024

View reviewed changes

davismcphee added 5 commits June 6, 2024 16:33

Merge branch 'main' into one-discover-context-awareness

aa78635

Revert "Ensure _id and _index are defined in EsHitRecord"

0c1d023

This reverts commit 6607524.

Omit getCellRenderers in DocumentProfile

8bab582

export hooks from index.ts

f2e6788

Pass empty ProfilesManager to DiscoverContainer

643e98d

weltenwort approved these changes Jun 7, 2024

View reviewed changes

Merge branch 'main' into one-discover-context-awareness

965b97f

tonyghiani approved these changes Jun 10, 2024

View reviewed changes

davismcphee merged commit 59912e8 into elastic:main Jun 10, 2024
20 checks passed

davismcphee deleted the one-discover-context-awareness branch June 10, 2024 14:23

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

	private registerProfiles() {
	// TODO: Conditionally register example profiles for functional testing in a follow up PR
	// this.rootProfileService.registerProvider(o11yRootProfileProvider);
	// this.dataSourceProfileService.registerProvider(logsDataSourceProfileProvider);
	// this.documentProfileService.registerProvider(logDocumentProfileProvider);
	}

	// Here we will read the space state and decide if we are in classic or project style
	const mockSpaceState: { solutionView?: 'classic' \| 'es' \| 'oblt' \| 'security' } = {
	solutionView: 'security', // Change this value to test different solution views
	};


		setRootProfileLoading(true);

		profilesManager.resolveRootProfile({ solutionNavId }).then(() => {

[Discover] One Discover context awareness #183797

[Discover] One Discover context awareness #183797

Conversation

davismcphee commented May 19, 2024 • edited Loading

Summary

Context

Composable profiles

Merged accessors

Supporting services

Bringing it all together

Followup work

Testing notes

Checklist

For maintainers

davismcphee commented May 22, 2024

davismcphee commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davismcphee commented May 28, 2024

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

Choose a reason for hiding this comment

davismcphee commented May 30, 2024

davismcphee commented May 31, 2024

davismcphee commented Jun 3, 2024

jughosta 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

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

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

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

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

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

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

weltenwort commented Jun 7, 2024 • edited Loading

kibana-ci commented Jun 10, 2024 • edited Loading

💚 Build Succeeded

Metrics [docs]

Module Count

davismcphee commented May 19, 2024 •

edited

Loading

weltenwort commented Jun 7, 2024 •

edited

Loading

kibana-ci commented Jun 10, 2024 •

edited

Loading