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

Add "Popular fields" to Lens as part of the "Unified Field List" initiative #147704

Closed
wants to merge 4 commits into from

Conversation

VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented Dec 16, 2022

Summary

Closes: #145687

Added "Popular fields" to Lens.

The field will appear in 'popular fields' section in following scenarios:

  • User will set popularity for field in dataview management section
  • Use the field in discover (as Lens and Discover use the same popularity counter if user add field in discover, that field also will appear in popular section in Lens)
  • User clicks the add field button from the field list
  • User drags and drops a field from the field list

@VladLasitsa VladLasitsa self-assigned this Dec 16, 2022
@VladLasitsa VladLasitsa added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens backport:skip This commit does not require backporting release_note:feature Makes this part of the condensed release notes v8.7.0 loe:medium Medium Level of Effort labels Dec 19, 2022
@VladLasitsa VladLasitsa marked this pull request as ready for review December 19, 2022 10:35
@VladLasitsa VladLasitsa requested a review from a team as a code owner December 19, 2022 10:35
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations)

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
lens 1.3MB 1.3MB +1.9KB
Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 61 67 +6
lens 19 20 +1
osquery 109 115 +6
securitySolution 439 445 +6
total +21

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 70 76 +6
lens 22 23 +1
osquery 110 117 +7
securitySolution 516 522 +6
total +22

History

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

cc @VladLasitsa

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

I would propose a small refactoring of the populaizeField function to do the actual work only in success case.

Found also a bug: when Combining two or more fields (using Top values multi-terms) the popularized function is not called.

I wonder if it makes sense to fire off a request every time one user uses a field. Would it be best to batch them by some longer time slot somehow? Do not think this is a vital information to have.

Comment on lines +346 to +378
if (!indexPatternId || !capabilities?.indexPatterns?.save) return;
const dataView = await dataViewsService.get(indexPatternId);
const field = dataView.fields.getByName(fieldName);
if (!field) {
return;
}

field.count++;

const refreshIndexPatternsListArgs = {
activeDatasources: Object.keys(datasourceStates).reduce(
(acc, datasourceId) => ({
...acc,
[datasourceId]: datasourceMap[datasourceId],
}),
{}
),
indexPatternId,
indexPatternService,
indexPatternsCache,
};

if (!dataView.isPersisted()) {
refreshIndexPatternsList(refreshIndexPatternsListArgs);
return;
}

// Catch 409 errors caused by user adding columns in a higher frequency that the changes can be persisted to Elasticsearch
try {
await dataViewsService.updateSavedObject(dataView, 0, true);
refreshIndexPatternsList(refreshIndexPatternsListArgs);
// eslint-disable-next-line no-empty
} catch {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would refactor a bit here to do the async work only in case of success for a persisted dataView:

Suggested change
if (!indexPatternId || !capabilities?.indexPatterns?.save) return;
const dataView = await dataViewsService.get(indexPatternId);
const field = dataView.fields.getByName(fieldName);
if (!field) {
return;
}
field.count++;
const refreshIndexPatternsListArgs = {
activeDatasources: Object.keys(datasourceStates).reduce(
(acc, datasourceId) => ({
...acc,
[datasourceId]: datasourceMap[datasourceId],
}),
{}
),
indexPatternId,
indexPatternService,
indexPatternsCache,
};
if (!dataView.isPersisted()) {
refreshIndexPatternsList(refreshIndexPatternsListArgs);
return;
}
// Catch 409 errors caused by user adding columns in a higher frequency that the changes can be persisted to Elasticsearch
try {
await dataViewsService.updateSavedObject(dataView, 0, true);
refreshIndexPatternsList(refreshIndexPatternsListArgs);
// eslint-disable-next-line no-empty
} catch {}
if (!indexPatternId || !capabilities?.indexPatterns?.save) return;
const lensDataView = indexPatternsCache[indexPatternId];
if (!lensDataView.getFieldByName(fieldName)) {
return;
}
if (lensDataView.isPersisted) {
// Catch 409 errors caused by user adding columns in a higher frequency that the changes can be persisted to Elasticsearch
try {
const dataView = await dataViewsService.get(indexPatternId);
const field = dataView.fields.getByName(fieldName);
if (field) {
field.count++;
await dataViewsService.updateSavedObject(dataView, 0, true);
}
// eslint-disable-next-line no-empty
} catch {}
}
const refreshIndexPatternsListArgs = {
activeDatasources: Object.keys(datasourceStates).reduce(
(acc, datasourceId) => ({
...acc,
[datasourceId]: datasourceMap[datasourceId],
}),
{}
),
indexPatternId,
indexPatternService,
indexPatternsCache,
};
refreshIndexPatternsList(refreshIndexPatternsListArgs);

It is a little bit more verbose but it saves some work for adHoc dataviews

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you @dej611, I will update my function. About batch requests I don't think that is a good idea. I call refreshIndexPatternsList here to show correct state of fields after updating popularity. In you proposal we should call refreshIndexPatternsList after some timeout which lead to showing correct list of popular fields with delay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dej611, also for adHoc dataviews we also should use this field.count++; not only for usual dataviews.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's wait for the main decision on the feature first.
But for the adHoc I think that operating on the cached version should be ok, no?

@stratoula
Copy link
Contributor

stratoula commented Dec 19, 2022

@dej611 one important note on this feature is that we want the counter to initialize only for actions made by the fields list.
This means:

  • add icon
  • drag and drop
    Personally I don't agree with this addition, in my point of view the popularity in lens is fields that are used in saved visualizations taking under consideration that I can change fields all the time (and operations).
    But this was the product request :)

@VladLasitsa
Copy link
Contributor Author

@dej611 one important note on this feature is that we want the counter to initialize only for actions made by the fields list. This means:

  • add icon
  • drag and drop
    Personally I don't agree with this addition, in my point of view the popularity in lens is fields that are used in saved visualizations taking under consideration that I can change fields all the time (and operations).
    But this was the product request :)

I've already added this info in PR description:)

@dej611
Copy link
Contributor

dej611 commented Dec 19, 2022

@dej611 one important note on this feature is that we want the counter to initialize only for actions made by the fields list. This means:

  • add icon
  • drag and drop
    Personally I don't agree with this addition, in my point of view the popularity in lens is fields that are used in saved visualizations taking under consideration that I can change fields all the time (and operations).
    But this was the product request :)

But combine is a drag and drop action 🤔

@stratoula
Copy link
Contributor

Yes I have tried to convince against this one and take some time to think about it more but it seems that it has been decided

cc @timductive @ninoslavmiskovic

@dej611
Copy link
Contributor

dej611 commented Dec 19, 2022

Besides the specific implementation I have few questions regarding this feature: what value is it going to bring to the user?

  • After trying it for testing I found the feature very invasive as "enabled by default" as it was using too much UI space to deliver very few useful information. At the second visualization I built I closed the accordion as it was just reducing the available list of fields I wanted to use.
  • the most confusing part is that the counter is shared between Lens and Discover now, which makes little sense to me. I think at least a different counter should be used for each use case: one for discover and another for data analysis.
  • too much noise is generated now: fields are dancing too much when creating a visualization, and the remove action is not taken into account at all. To me "popular" should be counted only for those fields actually used by the user - so, for instance also those picked from the field picker in the configuration panel, not covered here.
  • the amount of traffic generated can be quite high to the datasource. The code here is ignoring saving errors as fallback, but why poll the datasource in the first place?

Just few thoughts for a discussion here. cc @stratoula @ninoslavmiskovic

@VladLasitsa
Copy link
Contributor Author

But combine is a drag and drop action 🤔

Hm, but as I understand combine can be from field list or between dimensions, right?

@stratoula
Copy link
Contributor

I couldn't agree more @dej611. I will try to raise it again!

@dej611
Copy link
Contributor

dej611 commented Dec 19, 2022

But combine is a drag and drop action 🤔

Hm, but as I understand combine can be from field list or between dimensions, right?

combine_dnd

@VladLasitsa
Copy link
Contributor Author

VladLasitsa commented Dec 19, 2022

@dej611, Some thinking:

  • the most confusing part is that the counter is shared between Lens and Discover now, which makes little sense to me. I think at least a different counter should be used for each use case: one for discover and another for data analysis.

If we separate counter, what should we do with popularity which can be set in dataview management section?

  • too much noise is generated now: fields are dancing too much when creating a visualization, and the remove action is not taken into account at all. To me "popular" should be counted only for those fields actually used by the user - so, for instance also those picked from the field picker in the configuration panel, not covered here.
  • the amount of traffic generated can be quite high to the datasource. The code here is ignoring saving errors as fallback, but why poll the datasource in the first place?

About these two: I have used the same approach as in Discover (see here src/plugins/discover/public/utils/popularize_field.ts). One exception: In Discover if you remove field it will also increase popularity

@stratoula
Copy link
Contributor

@VladLasitsa @dej611 let's pause it for now. I pinged Tim to revisit because 3 engineers disagree with this feature and I think we should re-evaluate.

@stratoula
Copy link
Contributor

Moving this to draft as we decided to discuss more.

@stratoula stratoula marked this pull request as draft December 19, 2022 16:32
@ninoslavmiskovic
Copy link
Contributor

I have setup a meeting in the new year,so that we can align. I will go through "what we know today" regarding popular fields, what we aim to learn from market feedback and what user problems we are aiming to solve, when added to Lens. The aim is to bring clarity and most importantly provide value to the users, like it does in Discover.

@VladLasitsa
Copy link
Contributor Author

Since I will finish work on the project this year, removed my assignment, feel free to continue working here or close this PR.

@VladLasitsa VladLasitsa removed their assignment Dec 20, 2022
@kertal
Copy link
Member

kertal commented Dec 30, 2022

Besides the specific implementation I have few questions regarding this feature: what value is it going to bring to the user?

  • After trying it for testing I found the feature very invasive as "enabled by default" as it was using too much UI space to deliver very few useful information. At the second visualization I built I closed the accordion as it was just reducing the available list of fields I wanted to use.

Yes, I do agree. In Lens it takes too much screen space, and when there are just a few popular fields, a new field that was just popularized is eating up vertical space in "realtime"

Kapture 2022-12-30 at 22 26 18

Users can collapse the field sections, but it's not remembered - we should build in functionality to remember the collapse state in e.g. localStorage. FYI @jughosta because it would be a neat unified field list feature

  • the most confusing part is that the counter is shared between Lens and Discover now, which makes little sense to me. I think at least a different counter should be used for each use case: one for Discover and another for data analysis.

Yes, I also agree that different use cases need different field stats. And the pattern of increasing the statistics directly in the data view saved object I would consider has legacy behavior. The statistics isn't even accurate because of:

Catch 409 errors caused by user adding columns in a higher frequency that the changes can be persisted to Elasticsearch
So when you add field in Discover in a higher frequency than Elasticsearch allows, not every counter increase is persisted.

  • The quick solution was to hide this error (a long time ago)
  • The better solution would be to batch the updates
  • The best solution, IMO, would be, not to updated statistics in the data view saved object

I think popular fields make sense to help the user find his frequently used fields in a quicker way. This may vary by application and also by user type. What could be a solution to a) still use the counter in the data views saved object b) add statistics by user / application ?

For example we could still use the counter of the data view saved object, so admins could use those to provide base values. And then we would collect the the increments of the the counter values by application in localStorage. Then Lens/Discover would have different statistics based on a users field selection.

This would be the simplest solution that comes to my mind (and we should aim for simplicity)

FYI @mattkime because we chatted about the field popularization a while ago, you mentioned to could be collected in a separate saved object ... a solution like this would also provide a way to collect different statistic / application ... however it sounds like more effort

  • too much noise is generated now: fields are dancing too much when creating a visualization, and the remove action is not taken into account at all. To me "popular" should be counted only for those fields actually used by the user - so, for instance also those picked from the field picker in the configuration panel, not covered here.

I do agree about the "dancing", but I think it's also fine to limit the popularity to those fields the user interacted with in the sidebar field selection.

  • the amount of traffic generated can be quite high to the datasource. The code here is ignoring saving errors as fallback, but why poll the datasource in the first place?

You mean the 409 ignorance, when clicking faster than Elasticsearch allows, right?

@jughosta
Copy link
Contributor

jughosta commented Jan 4, 2023

Adding functionality to persist collapsed/expanded state of sections in localStorage as @kertal suggested: #148373

@dej611
Copy link
Contributor

dej611 commented Jan 9, 2023

So when you add field in Discover in a higher frequency than Elasticsearch allows, not every counter increase is persisted.

  • The quick solution was to hide this error (a long time ago)
  • The better solution would be to batch the updates
  • The best solution, IMO, would be, not to updated statistics in the data view saved object

I do agree about the "dancing", but I think it's also fine to limit the popularity to those fields the user interacted with in the sidebar field selection.

I would be ok, to me, to record user interactions as long as all add/remove operation are used. But this is not the case here, as only "some" adding flows are tracked and no removal.
Perhaps a radical approach here: why not update scores on editor exit? That can be either a save or "return to" action. That would reduce the traffic to ES and provide a meaningful score to the user, in my opinion.

You mean the 409 ignorance, when clicking faster than Elasticsearch allows, right?

Yes.

@kertal
Copy link
Member

kertal commented Jan 9, 2023

I would be ok, to me, to record user interactions as long as all add/remove operation are used. But this is not the case here, as only "some" adding flows are tracked and no removal. Perhaps a radical approach here: why not update scores on editor exit? That can be either a save or "return to" action. That would reduce the traffic to ES and provide a meaningful score to the user, in my opinion.

I do agree, when the user is interacting with the interface, popularity increase should not change the interface. And updating the score not immediately, in a patched way on certain events would be a good way to get rid of the 409s and get better statistics. One thing I've always been wondering, why should fields, when they are removed get more "popular"?

You mean the 409 ignorance, when clicking faster than Elasticsearch allows, right?

Yes.

Thx

Great work @jughosta BTW for already implementing state of the sections in localStorage 👍

@stratoula
Copy link
Contributor

@kertal tbh I was wondering the same thing. Following the code in Discover I see that we popularizeField when:

  • adding a column
  • removing a column
  • creating a filter from a field

but not when I am doing other things with the fields such as:

  • Visualize field
  • Add a field as a breakdown
  • etc

While I strongly believe that the popular fields in Lens should be fields that are actually used a lot on saved visualizations I think that we might consider re-thinking what popular fields mean in discover too.

@legrego legrego closed this Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens loe:medium Medium Level of Effort release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "Popular fields" to Lens as part of the "Unified Field List" initiative
10 participants