You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "justinpark (via GitHub)" <gi...@apache.org> on 2025/03/11 23:35:13 UTC
[PR] fix(welcome): perf on distinct recent activities [superset]
justinpark opened a new pull request, #32608:
URL: https://github.com/apache/superset/pull/32608
### SUMMARY
Currently, the `recent_activity` query for the log table groups by the dashboard ID and slice ID to extract a distinct list from the entire log table, which leads to performance issues.
(In the case of Airbnb, more than 1 million logs are generated each day, and grouping by dashboard and slice ID, even with indexing, significantly impacts database performance as shown in the following log)
```
| 3793593 | superset | 100.117.121.20:41904 | superset_production | Query | 1726 s| Sending data | SELECT anon_1.dashboard_id AS anon_1_dashboard_id, anon_1.slice_id AS anon_1_slice_id, anon_1.action AS anon_1_action, anon_1.dttm AS anon_1_dttm, dashboards.slug AS dashboard_slug, dashboards.dashboard_title AS dashboards_dashboard_title, slices.slice_name AS slices_slice_name
FROM (SELECT logs.dashboard_id AS dashboard_id, logs.slice_id AS slice_id, logs.action AS action, max(logs.dttm) AS dttm
FROM logs
WHERE logs.action IN ('explore', 'dashboard') AND logs.user_id = 13295 AND logs.dttm > '2024-03-06 18:00:12.201653' AND (logs.dashboard_id IS NOT NULL OR logs.slice_id IS NOT NULL) GROUP BY logs.dashboard_id, logs.slice_id, logs.action) AS anon_1 LEFT OUTER JOIN dashboards ON dashboards.id = anon_1.dashboard_id LEFT OUTER JOIN slices ON slices.id = anon_1.slice_id
WHERE dashboards.dashboard_title != '' OR slices.slice_name != '' ORDER BY anon_1.dttm DESC
LIMIT 0, 6 |
```
To resolve this issue, it would be appropriate to create a materialized activity statistics view through a daily/hourly batch job. However, functionally, the main purpose of recent activity is to display only a few of the most recently visited items. Therefore, we improved performance by changing the approach to fetch the latest (including duplicates) log entries (by `distinct: false`) and extract a distinct list using LRU cache in the frontend side.
### TESTING INSTRUCTIONS
specs
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(welcome): perf on distinct recent activities [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #32608:
URL: https://github.com/apache/superset/pull/32608#discussion_r1991343569
##########
superset-frontend/src/views/CRUD/utils.tsx:
##########
@@ -223,10 +224,14 @@ export const getRecentActivityObjs = (
) =>
SupersetClient.get({ endpoint: recent }).then(recentsRes => {
const res: any = {};
+ const distinctRes = lruCache<RecentActivity>(6);
+ recentsRes.json.result.reverse().forEach((record: RecentActivity) => {
+ distinctRes.set(record.item_url, record);
+ });
return getFilteredChartsandDashboards(addDangerToast, filters).then(
({ other }) => {
res.other = other;
- res.viewed = recentsRes.json.result;
+ res.viewed = distinctRes.values().reverse();
Review Comment:
Is it necessary to reverse twice or can this be optimized?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(welcome): perf on distinct recent activities [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #32608:
URL: https://github.com/apache/superset/pull/32608#issuecomment-2718598709
> > Its endpoint is /api/v1/log/recent_activity/ and the distinct param is included in the schema so it will be a break change.
>
> Oh, you're correct!
Therefore, it improves the frontend to fetch in an efficient way and keeps the API specification as-is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(welcome): perf on distinct recent activities [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #32608:
URL: https://github.com/apache/superset/pull/32608#issuecomment-2718400784
> Given that this endpoint is not under v1, it's not a breaking change.
Its endpoint is `/api/v1/log/recent_activity/` and the distinct param is included in the schema so it will be a break change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(welcome): perf on distinct recent activities [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32608:
URL: https://github.com/apache/superset/pull/32608#discussion_r1990298646
##########
superset-frontend/packages/superset-ui-core/src/utils/lruCache.ts:
##########
@@ -67,6 +67,10 @@ class LRUCache<T> {
public get size() {
return this.cache.size;
}
+
+ public entries(): T[] {
+ return [...this.cache.values()];
+ }
Review Comment:
### Misleading Method Name <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The entries() method name is misleading as it returns only values, not entries (key-value pairs) as is standard in JavaScript/TypeScript.
###### Why this matters
This can lead to confusion and bugs as developers expect entries() to return key-value pairs based on standard JavaScript Map/Set behavior.
###### Suggested change ∙ *Feature Preview*
Either rename the method to values() to match its actual behavior or modify it to return entries:
```typescript
public entries(): [string, T][] {
return [...this.cache.entries()];
}
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9f6aba30-dfbc-48be-a6bd-164209d2a014?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:c6554d10-a090-43ac-b197-01c033a34065 -->
##########
superset-frontend/src/views/CRUD/utils.tsx:
##########
@@ -223,10 +224,14 @@ export const getRecentActivityObjs = (
) =>
SupersetClient.get({ endpoint: recent }).then(recentsRes => {
const res: any = {};
+ const distinctRes = lruCache<RecentActivity>(6);
+ recentsRes.json.result.reverse().forEach((record: RecentActivity) => {
+ distinctRes.set(record.item_url, record);
+ });
Review Comment:
### Incorrect Chronological Order in Recent Activities <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The original array is being reversed before being processed, which modifies the chronological order of activities before applying the LRU cache.
###### Why this matters
This can lead to incorrect chronological ordering of recent activities in the UI, as older activities might appear as more recent than they actually are.
###### Suggested change ∙ *Feature Preview*
Process the records in their original order to maintain correct chronological sequence:
```typescript
recentsRes.json.result.forEach((record: RecentActivity) => {
distinctRes.set(record.item_url, record);
});
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ddc1346b-0ff3-4f40-920d-a4e0c331b4a5?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e78762da-335f-4900-8893-7f8cc66b6756 -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(welcome): perf on distinct recent activities [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32608:
URL: https://github.com/apache/superset/pull/32608#issuecomment-2718456034
> Its endpoint is /api/v1/log/recent_activity/ and the distinct param is included in the schema so it will be a break change.
Oh, you're correct!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(welcome): perf on distinct recent activities [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #32608:
URL: https://github.com/apache/superset/pull/32608#discussion_r1991833541
##########
superset-frontend/src/views/CRUD/utils.tsx:
##########
@@ -223,10 +224,14 @@ export const getRecentActivityObjs = (
) =>
SupersetClient.get({ endpoint: recent }).then(recentsRes => {
const res: any = {};
+ const distinctRes = lruCache<RecentActivity>(6);
+ recentsRes.json.result.reverse().forEach((record: RecentActivity) => {
+ distinctRes.set(record.item_url, record);
+ });
return getFilteredChartsandDashboards(addDangerToast, filters).then(
({ other }) => {
res.other = other;
- res.viewed = recentsRes.json.result;
+ res.viewed = distinctRes.values().reverse();
Review Comment:
The API returns the latest accesses in reverse chronological order. (i.e. [2025-03-10, 2025-03-09, 2025-03-07, ...]) For an LRU cache, access records need to be stored in chronological order. Therefore, the API list must be reversed to maintain a record of accesses in chronological order. After that, since the final LRU cache list has the most recent entries placed at the end, it needs to be reversed again to position the latest entries at the front.
This two-step reversal process is an algorithmically correct approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(welcome): perf on distinct recent activities [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark merged PR #32608:
URL: https://github.com/apache/superset/pull/32608
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org