feat: 🎸 Refactor session recording filters #2981
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ZedLi
force-pushed
the
use-sqlite-for-filters-session-recording
branch
from
bdf1081 to
a2d70f7
Compare
ZedLi
force-pushed
the
llb/migrate-to-sqlite
branch
from
470e1fc to
a2b2088
Compare
ZedLi
force-pushed
the
use-sqlite-for-filters-session-recording
branch
from
a2d70f7 to
3e883c8
Compare
ZedLi
force-pushed
the
use-sqlite-for-filters-session-recording
branch
from
3e883c8 to
616202f
Compare
ZedLi
force-pushed
the
llb/migrate-to-sqlite
branch
from
94a5676 to
25f015b
Compare
ZedLi
force-pushed
the
use-sqlite-for-filters-session-recording
branch
from
616202f to
24a3df0
Compare
ZedLi
force-pushed
the
use-sqlite-for-filters-session-recording
branch
from
24a3df0 to
38cb9e0
Compare
ZedLi
force-pushed
the
use-sqlite-for-filters-session-recording
branch
from
38cb9e0 to
9595de1
Compare
ZedLi
force-pushed
the
use-sqlite-for-filters-session-recording
branch
from
9595de1 to
62b53a5
Compare
ZedLi
force-pushed
the
use-sqlite-for-filters-session-recording
branch
from
db6b30a to
5729b0a
Compare
There was a problem hiding this comment.
I like how these filters are now on-demand. The addition of search per column I can see being handy in other cases, too. Could the this.store.query() api for select reference in terms of the model attributes instead of columns since the mapping currently exists?
Also could returnRawData option be removed and the return of raw data is implied when distinct is used as a special case? returnRawData and pushToStore: false are similar and I'm wondering if having raw data opens up the result sets: pojos[], models[], rawData[], raw data being the one stands out because it's shape will be in terms of sql output
| if (distinctColumns.length > 0) { | ||
| selectColumns = `DISTINCT ${distinctColumns.map(({ field }) => field).join(', ')}`; | ||
| } else { |
There was a problem hiding this comment.
Is it a valid case to have a mix of distinct and regular columns selected? If it isn't, maybe an error could be thrown?
| ); | ||
| conditions.push(searchConditions); | ||
| } | ||
| function constructSelectClause(select = [{ field: '*' }], tableName) { |
There was a problem hiding this comment.
Is it easier to use the model mapping and have selects expressed in terms of the model attributes and mapped to the underlying column? It feels like the outer api is starting to blend between the model/attributes and row/columns.
There was a problem hiding this comment.
Hmm yeah you're right, this is a little leaky. I could make a reverse mapping of modelMapping (since it currently goes from column to JSON path) and use that so we don't have to be aware of the columns
ZedLi
force-pushed
the
use-sqlite-for-filters-session-recording
branch
from
5729b0a to
ae6ba6f
Compare
This is actually an interesting point because I wasn't sure of the best way to go about this. I agree it's not great but my thinking was the use of It does start feeling like we are having more escape hatches than I would like but to me this also forces a conscious decision that what you're getting back isn't a model. Realistically I think this is a code smell and I was originally thinking if the better solution is to just not go through |
| uniqueMap.set(id, { id, name: projectName, parent_scope_id }); | ||
| } | ||
| page: 1, | ||
| pageSize: 250, |
There was a problem hiding this comment.
Should we also update the Dropdown component item limit from 500 to 250 to keep it consistent (since its used in desktop app too)?
| userFilters: { | ||
| select: [ | ||
| { field: 'user_id', isDistinct: true }, | ||
| { field: 'user_name', isDistinct: true }, |
There was a problem hiding this comment.
would it complicate the select objects too much if it were changed so that isDistinct is only specified once since it looks like the parser only looks to see if at least one field isDistinct? Totally get if the object is structured this way for consistency.
Description
A proof of concept at using sqlite to load the various filter options in session recordings page. Previously, we were loading all session recordings to then filter down the various filter options we need which takes a huge amount of time just returning all the data to filter.
This PR has a few notable changes:
Screenshots (if appropriate)
How to Test
Use this cluster and try loading session recordings compared to before this PR.
Checklist
a11y-testslabel to run a11y audit tests if neededPCI review checklist
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.