diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2019-11-06 00:07:46 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2019-11-06 00:07:46 +0300 |
commit | 82cef8dd1f48ffbc7aaa1ff7374cdb859137e01e (patch) | |
tree | 0ebdac2f1880dd1dd3f73a956082759314e0994f /doc | |
parent | 2baa63e740214382387abe77eeea6c0b1759e621 (diff) |
Add latest changes from gitlab-org/gitlab@master
Diffstat (limited to 'doc')
-rw-r--r-- | doc/api/commits.md | 1 | ||||
-rw-r--r-- | doc/api/issues.md | 2 | ||||
-rw-r--r-- | doc/api/merge_requests.md | 12 | ||||
-rw-r--r-- | doc/api/search.md | 3 | ||||
-rw-r--r-- | doc/development/contributing/merge_request_workflow.md | 2 | ||||
-rw-r--r-- | doc/development/merge_request_performance_guidelines.md | 179 |
6 files changed, 197 insertions, 2 deletions
diff --git a/doc/api/commits.md b/doc/api/commits.md index 3927a4bbc62..3f2932f2666 100644 --- a/doc/api/commits.md +++ b/doc/api/commits.md @@ -670,6 +670,7 @@ Example response: "merge_status":"can_be_merged", "sha":"af5b13261899fb2c0db30abdd0af8b07cb44fdc5", "merge_commit_sha":null, + "squash_commit_sha":null, "user_notes_count":0, "discussion_locked":null, "should_remove_source_branch":null, diff --git a/doc/api/issues.md b/doc/api/issues.md index 0ddbb18ce92..ded412a7af0 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -1416,6 +1416,7 @@ Example response: "merge_status": "cannot_be_merged", "sha": "3b7b528e9353295c1c125dad281ac5b5deae5f12", "merge_commit_sha": null, + "squash_commit_sha": null, "discussion_locked": null, "should_remove_source_branch": null, "force_remove_source_branch": false, @@ -1546,6 +1547,7 @@ Example response: "merge_status": "unchecked", "sha": "5a62481d563af92b8e32d735f2fa63b94e806835", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "should_remove_source_branch": null, "force_remove_source_branch": false, diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index ef5cbeb015e..7074d0249ef 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -126,6 +126,7 @@ Parameters: "merge_status": "can_be_merged", "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -287,6 +288,7 @@ Parameters: "merge_status": "can_be_merged", "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -440,6 +442,7 @@ Parameters: "merge_status": "can_be_merged", "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -563,6 +566,7 @@ Parameters: "merge_error": null, "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -769,6 +773,7 @@ Parameters: "subscribed" : true, "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "changes_count": "1", "should_remove_source_branch": true, @@ -976,6 +981,7 @@ order for it to take effect: "merge_error": null, "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -1129,6 +1135,7 @@ Must include at least one non-required attribute from above. "merge_error": null, "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -1298,6 +1305,7 @@ Parameters: "merge_error": null, "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -1470,6 +1478,7 @@ Parameters: "merge_error": null, "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -1755,6 +1764,7 @@ Example response: "merge_status": "can_be_merged", "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -1900,6 +1910,7 @@ Example response: "merge_status": "can_be_merged", "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -2061,6 +2072,7 @@ Example response: "subscribed": true, "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 7, "changes_count": "1", "should_remove_source_branch": true, diff --git a/doc/api/search.md b/doc/api/search.md index f9bd5143018..8e20722052e 100644 --- a/doc/api/search.md +++ b/doc/api/search.md @@ -181,6 +181,7 @@ Example response: "merge_status": "can_be_merged", "sha": "78765a2d5e0a43585945c58e61ba2f822e4d090b", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 0, "discussion_locked": null, "should_remove_source_branch": null, @@ -583,6 +584,7 @@ Example response: "merge_status": "can_be_merged", "sha": "78765a2d5e0a43585945c58e61ba2f822e4d090b", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 0, "discussion_locked": null, "should_remove_source_branch": null, @@ -890,6 +892,7 @@ Example response: "merge_status": "can_be_merged", "sha": "78765a2d5e0a43585945c58e61ba2f822e4d090b", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 0, "discussion_locked": null, "should_remove_source_branch": null, diff --git a/doc/development/contributing/merge_request_workflow.md b/doc/development/contributing/merge_request_workflow.md index b9d55a9395c..39557cf4174 100644 --- a/doc/development/contributing/merge_request_workflow.md +++ b/doc/development/contributing/merge_request_workflow.md @@ -222,7 +222,7 @@ requirements. on the CI server. 1. Regressions and bugs are covered with tests that reduce the risk of the issue happening again. -1. Performance/scalability implications have been considered, addressed, and tested. +1. [Performance guidelines](../merge_request_performance_guidelines.md) have been followed. 1. [Documented](../documentation/index.md) in the `/doc` directory. 1. [Changelog entry added](../changelog.md), if necessary. 1. Reviewed by relevant (UX/FE/BE/tech writing) reviewers and all concerns are addressed. diff --git a/doc/development/merge_request_performance_guidelines.md b/doc/development/merge_request_performance_guidelines.md index 4456e5e6d18..2e80e813a4b 100644 --- a/doc/development/merge_request_performance_guidelines.md +++ b/doc/development/merge_request_performance_guidelines.md @@ -1,7 +1,9 @@ # Merge Request Performance Guidelines +Each new introduced merge request **should be performant by default**. + To ensure a merge request does not negatively impact performance of GitLab -_every_ merge request **must** adhere to the guidelines outlined in this +_every_ merge request **should** adhere to the guidelines outlined in this document. There are no exceptions to this rule unless specifically discussed with and agreed upon by backend maintainers and performance specialists. @@ -12,6 +14,19 @@ the following guides: - [Performance Guidelines](performance.md) - [What requires downtime?](what_requires_downtime.md) +## Definition + +The term `SHOULD` per the [RFC 2119](https://www.ietf.org/rfc/rfc2119.txt) means: + +> This word, or the adjective "RECOMMENDED", mean that there +> may exist valid reasons in particular circumstances to ignore a +> particular item, but the full implications must be understood and +> carefully weighed before choosing a different course. + +Ideally, each of these tradeoffs should be documented +in the separate issues, labelled accordingly and linked +to original issue and epic. + ## Impact Analysis **Summary:** think about the impact your merge request may have on performance @@ -44,6 +59,64 @@ should ask one of the merge request reviewers to review your changes. You can find a list of these reviewers at <https://about.gitlab.com/company/team/>. A reviewer in turn can request a performance specialist to review the changes. +## Think outside of the box + +Everyone has their own perception how the new feature is going to be used. +Always consider how users might be using the feature instead. Usually, +users test our features in a very unconventional way, +like by brute forcing or abusing edge conditions that we have. + +## Data set + +The data set that will be processed by the merge request should be known +and documented. The feature should clearly document what the expected +data set is for this feature to process, and what problems it might cause. + +If you would think about the following example that puts +a strong emphasis of data set being processed. +The problem is simple: you want to filter a list of files from +some git repository. Your feature requests a list of all files +from the repository and perform search for the set of files. +As an author you should in context of that problem consider +the following: + +1. What repositories are going to be supported? +1. How long it will take for big repositories like Linux kernel? +1. Is there something that we can do differently to not process such a + big data set? +1. Should we build some fail-safe mechanism to contain + computational complexity? Usually it is better to degrade + the service for a single user instead of all users. + +## Query plans and database structure + +The query plan can answer the questions whether we need additional +indexes, or whether we perform expensive filtering (i.e. using sequential scans). + +Each query plan should be run against substantional size of data set. +For example if you look for issues with specific conditions, +you should consider validating the query against +a small number (a few hundred) and a big number (100_000) of issues. +See how the query will behave if the result will be a few +and a few thousand. + +This is needed as we have users using GitLab for very big projects and +in a very unconventional way. Even, if it seems that it is unlikely +that such big data set will be used, it is still plausible that one +of our customers will have the problem with the feature. + +Understanding ahead of time how it is going to behave at scale even if we accept it, +is the desired outcome. We should always have a plan or understanding what it takes +to optimise feature to the magnitude of higher usage patterns. + +Every database structure should be optimised and sometimes even over-described +to be prepared to be easily extended. The hardest part after some point is +data migration. Migrating millions of rows will always be troublesome and +can have negative impact on application. + +To better understand how to get help with the query plan reviews +read this section on [how to prepare the merge request for a database review](https://docs.gitlab.com/ee/development/database_review.html#how-to-prepare-the-merge-request-for-a-database-review). + ## Query Counts **Summary:** a merge request **should not** increase the number of executed SQL @@ -172,3 +245,107 @@ Caching data per transaction can be done using `Gitlab::SafeRequestStore` to avoid having to remember to check `RequestStore.active?`). Caching data in Redis can be done using [Rails' caching system](https://guides.rubyonrails.org/caching_with_rails.html). + +## Pagination + +Each feature that renders a list of items as a table needs to include pagination. + +The main styles of pagination are: + +1. Offset-based pagination: user goes to a specific page, like 1. User sees the next page number, + and the total number of pages. This style is well supported by all components of GitLab. +1. Offset-based pagination, but without the count: user goes to a specific page, like 1. + User sees only the next page number, but does not see the total amount of pages. +1. Next page using keyset-based pagination: user can only go to next page, as we do not know how many pages + are available. +1. Infinite scrolling pagination: user scrolls the page and next items are loaded asynchronously. This is ideal, + as it has exact same benefits as the previous one. + +The ultimately scalable solution for pagination is to use Keyset-based pagination. +However, we don't have support for that at GitLab at that moment. You +can follow the progress looking at [API: Keyset Pagination +](https://gitlab.com/groups/gitlab-org/-/epics/2039). + +Take into consideration the following when choosing a pagination strategy: + +1. It is very inefficient to calculate amount of objects that pass the filtering, + this operation usually can take seconds, and can time out, +1. It is very inefficent to get entries for page at higher ordinals, like 1000. + The database has to sort and iterate all previous items, and this operation usually + can result in substantial load put on database. + +## Badge counters + +Counters should always be truncated. It means that we do not want to present +the exact number over some threshold. The reason for that is for the cases where we want +to calculate exact number of items, we effectively need to filter each of them for +the purpose of knowing the exact number of items matching. + +From ~UX perspective it is often acceptable to see that you have over 1000+ pipelines, +instead of that you have 40000+ pipelines, but at a tradeoff of loading page for 2s longer. + +An example of this pattern is the list of pipelines and jobs. We truncate numbers to `1000+`, +but we show an accurate number of running pipelines, which is the most interesting information. + +There's a helper method that can be used for that purpose - `NumbersHelper.limited_counter_with_delimiter` - +that accepts an upper limit of counting rows. + +In some cases it is desired that badge counters are loaded asynchronously. +This can speed up the initial page load and give a better user experience overall. + +## Application/misuse limits + +Every new feature should have safe usage quotas introduced. +The quota should be optimised to a level that we consider the feature to +be performant and usable for the user, but **not limiting**. + +**We want the features to be fully usable for the users.** +**However, we want to ensure that the feature will continue to perform well if used at its limit** +**and it will not cause availability issues.** + +Consider that it is always better to start with some kind of limitation, +instead of later introducing a breaking change that would result in some +workflows breaking. + +The intent is to provide a safe usage pattern for the feature, +as our implementation decisions are optimised for the given data set. +Our feature limits should reflect the optimisations that we introduced. + +The intent of quotas could be different: + +1. We want to provide higher quotas for higher tiers of features: + we want to provide on GitLab.com more capabilities for different tiers, +1. We want to prevent misuse of the feature: someone accidentially creates + 10000 deploy tokens, because of a broken API script, +1. We want to prevent abuse of the feature: someone purposely creates + a 10000 pipelines to take advantage of the system. + +Examples: + +1. Pipeline Schedules: It is very unlikely that user will want to create + more than 50 schedules. + In such cases it is rather expected that this is either misuse + or abuse of the feature. Lack of the upper limit can result + in service degredation as the system will try to process all schedules + assigned the the project. + +1. GitLab CI includes: We started with the limit of maximum of 50 nested includes. + We understood that performance of the feature was acceptable at that level. + We received a request from the community that the limit is too small. + We had a time to understand the customer requirement, and implement an additional + fail-safe mechanism (time-based one) to increase the limit 100, and if needed increase it + further without negative impact on availability of the feature and GitLab. + +## Usage of feature flags + +Each feature that has performance critical elements or has a known performance deficiency +needs to come with feature flag to disable it. + +The feature flag makes our team more happy, because they can monitor the system and +quickly react without our users noticing the problem. + +Performance deficiencies should be addressed right away after we merge initial +changes. + +Read more about when and how feature flags should be used in +[Feature flags in GitLab development](https://docs.gitlab.com/ee/development/feature_flags/process.html#feature-flags-in-gitlab-development). |