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

jughosta · 2024-09-13T16:45:08Z

Closes [OneDiscover] Add contextual awareness resolution results to EBT telemetry events #186109

Summary

This PR extends EBT context with discoverProfiles - a list of active context-aware profiles.

Testing

Enable "Usage collection" global setting.

Navigate to Discover and observe kibana-browser requests in Network tab.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…profiles

…-ebt # Conflicts: # src/plugins/discover/public/plugin.tsx

elasticmachine · 2024-09-17T14:13:41Z

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

jughosta · 2024-09-18T14:07:33Z

Noticed an issue that once Discover is visited all following EBT events on other Kibana pages still have dscProfiles in their context. We should probably reset it when user navigates away from Discover, shouldn't we?

kertal · 2024-09-19T06:51:48Z

Noticed an issue that once Discover is visited all following EBT events on other Kibana pages still have dscProfiles in their context. We should probably reset it when user navigates away from Discover, shouldn't we?

I do agree this would be great, if we could do that, I think it was discussed as an acceptable tradeoff in an one older discussion if we can't. Just wanted to ask @davismcphee @flash1293, we are adding dscProfiles which makes absolutely sense for our current use case, but let's think beyond that, let's say the contextual awareness won't be limited to Discover, but will conquer the 🌏 ... or just Kibana, should we already think of making this more generic, like contextualProfiles, caProfiles (california 🏄 ) , someThingElse?

…-ebt # Conflicts: # src/plugins/discover/tsconfig.json

jughosta · 2024-09-19T07:53:22Z

I updated the approach, so now it should set and update dscProfiles only while on Discover page. If user navigates to another app then dscProfiles would be reset to [] and populated again on return to Discover.

802d3a2

Open for suggestions if dscProfiles naming does not fit.

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

…rofile.ts

davismcphee

Code changes look good and it works well, thanks! Left a few minor comments but nothing blocking.

Personally I'm ok with using a Discover specific property name to start since we don't know much about how profiles could work outside of Discover yet. We can always adjust later if needed. My only suggestion would be that maybe discoverProfiles would be clearer than dscProfiles for those unfamiliar with the property, but I'm happy with it either way.

davismcphee · 2024-09-24T01:42:31Z

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

@@ -130,6 +134,7 @@ export class ProfilesManager {
      return;
    }

+    this.trackActiveProfiles(this.rootContext$.getValue().profileId, context.profileId);


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?

@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.

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

davismcphee · 2024-09-24T01:58:37Z

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

+  constructor() {}
+
+  // https://docs.elastic.dev/telemetry/collection/event-based-telemetry
+  public register({ core }: { core: CoreSetup }) {


Nit: Initially I was confused about what register actually did until I read the code. I wonder if something like initialize would be clearer for this?

davismcphee · 2024-09-24T02:00:45Z

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

+            type: 'keyword',
+            _meta: {
+              description:
+                'List of profiles which are activated by Discover Context Awareness logic',


Suggested change

'List of profiles which are activated by Discover Context Awareness logic',

'List of active Discover context awareness profiles',

Just a suggestion for a potentially simpler description.

davismcphee · 2024-09-24T02:20:17Z

src/plugins/discover/public/plugin.tsx

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


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.

davismcphee · 2024-09-24T02:32:00Z

src/plugins/discover/public/plugin.tsx

    );
  }

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


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.

jughosta · 2024-09-24T08:45:52Z

@davismcphee Thanks for the review! I renamed to discoverProfiles and address your other suggestions.

davismcphee

Latest changes LGTM 👍 Thanks for improving our telemetry!

davismcphee · 2024-09-24T18:52:00Z

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

@@ -130,6 +134,7 @@ export class ProfilesManager {
      return;
    }

+    this.trackActiveProfiles(this.rootContext$.getValue().profileId, context.profileId);


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

kibana-ci · 2024-09-24T21:08:22Z

💚 Build Succeeded

Buildkite Build
Commit: 72a79ad

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	996	997	+1

Page load bundle

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

id	before	after	diff
`discover`	47.6KB	49.1KB	+1.5KB

History

💛 Build #236610 was flaky 37b12bf
💔 Build #236586 failed 0d0d141
💚 Build #236325 succeeded 0cec9c5
💛 Build #236009 was flaky adaca4b
💔 Build #235934 failed fa8c2c2
💛 Build #235865 was flaky 49e385f

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

cc @jughosta

…profiles (elastic#192908) - Closes elastic#186109 ## Summary This PR extends EBT context with `dscProfiles` - a list of active context-aware profiles. <img width="981" alt="Screenshot 2024-09-16 at 17 30 47" src="https://github.com/user-attachments/assets/64d49abc-3ee0-4d5a-8283-cdca5d78f963"> ## Testing Enable "Usage collection" global setting. Navigate to Discover and observe `kibana-browser` requests in Network tab. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit c28af87)

kibanamachine · 2024-09-24T21:12:52Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…aware profiles (#192908) (#193926) # Backport This will backport the following commits from `main` to `8.x`: - [[Discover] Extend EBT context with a list of activated context-aware profiles (#192908)](#192908)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Julia Rechkunova <[email protected]>

[Discover] Extend ebt context with a list of activated context-aware …

af9fc84

…profiles

jughosta self-assigned this Sep 13, 2024

jughosta added 9 commits September 16, 2024 16:59

[Discover] Update logic

e57b0c7

[Discover] Update logic

b842add

[Discover] Rename the prop

c5df319

[Discover] Update comment

56ef618

[Discover] Add tests

bfe2943

[Discover] Fix checks

64594e0

Merge branch 'main' into 186109-dsc-profiles-ebt

b42ca3a

Merge remote-tracking branch 'upstream/main' into 186109-dsc-profiles…

062a2e7

…-ebt # Conflicts: # src/plugins/discover/public/plugin.tsx

Merge branch 'main' into 186109-dsc-profiles-ebt

beb9d39

jughosta marked this pull request as ready for review September 17, 2024 14:13

jughosta requested a review from a team as a code owner September 17, 2024 14:13

jughosta changed the title ~~[Discover] Extend ebt context with a list of activated context-aware profiles~~ [Discover] Extend EBT context with a list of activated context-aware profiles Sep 17, 2024

jughosta marked this pull request as draft September 18, 2024 14:29

jughosta and others added 3 commits September 18, 2024 18:21

[Discover] Change the approach

802d3a2

[Discover] Clean up

12ffb6e

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

05fd9c5

jughosta added 2 commits September 19, 2024 09:43

Merge remote-tracking branch 'upstream/main' into 186109-dsc-profiles…

52b7cc4

…-ebt # Conflicts: # src/plugins/discover/tsconfig.json

[Discover] Clean up

32badc7

[Discover] Make it more generic

d7c9f94

jughosta marked this pull request as ready for review September 19, 2024 09:47

jughosta added 2 commits September 19, 2024 12:48

Merge branch 'main' into 186109-dsc-profiles-ebt

261b5f4

[Discover] Rename prop

49e385f

jughosta commented Sep 20, 2024

View reviewed changes

test/functional/apps/discover/context_awareness/_data_source_profile.ts Show resolved Hide resolved

jughosta added 3 commits September 20, 2024 14:39

Update test/functional/apps/discover/context_awareness/_data_source_p…

fa8c2c2

…rofile.ts

Merge branch 'main' into 186109-dsc-profiles-ebt

adaca4b

Merge branch 'main' into 186109-dsc-profiles-ebt

0cec9c5

davismcphee approved these changes Sep 24, 2024

View reviewed changes

jughosta added 3 commits September 24, 2024 10:37

Merge branch 'main' into 186109-dsc-profiles-ebt

5727e2a

[Discover] Rename according to PR comments

9a6bbff

[Discover] Make ebtContextManager private

0d0d141

jughosta requested a review from davismcphee September 24, 2024 08:45

[Discover] Rename in tests

37b12bf

davismcphee approved these changes Sep 24, 2024

View reviewed changes

Merge branch 'main' into 186109-dsc-profiles-ebt

72a79ad

jughosta enabled auto-merge (squash) September 24, 2024 19:45

jughosta merged commit c28af87 into elastic:main Sep 24, 2024
20 checks passed

kibanamachine added the v9.0.0 label Sep 24, 2024

kibanamachine mentioned this pull request Sep 24, 2024

[8.x] [Discover] Extend EBT context with a list of activated context-aware profiles (#192908) #193926

Merged

kibanamachine added the v8.16.0 label Sep 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jughosta commented Sep 13, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Sep 17, 2024

jughosta commented Sep 18, 2024

kertal commented Sep 19, 2024

jughosta commented Sep 19, 2024

davismcphee left a comment

davismcphee Sep 24, 2024

jughosta Sep 24, 2024

davismcphee Sep 24, 2024

davismcphee Sep 24, 2024

davismcphee Sep 24, 2024

davismcphee Sep 24, 2024

davismcphee Sep 24, 2024

jughosta commented Sep 24, 2024

davismcphee left a comment

davismcphee Sep 24, 2024

kibana-ci commented Sep 24, 2024

kibanamachine commented Sep 24, 2024

	'List of profiles which are activated by Discover Context Awareness logic',
	'List of active Discover context awareness profiles',

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

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

Conversation

jughosta commented Sep 13, 2024 • edited by kibanamachine Loading

Summary

Testing

Checklist

elasticmachine commented Sep 17, 2024

jughosta commented Sep 18, 2024

kertal commented Sep 19, 2024

jughosta commented Sep 19, 2024

davismcphee 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

jughosta commented Sep 24, 2024

davismcphee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Sep 24, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Page load bundle

History

kibanamachine commented Sep 24, 2024

💚 All backports created successfully

Questions ?

jughosta commented Sep 13, 2024 •

edited by kibanamachine

Loading