Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Discover] Extend EBT context with a list of activated context-aware profiles #192908

Merged
merged 26 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
af9fc84
[Discover] Extend ebt context with a list of activated context-aware …
jughosta Sep 13, 2024
e57b0c7
[Discover] Update logic
jughosta Sep 16, 2024
b842add
[Discover] Update logic
jughosta Sep 16, 2024
c5df319
[Discover] Rename the prop
jughosta Sep 16, 2024
56ef618
[Discover] Update comment
jughosta Sep 16, 2024
bfe2943
[Discover] Add tests
jughosta Sep 17, 2024
64594e0
[Discover] Fix checks
jughosta Sep 17, 2024
b42ca3a
Merge branch 'main' into 186109-dsc-profiles-ebt
jughosta Sep 17, 2024
062a2e7
Merge remote-tracking branch 'upstream/main' into 186109-dsc-profiles…
jughosta Sep 17, 2024
beb9d39
Merge branch 'main' into 186109-dsc-profiles-ebt
jughosta Sep 17, 2024
802d3a2
[Discover] Change the approach
jughosta Sep 18, 2024
12ffb6e
[Discover] Clean up
jughosta Sep 18, 2024
05fd9c5
[CI] Auto-commit changed files from 'node scripts/notice'
kibanamachine Sep 18, 2024
52b7cc4
Merge remote-tracking branch 'upstream/main' into 186109-dsc-profiles…
jughosta Sep 19, 2024
32badc7
[Discover] Clean up
jughosta Sep 19, 2024
d7c9f94
[Discover] Make it more generic
jughosta Sep 19, 2024
261b5f4
Merge branch 'main' into 186109-dsc-profiles-ebt
jughosta Sep 19, 2024
49e385f
[Discover] Rename prop
jughosta Sep 20, 2024
fa8c2c2
Update test/functional/apps/discover/context_awareness/_data_source_p…
jughosta Sep 20, 2024
adaca4b
Merge branch 'main' into 186109-dsc-profiles-ebt
jughosta Sep 20, 2024
0cec9c5
Merge branch 'main' into 186109-dsc-profiles-ebt
jughosta Sep 23, 2024
5727e2a
Merge branch 'main' into 186109-dsc-profiles-ebt
jughosta Sep 24, 2024
9a6bbff
[Discover] Rename according to PR comments
jughosta Sep 24, 2024
0d0d141
[Discover] Make ebtContextManager private
jughosta Sep 24, 2024
37b12bf
[Discover] Rename in tests
jughosta Sep 24, 2024
72a79ad
Merge branch 'main' into 186109-dsc-profiles-ebt
jughosta Sep 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/plugins/discover/public/build_services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ import type { DiscoverContextAppLocator } from './application/context/services/l
import type { DiscoverSingleDocLocator } from './application/doc/locator';
import type { DiscoverAppLocator } from '../common';
import type { ProfilesManager } from './context_awareness';
import type { DiscoverEBTContextManager } from './services/discover_ebt_context_manager';

/**
* Location state of internal Discover history instance
Expand Down Expand Up @@ -130,6 +131,7 @@ export interface DiscoverServices {
noDataPage?: NoDataPagePluginStart;
observabilityAIAssistant?: ObservabilityAIAssistantPublicStart;
profilesManager: ProfilesManager;
ebtContextManager: DiscoverEBTContextManager;
fieldsMetadata?: FieldsMetadataPublicStart;
}

Expand All @@ -145,6 +147,7 @@ export const buildServices = memoize(
scopedHistory,
urlTracker,
profilesManager,
ebtContextManager,
setHeaderActionMenu = noop,
}: {
core: CoreStart;
Expand All @@ -157,6 +160,7 @@ export const buildServices = memoize(
scopedHistory?: ScopedHistory;
urlTracker: UrlTracker;
profilesManager: ProfilesManager;
ebtContextManager: DiscoverEBTContextManager;
setHeaderActionMenu?: AppMountParameters['setHeaderActionMenu'];
}): DiscoverServices => {
const { usageCollection } = plugins;
Expand Down Expand Up @@ -217,6 +221,7 @@ export const buildServices = memoize(
noDataPage: plugins.noDataPage,
observabilityAIAssistant: plugins.observabilityAIAssistant,
profilesManager,
ebtContextManager,
fieldsMetadata: plugins.fieldsMetadata,
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
SolutionType,
} from '../profiles';
import { ProfilesManager } from '../profiles_manager';
import { DiscoverEBTContextManager } from '../../services/discover_ebt_context_manager';
import { createLogsContextServiceMock } from '@kbn/discover-utils/src/__mocks__';

export const createContextAwarenessMocks = ({
Expand Down Expand Up @@ -150,10 +151,12 @@ export const createContextAwarenessMocks = ({
documentProfileServiceMock.registerProvider(documentProfileProviderMock);
}

const ebtContextManagerMock = new DiscoverEBTContextManager();
const profilesManagerMock = new ProfilesManager(
rootProfileServiceMock,
dataSourceProfileServiceMock,
documentProfileServiceMock
documentProfileServiceMock,
ebtContextManagerMock
);

const profileProviderServices = createProfileProviderServicesMock();
Expand All @@ -169,6 +172,7 @@ export const createContextAwarenessMocks = ({
contextRecordMock2,
profilesManagerMock,
profileProviderServices,
ebtContextManagerMock,
};
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ describe('ProfilesManager', () => {
beforeEach(() => {
jest.clearAllMocks();
mocks = createContextAwarenessMocks();
jest.spyOn(mocks.ebtContextManagerMock, 'updateProfilesContextWith');
});

it('should return default profiles', () => {
Expand Down Expand Up @@ -60,6 +61,11 @@ describe('ProfilesManager', () => {
mocks.dataSourceProfileProviderMock.profile,
mocks.documentProfileProviderMock.profile,
]);

expect(mocks.ebtContextManagerMock.updateProfilesContextWith).toHaveBeenCalledWith([
'root-profile',
'data-source-profile',
]);
});

it('should expose profiles as an observable', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import type {
DocumentContext,
} from './profiles';
import type { ContextWithProfileId } from './profile_service';
import { DiscoverEBTContextManager } from '../services/discover_ebt_context_manager';

interface SerializedRootProfileParams {
solutionNavId: RootProfileProviderParams['solutionNavId'];
Expand Down Expand Up @@ -52,6 +53,7 @@ export interface GetProfilesOptions {
export class ProfilesManager {
private readonly rootContext$: BehaviorSubject<ContextWithProfileId<RootContext>>;
private readonly dataSourceContext$: BehaviorSubject<ContextWithProfileId<DataSourceContext>>;
public readonly ebtContextManager: DiscoverEBTContextManager;

private prevRootProfileParams?: SerializedRootProfileParams;
private prevDataSourceProfileParams?: SerializedDataSourceProfileParams;
Expand All @@ -61,10 +63,12 @@ export class ProfilesManager {
constructor(
private readonly rootProfileService: RootProfileService,
private readonly dataSourceProfileService: DataSourceProfileService,
private readonly documentProfileService: DocumentProfileService
private readonly documentProfileService: DocumentProfileService,
ebtContextManager: DiscoverEBTContextManager
) {
this.rootContext$ = new BehaviorSubject(rootProfileService.defaultContext);
this.dataSourceContext$ = new BehaviorSubject(dataSourceProfileService.defaultContext);
this.ebtContextManager = ebtContextManager;
}

/**
Expand Down Expand Up @@ -130,6 +134,7 @@ export class ProfilesManager {
return;
}

this.trackActiveProfiles(this.rootContext$.getValue().profileId, context.profileId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also call this during resolveRootProfile in the case that an event could fire between when the root profile is resolved and the data source profile is resolved? Probably not particularly likely, but maybe possible in cases where searchOnPageLoad is off?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davismcphee I don't think that it would be helpful to report a default default-data-source-profile in this case. Better to track the active profiles when users actually access their data and we know for sure what data source was it. Then we would have a cleaner reporting regarding the usage of one profile versus another.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough 👍 It's easy enough change later on anyway if we ever identify a need for this

this.dataSourceContext$.next(context);
this.prevDataSourceProfileParams = serializedParams;
}
Expand Down Expand Up @@ -194,6 +199,15 @@ export class ProfilesManager {
map(() => this.getProfiles(options))
);
}

/**
* Tracks the active profiles in the EBT context
*/
private trackActiveProfiles(rootContextProfileId: string, dataSourceContextProfileId: string) {
const dscProfiles = [rootContextProfileId, dataSourceContextProfileId];

this.ebtContextManager.updateProfilesContextWith(dscProfiles);
}
}

const serializeRootProfileParams = (
Expand Down
33 changes: 28 additions & 5 deletions src/plugins/discover/public/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import { RootProfileService } from './context_awareness/profiles/root_profile';
import { DataSourceProfileService } from './context_awareness/profiles/data_source_profile';
import { DocumentProfileService } from './context_awareness/profiles/document_profile';
import { ProfilesManager } from './context_awareness/profiles_manager';
import { DiscoverEBTContextManager } from './services/discover_ebt_context_manager';

/**
* Contains Discover, one of the oldest parts of Kibana
Expand Down Expand Up @@ -149,6 +150,9 @@ export class DiscoverPlugin
this.urlTracker = { setTrackedUrl, restorePreviousUrl, setTrackingEnabled };
this.stopUrlTracking = stopUrlTracker;

const ebtContextManager = new DiscoverEBTContextManager();
ebtContextManager.register({ core });

core.application.register({
id: PLUGIN_ID,
title: 'Discover',
Expand All @@ -173,6 +177,8 @@ export class DiscoverPlugin
window.dispatchEvent(new HashChangeEvent('hashchange'));
});

ebtContextManager.enable();

const services = buildServices({
core: coreStart,
plugins: discoverStartPlugins,
Expand All @@ -183,7 +189,11 @@ export class DiscoverPlugin
history: this.historyService.getHistory(),
scopedHistory: this.scopedHistory,
urlTracker: this.urlTracker!,
profilesManager: await this.createProfilesManager({ plugins: discoverStartPlugins }),
profilesManager: await this.createProfilesManager({
plugins: discoverStartPlugins,
ebtContextManager,
}),
ebtContextManager,
setHeaderActionMenu: params.setHeaderActionMenu,
});

Expand Down Expand Up @@ -216,6 +226,7 @@ export class DiscoverPlugin
});

return () => {
ebtContextManager.disableAndReset();
unlistenParentHistory();
unmount();
appUnMounted();
Expand Down Expand Up @@ -320,22 +331,30 @@ export class DiscoverPlugin
return { rootProfileService, dataSourceProfileService, documentProfileService };
});

private async createProfilesManager({ plugins }: { plugins: DiscoverStartPlugins }) {
private async createProfilesManager({
plugins,
ebtContextManager,
}: {
plugins: DiscoverStartPlugins;
ebtContextManager: DiscoverEBTContextManager;
}) {
const { rootProfileService, dataSourceProfileService, documentProfileService } =
await this.createProfileServices({ plugins });

return new ProfilesManager(
rootProfileService,
dataSourceProfileService,
documentProfileService
documentProfileService,
ebtContextManager
);
}

private createEmptyProfilesManager() {
return new ProfilesManager(
new RootProfileService(),
new DataSourceProfileService(),
new DocumentProfileService()
new DocumentProfileService(),
new DiscoverEBTContextManager() // it's not enabled outside of Discover
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sort of overlooked that this approach wouldn't work for embeddables in our initial discussions about it. I think it works well for events in Discover and we should keep it, but I also feel like we may need an additional approach later not based around global EBT context to help track events in dashboard panels. This still seems like a great place to start though since Discover usage is most important right now.

);
}

Expand All @@ -354,6 +373,7 @@ export class DiscoverPlugin
history: this.historyService.getHistory(),
urlTracker: this.urlTracker!,
profilesManager,
ebtContextManager: profilesManager.ebtContextManager,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than making profilesManager.ebtContextManager public, could we instead make it private and just pass ebtContextManager to getDiscoverServices? It feels like the EBT manager should be just an implementation detail of the profiles manager.

});
};

Expand All @@ -368,7 +388,10 @@ export class DiscoverPlugin

const getDiscoverServicesInternal = async () => {
const [coreStart, deps] = await core.getStartServices();
const profilesManager = await this.createProfilesManager({ plugins: deps });
const profilesManager = await this.createProfilesManager({
plugins: deps,
ebtContextManager: new DiscoverEBTContextManager(),
});
return this.getDiscoverServices(coreStart, deps, profilesManager);
};

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import { BehaviorSubject } from 'rxjs';
import { coreMock } from '@kbn/core/public/mocks';
import { DiscoverEBTContextManager } from './discover_ebt_context_manager';

const coreSetupMock = coreMock.createSetup();

describe('DiscoverEBTContextManager', () => {
let discoverEBTContextManager: DiscoverEBTContextManager;

beforeEach(() => {
discoverEBTContextManager = new DiscoverEBTContextManager();
});

describe('register', () => {
it('should register the context provider', () => {
discoverEBTContextManager.register({ core: coreSetupMock });

expect(coreSetupMock.analytics.registerContextProvider).toHaveBeenCalledWith({
name: 'discover_context',
context$: expect.any(BehaviorSubject),
schema: {
dscProfiles: {
type: 'array',
items: {
type: 'keyword',
_meta: {
description:
'List of profiles which are activated by Discover Context Awareness logic',
},
},
},
},
});
});
});

describe('updateProfilesWith', () => {
it('should update the profiles with the provided props', () => {
const dscProfiles = ['profile1', 'profile2'];
const dscProfiles2 = ['profile21', 'profile22'];
discoverEBTContextManager.register({ core: coreSetupMock });
discoverEBTContextManager.enable();

discoverEBTContextManager.updateProfilesContextWith(dscProfiles);
expect(discoverEBTContextManager.getProfilesContext()).toBe(dscProfiles);

discoverEBTContextManager.updateProfilesContextWith(dscProfiles2);
expect(discoverEBTContextManager.getProfilesContext()).toBe(dscProfiles2);
});

it('should not update the profiles if profile list did not change', () => {
const dscProfiles = ['profile1', 'profile2'];
const dscProfiles2 = ['profile1', 'profile2'];
discoverEBTContextManager.register({ core: coreSetupMock });
discoverEBTContextManager.enable();

discoverEBTContextManager.updateProfilesContextWith(dscProfiles);
expect(discoverEBTContextManager.getProfilesContext()).toBe(dscProfiles);

discoverEBTContextManager.updateProfilesContextWith(dscProfiles2);
expect(discoverEBTContextManager.getProfilesContext()).toBe(dscProfiles);
});

it('should not update the profiles if not enabled yet', () => {
const dscProfiles = ['profile1', 'profile2'];
discoverEBTContextManager.register({ core: coreSetupMock });

discoverEBTContextManager.updateProfilesContextWith(dscProfiles);
expect(discoverEBTContextManager.getProfilesContext()).toEqual([]);
});

it('should not update the profiles after resetting unless enabled again', () => {
const dscProfiles = ['profile1', 'profile2'];
discoverEBTContextManager.register({ core: coreSetupMock });
discoverEBTContextManager.enable();
discoverEBTContextManager.updateProfilesContextWith(dscProfiles);
expect(discoverEBTContextManager.getProfilesContext()).toBe(dscProfiles);
discoverEBTContextManager.disableAndReset();
expect(discoverEBTContextManager.getProfilesContext()).toEqual([]);
discoverEBTContextManager.updateProfilesContextWith(dscProfiles);
expect(discoverEBTContextManager.getProfilesContext()).toEqual([]);
discoverEBTContextManager.enable();
discoverEBTContextManager.updateProfilesContextWith(dscProfiles);
expect(discoverEBTContextManager.getProfilesContext()).toBe(dscProfiles);
});
});
});
Loading