-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Changes from all commits
af9fc84
e57b0c7
b842add
c5df319
56ef618
bfe2943
64594e0
b42ca3a
062a2e7
beb9d39
802d3a2
12ffb6e
05fd9c5
52b7cc4
32badc7
d7c9f94
261b5f4
49e385f
fa8c2c2
adaca4b
0cec9c5
5727e2a
9a6bbff
0d0d141
37b12bf
72a79ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -149,6 +150,9 @@ export class DiscoverPlugin | |
this.urlTracker = { setTrackedUrl, restorePreviousUrl, setTrackingEnabled }; | ||
this.stopUrlTracking = stopUrlTracker; | ||
|
||
const ebtContextManager = new DiscoverEBTContextManager(); | ||
ebtContextManager.initialize({ core }); | ||
|
||
core.application.register({ | ||
id: PLUGIN_ID, | ||
title: 'Discover', | ||
|
@@ -173,6 +177,8 @@ export class DiscoverPlugin | |
window.dispatchEvent(new HashChangeEvent('hashchange')); | ||
}); | ||
|
||
ebtContextManager.enable(); | ||
|
||
const services = buildServices({ | ||
core: coreStart, | ||
plugins: discoverStartPlugins, | ||
|
@@ -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, | ||
}); | ||
|
||
|
@@ -216,6 +226,7 @@ export class DiscoverPlugin | |
}); | ||
|
||
return () => { | ||
ebtContextManager.disableAndReset(); | ||
unlistenParentHistory(); | ||
unmount(); | ||
appUnMounted(); | ||
|
@@ -285,7 +296,12 @@ export class DiscoverPlugin | |
} | ||
|
||
const getDiscoverServicesInternal = () => { | ||
return this.getDiscoverServices(core, plugins, this.createEmptyProfilesManager()); | ||
return this.getDiscoverServices( | ||
core, | ||
plugins, | ||
this.createEmptyProfilesManager(), | ||
new DiscoverEBTContextManager() // it's not enabled outside of Discover | ||
); | ||
}; | ||
|
||
return { | ||
|
@@ -320,29 +336,38 @@ 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
); | ||
} | ||
|
||
private getDiscoverServices = ( | ||
core: CoreStart, | ||
plugins: DiscoverStartPlugins, | ||
profilesManager: ProfilesManager | ||
profilesManager: ProfilesManager, | ||
ebtContextManager: DiscoverEBTContextManager | ||
) => { | ||
return buildServices({ | ||
core, | ||
|
@@ -354,6 +379,7 @@ export class DiscoverPlugin | |
history: this.historyService.getHistory(), | ||
urlTracker: this.urlTracker!, | ||
profilesManager, | ||
ebtContextManager, | ||
}); | ||
}; | ||
|
||
|
@@ -368,8 +394,12 @@ export class DiscoverPlugin | |
|
||
const getDiscoverServicesInternal = async () => { | ||
const [coreStart, deps] = await core.getStartServices(); | ||
const profilesManager = await this.createProfilesManager({ plugins: deps }); | ||
return this.getDiscoverServices(coreStart, deps, profilesManager); | ||
const ebtContextManager = new DiscoverEBTContextManager(); // it's not enabled outside of Discover | ||
const profilesManager = await this.createProfilesManager({ | ||
plugins: deps, | ||
ebtContextManager, | ||
}); | ||
return this.getDiscoverServices(coreStart, deps, profilesManager, ebtContextManager); | ||
}; | ||
|
||
plugins.embeddable.registerReactEmbeddableSavedObject<SavedSearchAttributes>({ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
/* | ||
* 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.initialize({ core: coreSetupMock }); | ||
|
||
expect(coreSetupMock.analytics.registerContextProvider).toHaveBeenCalledWith({ | ||
name: 'discover_context', | ||
context$: expect.any(BehaviorSubject), | ||
schema: { | ||
discoverProfiles: { | ||
type: 'array', | ||
items: { | ||
type: 'keyword', | ||
_meta: { | ||
description: 'List of active Discover context awareness profiles', | ||
}, | ||
}, | ||
}, | ||
}, | ||
}); | ||
}); | ||
}); | ||
|
||
describe('updateProfilesWith', () => { | ||
it('should update the profiles with the provided props', () => { | ||
const dscProfiles = ['profile1', 'profile2']; | ||
const dscProfiles2 = ['profile21', 'profile22']; | ||
discoverEBTContextManager.initialize({ 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.initialize({ 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.initialize({ 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.initialize({ 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); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
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 wheresearchOnPageLoad
is off?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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