Skip to content

[dashboard] update feature flags only on demand #12800

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

Merged
merged 1 commit into from
Sep 12, 2022

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented Sep 9, 2022

Description

Each workspace page is loading the dashboard in iframe to show loading screen while workspace is not running. It causes a lot of requests each 3 minutes to ConfigCat, since the loading screen is present all the time in the background. This PR changes the strategy to update the settings values on demand and cache them for 3 minutes. For the workspace page it would mean no requests anymore. I don't think it makes big effect on other pages, but generally reduce amount of requests for the dashboard.

Related Issue(s)

Fixes #

How to test

  • Open the empty page and enable devtools to see network traffic.
  • Load the dashboard and check that request are done to ConfigCat.
  • Create couple projects and navigate between them. If navigation is done within the same minute you should not see requests, otherwise settings should be refreshed.
  • Start a workspace, since ConfigCat is caching per origin then you should not see any requests at all anymore.

Release Notes

NONE

Documentation

Werft options:

  • /werft with-preview

@akosyakov
Copy link
Member Author

akosyakov commented Sep 9, 2022

/werft run

👍 started the job as gitpod-build-ak-dashboard-lazy-experiments.1
(with .werft/ from main)

@akosyakov akosyakov marked this pull request as ready for review September 9, 2022 08:20
@akosyakov akosyakov requested a review from a team September 9, 2022 08:20
@akosyakov akosyakov requested a review from easyCZ September 9, 2022 08:20
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Sep 9, 2022
Comment on lines 38 to 42
const client = configcat.createClientWithLazyLoad(clientKey, {
logger: configcat.createConsoleLogger(LogLevel.Error),
pollIntervalSeconds: 60 * 3, // 3 minutes
maxInitWaitTimeSeconds: 0,
cacheTimeToLiveSeconds: 60 * 1, // 1 minute
});
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is desired behaviour from a UX perspective. This would block a UI element until the flags have been fetched.

What's the driver for this change?

Copy link
Member

Choose a reason for hiding this comment

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

What's the driver for this change?

Just read the description as well.

Copy link
Member Author

@akosyakov akosyakov Sep 9, 2022

Choose a reason for hiding this comment

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

What's the driver for this change?

Each workspace page is loading the dashboard in iframe to show loading screen while workspace is not running. It causes a lot of requests each 3 minutes to ConfigCat, since the loading screen is present all the time in the background.

I'm not sure if this is desired behaviour from a UX perspective. This would block a UI element until the flags have been fetched.

We can reduce timeout time to 1500 milliseconds as we did in VS Code Desktop.

Copy link
Member Author

Choose a reason for hiding this comment

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

I increased cache time to 3 mins and set timeout to 1500 milliseconds.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. I think I'm okay with this. Will just play around in preview.

Copy link
Member Author

@akosyakov akosyakov Sep 9, 2022

Choose a reason for hiding this comment

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

Also looking at the code it does not seem that it can block, since flags are computed from ConfigCat async in effect, so in worst case default values will be applied.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I should've been clearer. I meant block as in not show for example the Usage UI immediately, only after it's fetched.

This comment was marked as outdated.

Copy link
Member Author

@akosyakov akosyakov Sep 9, 2022

Choose a reason for hiding this comment

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

Sorry, I should've been clearer. I meant block as in not show for example the Usage UI immediately, only after it's fetched.

I don't think timing is changing. The client is created only on demand in use effect and it immediately calls getValueAsync after creation. I think it was almost the same before.

If we want to improve timing we should create client at time of load the js module.

@akosyakov akosyakov force-pushed the ak/dashboard_lazy_experiments branch from 9f11578 to cc06ad9 Compare September 9, 2022 08:26
Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

Let's try this out.

@roboquat roboquat merged commit da0410f into main Sep 12, 2022
@roboquat roboquat deleted the ak/dashboard_lazy_experiments branch September 12, 2022 11:03
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/S team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants