Add latest changes from gitlab-org/gitlab@master

app/assets/javascripts/raven/index.js → app/assets/javascripts/sentry/index.js

-import RavenConfig from './raven_config';
+import SentryConfig from './sentry_config';
 const index = function index() {
-  RavenConfig.init({
-    sentryDsn: gon.sentry_dsn,
+  SentryConfig.init({
+    dsn: gon.sentry_dsn,
     currentUserId: gon.current_user_id,
     whitelistUrls:
       process.env.NODE_ENV === 'production'
@@ -15,7 +15,7 @@ const index = function index() {
     },
   });
-  return RavenConfig;
+  return SentryConfig;
 };
 index();

app/assets/javascripts/raven/raven_config.js → app/assets/javascripts/sentry/sentry_config.js

-import Raven from 'raven-js';
+import * as Sentry from '@sentry/browser';
 import $ from 'jquery';
 import { __ } from '~/locale';
@@ -26,7 +26,7 @@ const IGNORE_ERRORS = [
   'conduitPage',
 ];
-const IGNORE_URLS = [
+const BLACKLIST_URLS = [
   // Facebook flakiness
   /graph\.facebook\.com/i,
   // Facebook blocked
@@ -43,62 +43,62 @@ const IGNORE_URLS = [
   /metrics\.itunes\.apple\.com\.edgesuite\.net\//i,
 ];
-const SAMPLE_RATE = 95;
-const RavenConfig = {
+const SAMPLE_RATE = 0.95;
+const SentryConfig = {
   IGNORE_ERRORS,
-  IGNORE_URLS,
+  BLACKLIST_URLS,
   SAMPLE_RATE,
   init(options = {}) {
     this.options = options;
     this.configure();
-    this.bindRavenErrors();
+    this.bindSentryErrors();
     if (this.options.currentUserId) this.setUser();
   },
   configure() {
-    Raven.config(this.options.sentryDsn, {
-      release: this.options.release,
-      tags: this.options.tags,
-      whitelistUrls: this.options.whitelistUrls,
-      environment: this.options.environment,
-      ignoreErrors: this.IGNORE_ERRORS,
-      ignoreUrls: this.IGNORE_URLS,
-      shouldSendCallback: this.shouldSendSample.bind(this),
-    }).install();
+    const { dsn, release, tags, whitelistUrls, environment } = this.options;
+    Sentry.init({
+      dsn,
+      release,
+      tags,
+      whitelistUrls,
+      environment,
+      ignoreErrors: this.IGNORE_ERRORS, // TODO: Remove in favor of https://gitlab.com/gitlab-org/gitlab/issues/35144
+      blacklistUrls: this.BLACKLIST_URLS,
+      sampleRate: SAMPLE_RATE,
+    });
   },
   setUser() {
-    Raven.setUserContext({
+    Sentry.setUser({
       id: this.options.currentUserId,
     });
   },
-  bindRavenErrors() {
-    $(document).on('ajaxError.raven', this.handleRavenErrors);
+  bindSentryErrors() {
+    $(document).on('ajaxError.sentry', this.handleSentryErrors);
   },
-  handleRavenErrors(event, req, config, err) {
+  handleSentryErrors(event, req, config, err) {
     const error = err || req.statusText;
-    const responseText = req.responseText || __('Unknown response text');
-    Raven.captureMessage(error, {
+    const { responseText = __('Unknown response text') } = req;
+    const { type, url, data } = config;
+    const { status } = req;
+    Sentry.captureMessage(error, {
       extra: {
-        type: config.type,
-        url: config.url,
-        data: config.data,
-        status: req.status,
+        type,
+        url,
+        data,
+        status,
         response: responseText,
         error,
         event,
       },
     });
   },
-
-  shouldSendSample() {
-    return Math.random() * 100 <= this.SAMPLE_RATE;
-  },
 };
-export default RavenConfig;
+export default SentryConfig;

app/models/merge_request.rb

@@ -16,6 +16,9 @@ class MergeRequest < ApplicationRecord
   include ReactiveCaching
   include FromUnion
   include DeprecatedAssignee
+  include ShaAttribute
+
+  sha_attribute :squash_commit_sha
   self.reactive_cache_key = ->(model) { [model.project.id, model.iid] }
   self.reactive_cache_refresh_interval = 10.minutes

app/services/merge_requests/ff_merge_service.rb

@@ -20,6 +20,10 @@ module MergeRequests
     rescue StandardError => e
       raise MergeError, "Something went wrong during merge: #{e.message}"
     ensure
+      if merge_request.squash
+        merge_request.update_column(:squash_commit_sha, merge_request.in_progress_merge_commit_sha)
+      end
+
       merge_request.update(in_progress_merge_commit_sha: nil)
     end
   end

app/services/metrics/dashboard/custom_metric_embed_service.rb

@@ -40,7 +40,7 @@ module Metrics
         # All custom metrics are displayed on the system dashboard.
         # Nil is acceptable as we'll default to the system dashboard.
         def valid_dashboard?(dashboard)
-          dashboard.nil? || SystemDashboardService.system_dashboard?(dashboard)
+          dashboard.nil? || ::Metrics::Dashboard::SystemDashboardService.system_dashboard?(dashboard)
         end
       end

app/views/layouts/_head.html.haml

@@ -57,7 +57,7 @@
     = yield :library_javascripts
   = javascript_include_tag locale_path unless I18n.locale == :en
-  = webpack_bundle_tag "raven" if Gitlab.config.sentry.enabled
+  = webpack_bundle_tag "sentry" if Gitlab.config.sentry.enabled
   - if content_for?(:page_specific_javascripts)
     = yield :page_specific_javascripts

changelogs/unreleased/26138-replace-raven-js-with-sentry-browser.yml

+---
+title: Replace raven-js with @sentry/browser
+merge_request: 17715
+author:
+type: changed

changelogs/unreleased/ak-fix-undefined-value.yml

+---
+title: Fix uninitialized constant SystemDashboardService
+merge_request: 19453
+author:
+type: fixed

config/webpack.config.js

@@ -73,7 +73,7 @@ function generateEntries() {
   const manualEntries = {
     default: defaultEntries,
-    raven: './raven/index.js',
+    sentry: './sentry/index.js',
   };
   return Object.assign(manualEntries, autoEntries);

db/migrate/20191013100213_add_squash_commit_sha_to_merge_requests.rb

+# frozen_string_literal: true
+
+class AddSquashCommitShaToMergeRequests < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def change
+    add_column :merge_requests, :squash_commit_sha, :binary
+  end
+end

db/schema.rb

@@ -2334,6 +2334,7 @@ ActiveRecord::Schema.define(version: 2019_10_29_191901) do
     t.boolean "allow_maintainer_to_push"
     t.integer "state_id", limit: 2, default: 1, null: false
     t.string "rebase_jid"
+    t.binary "squash_commit_sha"
     t.index ["assignee_id"], name: "index_merge_requests_on_assignee_id"
     t.index ["author_id"], name: "index_merge_requests_on_author_id"
     t.index ["created_at"], name: "index_merge_requests_on_created_at"

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,

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,

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,

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,

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.

doc/development/merge_request_performance_guidelines.md

 # 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).

lib/api/entities.rb

@@ -755,6 +755,7 @@ module API
       end
       expose :diff_head_sha, as: :sha
       expose :merge_commit_sha
+      expose :squash_commit_sha
       expose :discussion_locked
       expose :should_remove_source_branch?, as: :should_remove_source_branch
       expose :force_remove_source_branch?, as: :force_remove_source_branch

package.json

@@ -40,6 +40,7 @@
     "@gitlab/svgs": "^1.80.0",
     "@gitlab/ui": "7.3.0",
     "@gitlab/visual-review-tools": "1.0.3",
+    "@sentry/browser": "^5.7.1",
     "apollo-cache-inmemory": "^1.5.1",
     "apollo-client": "^2.6.4",
     "apollo-link": "^1.2.11",
@@ -109,7 +110,6 @@
     "prosemirror-markdown": "^1.3.0",
     "prosemirror-model": "^1.6.4",
     "raphael": "^2.2.7",
-    "raven-js": "^3.22.1",
     "raw-loader": "^3.1.0",
     "sanitize-html": "^1.16.1",
     "select2": "3.5.2-browserify",

spec/features/raven_js_spec.rb → spec/features/sentry_js_spec.rb

@@ -2,26 +2,27 @@
 require 'spec_helper'
-describe 'RavenJS' do
-  let(:raven_path) { '/raven.chunk.js' }
-  it 'does not load raven if sentry is disabled' do
+describe 'Sentry' do
+  let(:sentry_path) { '/sentry.chunk.js' }
+  it 'does not load sentry if sentry is disabled' do
+    allow(Gitlab.config.sentry).to receive(:enabled).and_return(false)
     visit new_user_session_path
-    expect(has_requested_raven).to eq(false)
+    expect(has_requested_sentry).to eq(false)
   end
-  it 'loads raven if sentry is enabled' do
+  it 'loads sentry if sentry is enabled' do
     stub_sentry_settings
     visit new_user_session_path
-    expect(has_requested_raven).to eq(true)
+    expect(has_requested_sentry).to eq(true)
   end
-  def has_requested_raven
+  def has_requested_sentry
     page.all('script', visible: false).one? do |elm|
-      elm[:src] =~ /#{raven_path}$/
+      elm[:src] =~ /#{sentry_path}$/
     end
   end
 end