This repository was archived by the owner on Sep 30, 2024. It is now read-only.
Fix performance: delete activation from nav bar, user profile, admin overview page#43591
Merged
Fix performance: delete activation from nav bar, user profile, admin overview page#43591
Conversation
Contributor
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 68ad8ff...ab70cca.
|
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits ab70cca and 68ad8ff or learn more. Open explanation
|
umpox
approved these changes
Oct 28, 2022
Contributor
There was a problem hiding this comment.
I think this is fine to remove given the impact this is having, but just tagging @sourcegraph/iam for visibility. We might want to consider an alternative UI if this has more importance for site admins. (Non blocking)
Contributor
Author
There was a problem hiding this comment.
@kopancek already approved and @pjlast was on the Zoom call with me when I removed it :)
We should tag @erzhtor @thenamankumar and @rrhyne here so they're aware. @malomarrec also gave his approval below, so we're good to go.
malomarrec
approved these changes
Oct 28, 2022
mrnugget
force-pushed
the
mrn+pj/remove-activation
branch
from
f6eb570 to
0ee90cc
Compare
4 tasks
Contributor
Author
|
Build green. Running |
This was referenced Oct 28, 2022
Closed
eseliger
reviewed
Oct 28, 2022
Member
eseliger
left a comment
eseliger
left a comment
There was a problem hiding this comment.
LGTM, thanks for doing that so quickly!
BolajiOlajide
pushed a commit
that referenced
this pull request
Oct 28, 2022
…overview page (#43591) This removes the "search code, find ref, search symbol" onboarding checklist from - global nav bar - users profile page - admin analytics overview page Customer reported severe impact on database performance after upgrading from 3.43.2 to 4.0.1. The reason for the performance regression is a change made in #40674: the `user { usageStats }` GraphQL query would now hit PostgreSQL instead of Redis to get the usage stats. Here is the code path in 3.43.2 for `user { usageStats }`: https://github.com/sourcegraph/sourcegraph/blob/fd7599b9cbf3e80334a6167763b3c74ee9cddf9c/cmd/frontend/internal/usagestatsdeprecated/usage_stats.go#L42-L43 Here it is in 4.0+ https://github.com/sourcegraph/sourcegraph/blob/41e19f207038b4822dc9ebb052eb857e3dae9103/internal/usagestats/usage_stats.go#L110-L136 The SQL queries run by the latter have bad performance because they do multiple queries over one of the biggest tables we have: `event_logs`. Some of the performance problems with that have been addressed in https://github.com/sourcegraph/sourcegraph/pull/43564 and https://github.com/sourcegraph/sourcegraph/pull/43582. Big problem: **these queries are run on every page load, due to the `withActivation` wrapper we had around the `Layout` component.** That wrapper would load the `usageStatistics` for the currently-logged-in user on every page load to display that onboarding checklist at the top. On the customer's instance this resulted in slow queries piling up in the database, pinning CPU usage at 100% and exhausting the database connection limits. After discussing the issue in Slack, [we decided to remove this component altogether](https://sourcegraph.slack.com/archives/C03TGHH1S3V/p1666951569092379?thread_ts=1666747586.140109&cid=C03TGHH1S3V), since we deem the risk low: the component has been unchanged in 3 years, it's unloved and unowned, it has performance implications, and it most likely doesn't have any effect on user onboarding.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This removes this thing
from
Why?
Customer reported severe impact on database performance after upgrading from 3.43.2 to 4.0.1. The reason for the performance regression is a change made in #40674: the
user { usageStats }GraphQL query would now hit PostgreSQL instead of Redis to get the usage stats.Here is the code path in 3.43.2 for
user { usageStats }: https://github.com/sourcegraph/sourcegraph/blob/fd7599b9cbf3e80334a6167763b3c74ee9cddf9c/cmd/frontend/internal/usagestatsdeprecated/usage_stats.go#L42-L43Here it is in 4.0+ https://github.com/sourcegraph/sourcegraph/blob/41e19f207038b4822dc9ebb052eb857e3dae9103/internal/usagestats/usage_stats.go#L110-L136
The SQL queries run by the latter have bad performance because they do multiple queries over one of the biggest tables we have:
event_logs. Some of the performance problems with that have been addressed in https://github.com/sourcegraph/sourcegraph/pull/43564 and https://github.com/sourcegraph/sourcegraph/pull/43582.Big problem: these queries are run on every page load, due to the
withActivationwrapper we had around theLayoutcomponent. That wrapper would load theusageStatisticsfor the currently-logged-in user on every page load to display that onboarding checklist at the top.On the customer's instance this resulted in slow queries piling up in the database, pinning CPU usage at 100% and exhausting the database connection limits.
After discussing the issue in Slack, we decided to remove this component altogether, since we deem the risk low: the component has been unchanged in 3 years, it's unloved and unowned, it has performance implications, and it most likely doesn't have any effect on user onboarding.
Test plan
App preview:
Check out the client app preview documentation to learn more.