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

[IMPROVEMENT] Improve performance of rendering alerts and silences #372

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

KarboniteKream
Copy link
Member

@KarboniteKream KarboniteKream commented Nov 13, 2021

Description

This PR improves performance of rendering alerts and silences in Vue.

Currently, all alerts and silences are always rendered and hidden using the collapse CSS rule. This causes rendering delays on page load and whenever components need to be re-rendered (for example, on form input change; #370).

With changes introduced in this PR, all alerts and silences are now hidden behind v-if, so the HTML elements are not generated/rendered until the expression is truthy.

Detailed changes

  • Introduce toggleComponent(), which dynamically toggles the state of components
    • The initial state of all components is falsy, so they're not shown by default
    • Since the components object does not have any properties, it needs to be updated using this.$set()
  • Use toggleComponent() for all elements that previously used toggleTarget() and collapse
    • Add v-cloak to top-level element of specific components
  • Replace v-show with v-if so we skip rendering of v-for if it's not necessary
  • Remove unnecessary prevent modifier on @click
  • Improve caching of computed methods
    • We can reuse filterActive{Alerts,Silences}
    • Since we return a Set, there's no need to sort the values
  • Rename toggleTarget() to toggleCollapse()
    • Alerts and silences on the project detail page are expanded by default, so toggleComponent() cannot be used

Improvements

Before the improvements, the page would be mostly unresponsive until the Alerts block is rendered.
Note: All measurements are approximate page reload times without cache, and were performed with 1660 alerts, 25 silences and 6000 farms.

Component Before After
Home page 2s 0.8s
Global alerts section 2s 2.5s
Services page 5.2s 1.9s
Link Farm page 16s 6.7s
  • Page loads are now completely responsive, except for Link Farm since that needs to render the HTML of 6000 farms.
  • The global Alerts section, now takes 500ms longer to open, since the HTML elements need to be generated.
  • Form input lag is almost nonexistent, unless the global Alerts section is open.
  • The bottleneck now seem seem to be API responses

Copy link
Member Author

@KarboniteKream KarboniteKream left a comment

Choose a reason for hiding this comment

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

For the Link Farm page, we could improve the performance by limiting the number of items shown. Creating a separate API and controlling rendering in Vue would be one of the options.

<div class="panel-heading">
<a @click.prevent="toggleTarget('silences-project-{{project.name|slugify}}')" class="btn btn-warning btn-sm" role="button">Silences</a>
<a @click="toggleCollapse('silences-project-{{project.name|slugify}}')" class="btn btn-warning btn-sm" role="button">Silences</a>
</div>
<table id="silences-project-{{project.name|slugify}}" class="table table-bordered table-condensed">
<tr v-for="(silence, index) in filterActiveSilences" v-if="silence.labels.project == '{{project.name}}'">
Copy link
Member Author

Choose a reason for hiding this comment

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

Using v-for with v-if is not recommended. Additionally, this iterates over all active alerts/silences, which takes time, but currently doesn't look like a problem. I'll improve this in the next PR.

Comment on lines -158 to +155
return new Set(this.globalSilences
.filter(x => x.status.state == 'active')
return new Set(this.filterActiveSilences
Copy link
Member Author

@KarboniteKream KarboniteKream Nov 13, 2021

Choose a reason for hiding this comment

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

The behavior here has changed slightly. I noticed that filterActiveSilences was changed from state == 'active' to state != 'expired'. I think we want the same behavior here?

@kfdm kfdm self-requested a review November 15, 2021 00:22
@kfdm kfdm self-assigned this Nov 15, 2021
@kfdm kfdm merged commit 485fb77 into line:master Nov 16, 2021
@KarboniteKream KarboniteKream deleted the perf/alerts-silences branch November 16, 2021 05:08
kfdm added a commit that referenced this pull request Jan 12, 2022
* Extract silence form into a new `<silence-form>` component
  * Template is now included as a X-Template. Without transpilation, we don't have access to single file components, and this way we don't have to keep it as a string
* Extract shared state into a simple state store
  * This allowed to simplify silence label update functions
* Cleanup `silence_form.html`
  * CSRF tokens are no longer needed, since we pass them with cookies
  * `.lazy` modifier is no longer necessary after #372 
* Use arrow functions to avoid `this` reassignment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants