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

Get rid of propertyDefinitionModel and eventDefinitionModel #8257

Closed
mariusandra opened this issue Jan 25, 2022 · 5 comments · Fixed by #11467
Closed

Get rid of propertyDefinitionModel and eventDefinitionModel #8257

mariusandra opened this issue Jan 25, 2022 · 5 comments · Fixed by #11467
Labels
bug Something isn't working right tech-debt

Comments

@mariusandra
Copy link
Collaborator

Bug description

There are many parts of the app that still rely on all propertyDefinitions to be loaded into propertyDefinitionModel before they start to work properly.

Unfortunately the property flattener plugin makes it possible to easily have 100k+ properties, rendering parts of the app flakey for the first 10sec or even (much, much) longer, while all the properties load:

2022-01-25 13 40 15

Some recent issues where this has come up:

We have a few heavy users who could even load hundreds of megabytes of properties into memory.

Expected behavior

Various filters and other widgets still rely on propertyDefinitionsModel and eventDefinitionsModel to be populated with data that's always there.

We need to remove these models, and introduce paginated data loading where needed.

@pauldambra
Copy link
Member

I don't think that #8255 is the same issue

Although I don't want that to take away from my agreement that the "maybe you can query for this thing" behaviour is confusing :)

@mariusandra
Copy link
Collaborator Author

mariusandra commented Feb 1, 2022

Here are the various ways propertyDefinitionsModel is used. Unfortunately there's no quick "one size fits all" solution, so we're going to have to uproot it one by one.

  • In the Properties table, we check for the type of the property and display it next to the actual property like string. We display the type in the database even if the actual displayed value has a different type:
    image

  • In the Events table the list of property names is used to configure table
    image

  • The OperatorValueSelect component uses the property type from the model to limit the comparisons you can do with a property
    image

  • The PropertyValue and SelectGradientOverflow components use formatForDisplay to format the list of values depending on the type of the property. This function currently just provides custom formatting for datetimes. Since we don't autocomplete them anymore, perhaps this can be removed. (Screenshot taken via a bug: set it to a string property, then type $time and press enter, you'll get the old "equals" filter with a list)
    image

  • But even if we don't use it in the list, we use it in the PropertyFilterButton pill:
    image

  • The MathSelector in ActionFilterRow toggles the option to sum/average/etc any numerical property, if we have any numerical properties, plus presents a list of numerical properties to choose from: Replace math property selector with taxonomic selector #8389
    image

  • The Properties stats page depends on this. Doesn't exist anymore.
    image

Depending on the usage, we need to either move to a universal paginated list, change to an autocomplete filter, or making bespoke queries to fetch just one property (e.g. loading a page with a filter --> find out what type it is).

@mariusandra
Copy link
Collaborator Author

mariusandra commented Feb 1, 2022

The eventDefinitionModel is used:

@pauldambra
Copy link
Member

excellent analysis 🙌

(Screenshot taken via a bug: set it to a string property, then type $time and press enter, you'll get the old "equals" filter with a list)

That component is going to make me go bald even faster! e.g. #7970

@pauldambra
Copy link
Member

I hope you've done a victory lap around the house @mariusandra

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working right tech-debt
Projects
Development

Successfully merging a pull request may close this issue.

3 participants