diff options
32 files changed, 404 insertions, 93 deletions
diff --git a/CHANGELOG b/CHANGELOG index 0997c15cfdc..c25ee3c71d3 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -13,6 +13,7 @@ v 8.7.0 (unreleased) - Allow back dating on issues when created through the API - Fix Error 500 after renaming a project path (Stan Hu) - Fix avatar stretching by providing a cropping feature + - API: Expose `subscribed` for issues and merge requests (Robert Schilling) - Allow SAML to handle external users based on user's information !3530 - Add endpoints to archive or unarchive a project !3372 - Add links to CI setup documentation from project settings and builds pages diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 511336f384c..1f26a5d7eaf 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -448,7 +448,7 @@ merge request: - multi-line method chaining style **Option B**: dot `.` on previous line - string literal quoting style **Option A**: single quoted by default 1. [Rails](https://github.com/bbatsov/rails-style-guide) -1. [Testing](https://github.com/thoughtbot/guides/tree/master/style/testing) +1. [Testing](doc/development/testing.md) 1. [CoffeeScript](https://github.com/thoughtbot/guides/tree/master/style/coffeescript) 1. [SCSS styleguide][scss-styleguide] 1. [Shell commands](doc/development/shell_commands.md) created by GitLab diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION index 24ba9a38de6..37c2961c243 100644 --- a/GITLAB_SHELL_VERSION +++ b/GITLAB_SHELL_VERSION @@ -1 +1 @@ -2.7.0 +2.7.2 diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index 39e898a4f95..7486fdbc50b 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -0.7.1 +0.7.2 @@ -149,6 +149,10 @@ gem 'version_sorter', '~> 2.0.0' # Cache gem "redis-rails", '~> 4.0.0' +# Redis +gem 'redis', '~> 3.2' +gem 'connection_pool', '~> 2.0' + # Campfire integration gem 'tinder', '~> 1.10.0' @@ -229,7 +233,6 @@ group :metrics do gem 'allocations', '~> 1.0', require: false, platform: :mri gem 'method_source', '~> 0.8', require: false gem 'influxdb', '~> 0.2', require: false - gem 'connection_pool', '~> 2.0', require: false end group :development do diff --git a/Gemfile.lock b/Gemfile.lock index 1ba8d748db1..a30706a4318 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -999,6 +999,7 @@ DEPENDENCIES rdoc (~> 3.6) recaptcha redcarpet (~> 3.3.3) + redis (~> 3.2) redis-namespace redis-rails (~> 4.0.0) request_store (~> 1.3.0) diff --git a/app/assets/javascripts/todos.js.coffee b/app/assets/javascripts/todos.js.coffee index ec2df6c5b73..886da72e261 100644 --- a/app/assets/javascripts/todos.js.coffee +++ b/app/assets/javascripts/todos.js.coffee @@ -57,5 +57,10 @@ class @Todos $('.todos-pending .badge, .todos-pending-count').text data.count $('.todos-done .badge').text data.done_count - goToTodoUrl: -> - Turbolinks.visit($(this).data('url')) + goToTodoUrl: (e)-> + todoLink = $(this).data('url') + if e.metaKey + e.preventDefault() + window.open(todoLink,'_blank') + else + Turbolinks.visit(todoLink) diff --git a/app/views/users/calendar.html.haml b/app/views/users/calendar.html.haml index 7f29918dba3..1de71f37d1a 100644 --- a/app/views/users/calendar.html.haml +++ b/app/views/users/calendar.html.haml @@ -7,4 +7,4 @@ '#{user_calendar_activities_path}' ); -.calendar-hint Summary of issues, merge requests and push events +.calendar-hint Summary of issues, merge requests, and push events diff --git a/config/application.rb b/config/application.rb index 5a0ac70aa2a..2e2ed48db07 100644 --- a/config/application.rb +++ b/config/application.rb @@ -4,11 +4,9 @@ require 'rails/all' require 'devise' I18n.config.enforce_available_locales = false Bundler.require(:default, Rails.env) -require_relative '../lib/gitlab/redis_config' +require_relative '../lib/gitlab/redis' module Gitlab - REDIS_CACHE_NAMESPACE = 'cache:gitlab' - class Application < Rails::Application # Settings in config/environments/* take precedence over those specified here. # Application configuration should go into files in config/initializers @@ -69,8 +67,8 @@ module Gitlab end end - redis_config_hash = Gitlab::RedisConfig.redis_store_options - redis_config_hash[:namespace] = REDIS_CACHE_NAMESPACE + redis_config_hash = Gitlab::Redis.redis_store_options + redis_config_hash[:namespace] = Gitlab::Redis::CACHE_NAMESPACE redis_config_hash[:expires_in] = 2.weeks # Cache should not grow forever config.cache_store = :redis_store, redis_config_hash diff --git a/config/initializers/metrics.rb b/config/initializers/metrics.rb index 3e1deb8d306..a9fc38fb04a 100644 --- a/config/initializers/metrics.rb +++ b/config/initializers/metrics.rb @@ -7,6 +7,7 @@ if Gitlab::Metrics.enabled? # ActiveSupport. require 'gitlab/metrics/subscribers/action_view' require 'gitlab/metrics/subscribers/active_record' + require 'gitlab/metrics/subscribers/rails_cache' Gitlab::Application.configure do |config| config.middleware.use(Gitlab::Metrics::RackMiddleware) diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb index 3da5d46be92..70285255877 100644 --- a/config/initializers/session_store.rb +++ b/config/initializers/session_store.rb @@ -13,7 +13,7 @@ end if Rails.env.test? Gitlab::Application.config.session_store :cookie_store, key: "_gitlab_session" else - redis_config = Gitlab::RedisConfig.redis_store_options + redis_config = Gitlab::Redis.redis_store_options redis_config[:namespace] = 'session:gitlab' Gitlab::Application.config.session_store( diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb index cc83137745a..9182d929809 100644 --- a/config/initializers/sidekiq.rb +++ b/config/initializers/sidekiq.rb @@ -2,7 +2,7 @@ SIDEKIQ_REDIS_NAMESPACE = 'resque:gitlab' Sidekiq.configure_server do |config| config.redis = { - url: Gitlab::RedisConfig.url, + url: Gitlab::Redis.url, namespace: SIDEKIQ_REDIS_NAMESPACE } @@ -29,7 +29,7 @@ end Sidekiq.configure_client do |config| config.redis = { - url: Gitlab::RedisConfig.url, + url: Gitlab::Redis.url, namespace: SIDEKIQ_REDIS_NAMESPACE } end diff --git a/config/mail_room.yml b/config/mail_room.yml index 60257329f3e..761a32adb9e 100644 --- a/config/mail_room.yml +++ b/config/mail_room.yml @@ -2,7 +2,7 @@ <% require "yaml" require "json" -require_relative "lib/gitlab/redis_config" +require_relative "lib/gitlab/redis" rails_env = ENV["RAILS_ENV"] || ENV["RACK_ENV"] || "development" @@ -18,7 +18,7 @@ if File.exists?(config_file) config['mailbox'] = "inbox" if config['mailbox'].nil? if config['enabled'] && config['address'] - redis_url = Gitlab::RedisConfig.new(rails_env).url + redis_url = Gitlab::Redis.new(rails_env).url %> - :host: <%= config['host'].to_json %> diff --git a/doc/api/issues.md b/doc/api/issues.md index cc6355d34ef..1c635a6cdcf 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -76,8 +76,9 @@ Example response: "title" : "Consequatur vero maxime deserunt laboriosam est voluptas dolorem.", "created_at" : "2016-01-04T15:31:51.081Z", "iid" : 6, - "labels" : [] - }, + "labels" : [], + "subscribed" : false + } ] ``` @@ -152,7 +153,8 @@ Example response: "id" : 41, "title" : "Ut commodi ullam eos dolores perferendis nihil sunt.", "updated_at" : "2016-01-04T15:31:46.176Z", - "created_at" : "2016-01-04T15:31:46.176Z" + "created_at" : "2016-01-04T15:31:46.176Z", + "subscribed" : false } ] ``` @@ -213,7 +215,8 @@ Example response: "id" : 41, "title" : "Ut commodi ullam eos dolores perferendis nihil sunt.", "updated_at" : "2016-01-04T15:31:46.176Z", - "created_at" : "2016-01-04T15:31:46.176Z" + "created_at" : "2016-01-04T15:31:46.176Z", + "subscribed": false } ``` @@ -267,7 +270,8 @@ Example response: }, "description" : null, "updated_at" : "2016-01-07T12:44:33.959Z", - "milestone" : null + "milestone" : null, + "subscribed" : true } ``` @@ -323,7 +327,8 @@ Example response: ], "id" : 85, "assignee" : null, - "milestone" : null + "milestone" : null, + "subscribed" : true } ``` diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index b20a6300b7a..20db73ea6c0 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -66,7 +66,8 @@ Parameters: "due_date": null }, "merge_when_build_succeeds": true, - "merge_status": "can_be_merged" + "merge_status": "can_be_merged", + "subscribed" : false } ] ``` @@ -128,7 +129,8 @@ Parameters: "due_date": null }, "merge_when_build_succeeds": true, - "merge_status": "can_be_merged" + "merge_status": "can_be_merged", + "subscribed" : true } ``` @@ -227,6 +229,7 @@ Parameters: }, "merge_when_build_succeeds": true, "merge_status": "can_be_merged", + "subscribed" : true, "changes": [ { "old_path": "VERSION", @@ -304,7 +307,8 @@ Parameters: "due_date": null }, "merge_when_build_succeeds": true, - "merge_status": "can_be_merged" + "merge_status": "can_be_merged", + "subscribed" : true } ``` @@ -373,7 +377,8 @@ Parameters: "due_date": null }, "merge_when_build_succeeds": true, - "merge_status": "can_be_merged" + "merge_status": "can_be_merged", + "subscribed" : true } ``` @@ -466,7 +471,8 @@ Parameters: "due_date": null }, "merge_when_build_succeeds": true, - "merge_status": "can_be_merged" + "merge_status": "can_be_merged", + "subscribed" : true } ``` @@ -530,7 +536,8 @@ Parameters: "due_date": null }, "merge_when_build_succeeds": true, - "merge_status": "can_be_merged" + "merge_status": "can_be_merged", + "subscribed" : true } ``` diff --git a/doc/ci/ssh_keys/README.md b/doc/ci/ssh_keys/README.md index 210f9c3e849..d790015aca1 100644 --- a/doc/ci/ssh_keys/README.md +++ b/doc/ci/ssh_keys/README.md @@ -57,7 +57,7 @@ before_script: # WARNING: Use this only with the Docker executor, if you use it with shell # you will overwrite your user's SSH config. - mkdir -p ~/.ssh - - '[[ -f /.dockerinit ]] && echo -e "Host *\n\tStrictHostKeyChecking no\n\n" > ~/.ssh/config` + - '[[ -f /.dockerinit ]] && echo -e "Host *\n\tStrictHostKeyChecking no\n\n" > ~/.ssh/config' ``` As a final step, add the _public_ key from the one you created earlier to the diff --git a/doc/development/README.md b/doc/development/README.md index 8940b558fb6..2f4e7845ccc 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -10,4 +10,5 @@ - [Shell commands](shell_commands.md) in the GitLab codebase - [Sidekiq debugging](sidekiq_debugging.md) - [SQL guidelines](sql.md) for SQL guidelines +- [Testing standards and style guidelines](testing.md) - [UI guide](ui_guide.md) for building GitLab with existing css styles and elements diff --git a/doc/development/testing.md b/doc/development/testing.md new file mode 100644 index 00000000000..23417845f16 --- /dev/null +++ b/doc/development/testing.md @@ -0,0 +1,134 @@ +# Testing Standards and Style Guidelines + +This guide outlines standards and best practices for automated testing of GitLab +CE and EE. + +It is meant to be an _extension_ of the [thoughtbot testing +styleguide](https://github.com/thoughtbot/guides/tree/master/style/testing). If +this guide defines a rule that contradicts the thoughtbot guide, this guide +takes precedence. Some guidelines may be repeated verbatim to stress their +importance. + +## Factories + +GitLab uses [factory_girl] as a test fixture replacement. + +- Factory definitions live in `spec/factories/`, named using the pluralization + of their corresponding model (`User` factories are defined in `users.rb`). +- There should be only one top-level factory definition per file. +- Make use of [Traits] to clean up definitions and usages. +- When defining a factory, don't define attributes that are not required for the + resulting record to pass validation. +- When instantiating from a factory, don't supply extraneous attributes that + aren't required by the test. +- Factories don't have to be limited to `ActiveRecord` objects. + [See example](https://gitlab.com/gitlab-org/gitlab-ce/commit/0b8cefd3b2385a21cfed779bd659978c0402766d). + +[factory_girl]: https://github.com/thoughtbot/factory_girl +[Traits]: http://www.rubydoc.info/gems/factory_girl/file/GETTING_STARTED.md#Traits + +## JavaScript + +GitLab uses [Teaspoon] to run its [Jasmine] JavaScript specs. They can be run on +the command line via `bundle exec teaspoon`, or via a web browser at +`http://localhost:3000/teaspoon` when the Rails server is running. + +- JavaScript tests live in `spec/javascripts/`, matching the folder structure of + `app/assets/javascripts/`: `app/assets/javascripts/behaviors/autosize.js.coffee` has a corresponding + `spec/javascripts/behaviors/autosize_spec.js.coffee` file. +- Haml fixtures required for JavaScript tests live in + `spec/javascripts/fixtures`. They should contain the bare minimum amount of + markup necessary for the test. + + > **Warning:** Keep in mind that a Rails view may change and + invalidate your test, but everything will still pass because your fixture + doesn't reflect the latest view. + +- Keep in mind that in a CI environment, these tests are run in a headless + browser and you will not have access to certain APIs, such as + [`Notification`](https://developer.mozilla.org/en-US/docs/Web/API/notification), + which will have to be stubbed. + +[Teaspoon]: https://github.com/modeset/teaspoon +[Jasmine]: https://github.com/jasmine/jasmine + +## RSpec + +### General Guidelines + +- Use a single, top-level `describe ClassName` block. +- Use `described_class` instead of repeating the class name being described. +- Use `.method` to describe class methods and `#method` to describe instance + methods. +- Use `context` to test branching logic. +- Don't `describe` symbols (see [Gotchas](gotchas.md#dont-describe-symbols)). +- Prefer `not_to` to `to_not`. +- Try to match the ordering of tests to the ordering within the class. +- Try to follow the [Four-Phase Test][four-phase-test] pattern, using newlines + to separate phases. + +[four-phase-test]: https://robots.thoughtbot.com/four-phase-test + +### `let` variables + +GitLab's RSpec suite has made extensive use of `let` variables to reduce +duplication. However, this sometimes [comes at the cost of clarity][lets-not], +so we need to set some guidelines for their use going forward: + +- `let` variables are preferable to instance variables. Local variables are + preferable to `let` variables. +- Use `let` to reduce duplication throughout an entire spec file. +- Don't use `let` to define variables used by a single test; define them as + local variables inside the test's `it` block. +- Don't define a `let` variable inside the top-level `describe` block that's + only used in a more deeply-nested `context` or `describe` block. Keep the + definition as close as possible to where it's used. +- Try to avoid overriding the definition of one `let` variable with another. +- Don't define a `let` variable that's only used by the definition of another. + Use a helper method instead. + +[lets-not]: https://robots.thoughtbot.com/lets-not + +### Test speed + +GitLab has a massive test suite that, without parallelization, can take more +than an hour to run. It's important that we make an effort to write tests that +are accurate and effective _as well as_ fast. + +Here are some things to keep in mind regarding test performance: + +- `double` and `spy` are faster than `FactoryGirl.build(...)` +- `FactoryGirl.build(...)` and `.build_stubbed` are faster than `.create`. +- Don't `create` an object when `build`, `build_stubbed`, `attributes_for`, + `spy`, or `double` will do. Database persistence is slow! +- Use `create(:empty_project)` instead of `create(:project)` when you don't need + the underlying repository. Filesystem operations are slow! +- Don't mark a feature as requiring JavaScript (through `@javascript` in + Spinach or `js: true` in RSpec) unless it's _actually_ required for the test + to be valid. Headless browser testing is slow! + +### Features / Integration + +- Feature specs live in `spec/features/` and should be named + `ROLE_ACTION_spec.rb`, such as `user_changes_password_spec.rb`. +- Use only one `feature` block per feature spec file. +- Use scenario titles that describe the success and failure paths. +- Avoid scenario titles that add no information, such as "successfully." +- Avoid scenario titles that repeat the feature title. + +## Spinach (feature) tests + +GitLab [moved from Cucumber to Spinach](https://github.com/gitlabhq/gitlabhq/pull/1426) +for its feature/integration tests in September 2012. + +As of March 2016, we are [trying to avoid adding new Spinach +tests](https://gitlab.com/gitlab-org/gitlab-ce/issues/14121) going forward, +opting for [RSpec feature](#features-integration) specs. + +Adding new Spinach scenarios is acceptable _only if_ the new scenario requires +no more than one new `step` definition. If more than that is required, the +test should be re-implemented using RSpec instead. + +--- + +[Return to Development documentation](README.md) diff --git a/doc/development/ui_guide.md b/doc/development/ui_guide.md index 2f01defc11d..a3e260a5f89 100644 --- a/doc/development/ui_guide.md +++ b/doc/development/ui_guide.md @@ -1,9 +1,5 @@ # UI Guide for building GitLab -## Best practices for creating new pages in GitLab - -TODO: write some best practices when develop GitLab features. - ## GitLab UI development kit We created a page inside GitLab where you can check commonly used html and css elements. diff --git a/doc/install/installation.md b/doc/install/installation.md index f8f7d6a9ebe..d97dc7d1311 100644 --- a/doc/install/installation.md +++ b/doc/install/installation.md @@ -352,7 +352,7 @@ GitLab Shell is an SSH access and repository management software developed speci cd /home/git sudo -u git -H git clone https://gitlab.com/gitlab-org/gitlab-workhorse.git cd gitlab-workhorse - sudo -u git -H git checkout v0.7.1 + sudo -u git -H git checkout v0.7.2 sudo -u git -H make ### Initialize Database and Activate Advanced Features diff --git a/doc/update/8.6-to-8.7.md b/doc/update/8.6-to-8.7.md index 8599133a726..57847d2d9fd 100644 --- a/doc/update/8.6-to-8.7.md +++ b/doc/update/8.6-to-8.7.md @@ -58,7 +58,7 @@ GitLab 8.1. ```bash cd /home/git/gitlab-workhorse sudo -u git -H git fetch --all -sudo -u git -H git checkout v0.7.1 +sudo -u git -H git checkout v0.7.2 sudo -u git -H make ``` diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 4c49442bf8b..d76b46b8836 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -170,6 +170,10 @@ module API expose :label_names, as: :labels expose :milestone, using: Entities::Milestone expose :assignee, :author, using: Entities::UserBasic + + expose :subscribed do |issue, options| + issue.subscribed?(options[:current_user]) + end end class MergeRequest < ProjectEntity @@ -183,6 +187,10 @@ module API expose :milestone, using: Entities::Milestone expose :merge_when_build_succeeds expose :merge_status + + expose :subscribed do |merge_request, options| + merge_request.subscribed?(options[:current_user]) + end end class MergeRequestChanges < MergeRequest diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 1fee1dee1a6..c4ea05ee6cf 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -55,7 +55,7 @@ module API issues = filter_issues_state(issues, params[:state]) unless params[:state].nil? issues = filter_issues_labels(issues, params[:labels]) unless params[:labels].nil? issues.reorder(issuable_order_by => issuable_sort) - present paginate(issues), with: Entities::Issue + present paginate(issues), with: Entities::Issue, current_user: current_user end end @@ -92,7 +92,7 @@ module API end issues.reorder(issuable_order_by => issuable_sort) - present paginate(issues), with: Entities::Issue + present paginate(issues), with: Entities::Issue, current_user: current_user end # Get a single project issue @@ -105,7 +105,7 @@ module API get ":id/issues/:issue_id" do @issue = user_project.issues.find(params[:issue_id]) not_found! unless can?(current_user, :read_issue, @issue) - present @issue, with: Entities::Issue + present @issue, with: Entities::Issue, current_user: current_user end # Create a new project issue @@ -149,7 +149,7 @@ module API issue.add_labels_by_names(params[:labels].split(',')) end - present issue, with: Entities::Issue + present issue, with: Entities::Issue, current_user: current_user else render_validation_error!(issue) end @@ -189,7 +189,7 @@ module API issue.add_labels_by_names(params[:labels].split(',')) end - present issue, with: Entities::Issue + present issue, with: Entities::Issue, current_user: current_user else render_validation_error!(issue) end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 93052fba06b..4e7de8867b4 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -56,7 +56,7 @@ module API end merge_requests = merge_requests.reorder(issuable_order_by => issuable_sort) - present paginate(merge_requests), with: Entities::MergeRequest + present paginate(merge_requests), with: Entities::MergeRequest, current_user: current_user end # Create MR @@ -94,7 +94,7 @@ module API merge_request.add_labels_by_names(params[:labels].split(",")) end - present merge_request, with: Entities::MergeRequest + present merge_request, with: Entities::MergeRequest, current_user: current_user else handle_merge_request_errors! merge_request.errors end @@ -130,7 +130,7 @@ module API authorize! :read_merge_request, merge_request - present merge_request, with: Entities::MergeRequest + present merge_request, with: Entities::MergeRequest, current_user: current_user end # Show MR commits @@ -162,7 +162,7 @@ module API merge_request = user_project.merge_requests. find(params[:merge_request_id]) authorize! :read_merge_request, merge_request - present merge_request, with: Entities::MergeRequestChanges + present merge_request, with: Entities::MergeRequestChanges, current_user: current_user end # Update MR @@ -204,7 +204,7 @@ module API merge_request.add_labels_by_names(params[:labels].split(",")) end - present merge_request, with: Entities::MergeRequest + present merge_request, with: Entities::MergeRequest, current_user: current_user else handle_merge_request_errors! merge_request.errors end @@ -246,7 +246,7 @@ module API execute(merge_request) end - present merge_request, with: Entities::MergeRequest + present merge_request, with: Entities::MergeRequest, current_user: current_user end # Cancel Merge if Merge When build succeeds is enabled @@ -325,7 +325,7 @@ module API get "#{path}/closes_issues" do merge_request = user_project.merge_requests.find(params[:merge_request_id]) issues = ::Kaminari.paginate_array(merge_request.closes_issues(current_user)) - present paginate(issues), with: Entities::Issue + present paginate(issues), with: Entities::Issue, current_user: current_user end end end diff --git a/lib/api/milestones.rb b/lib/api/milestones.rb index afb6ffa3609..0f3f505fa05 100644 --- a/lib/api/milestones.rb +++ b/lib/api/milestones.rb @@ -103,7 +103,7 @@ module API authorize! :read_milestone, user_project @milestone = user_project.milestones.find(params[:milestone_id]) - present paginate(@milestone.issues), with: Entities::Issue + present paginate(@milestone.issues), with: Entities::Issue, current_user: current_user end end diff --git a/lib/gitlab/exclusive_lease.rb b/lib/gitlab/exclusive_lease.rb index c73eca832d7..c2260a5f7ac 100644 --- a/lib/gitlab/exclusive_lease.rb +++ b/lib/gitlab/exclusive_lease.rb @@ -43,7 +43,9 @@ module Gitlab # false if the lease is already taken. def try_obtain # Performing a single SET is atomic - !!redis.set(redis_key, '1', nx: true, ex: @timeout) + Gitlab::Redis.with do |redis| + !!redis.set(redis_key, '1', nx: true, ex: @timeout) + end end # No #cancel method. See comments above! diff --git a/lib/gitlab/metrics/metric.rb b/lib/gitlab/metrics/metric.rb index 7ea9555cc8c..1cd1ca30f70 100644 --- a/lib/gitlab/metrics/metric.rb +++ b/lib/gitlab/metrics/metric.rb @@ -2,6 +2,8 @@ module Gitlab module Metrics # Class for storing details of a single metric (label, value, etc). class Metric + JITTER_RANGE = 0.000001..0.001 + attr_reader :series, :values, :tags, :created_at # series - The name of the series (as a String) to store the metric in. @@ -16,11 +18,29 @@ module Gitlab # Returns a Hash in a format that can be directly written to InfluxDB. def to_hash + # InfluxDB overwrites an existing point if a new point has the same + # series, tag set, and timestamp. In a highly concurrent environment + # this means that using the number of seconds since the Unix epoch is + # inevitably going to collide with another timestamp. For example, two + # Rails requests processed by different processes may end up generating + # metrics using the _exact_ same timestamp (in seconds). + # + # Due to the way InfluxDB is set up there's no solution to this problem, + # all we can do is lower the amount of collisions. We do this by using + # Time#to_f which returns the seconds as a Float providing greater + # accuracy. We then add a small random value that is large enough to + # distinguish most timestamps but small enough to not alter the amount + # of seconds. + # + # See https://gitlab.com/gitlab-com/operations/issues/175 for more + # information. + time = @created_at.to_f + rand(JITTER_RANGE) + { series: @series, tags: @tags, values: @values, - timestamp: @created_at.to_i * 1_000_000_000 + timestamp: (time * 1_000_000_000).to_i } end end diff --git a/lib/gitlab/metrics/subscribers/rails_cache.rb b/lib/gitlab/metrics/subscribers/rails_cache.rb new file mode 100644 index 00000000000..49e5f86e6e6 --- /dev/null +++ b/lib/gitlab/metrics/subscribers/rails_cache.rb @@ -0,0 +1,39 @@ +module Gitlab + module Metrics + module Subscribers + # Class for tracking the total time spent in Rails cache calls + class RailsCache < ActiveSupport::Subscriber + attach_to :active_support + + def cache_read(event) + increment(:cache_read_duration, event.duration) + end + + def cache_write(event) + increment(:cache_write_duration, event.duration) + end + + def cache_delete(event) + increment(:cache_delete_duration, event.duration) + end + + def cache_exist?(event) + increment(:cache_exists_duration, event.duration) + end + + def increment(key, duration) + return unless current_transaction + + current_transaction.increment(:cache_duration, duration) + current_transaction.increment(key, duration) + end + + private + + def current_transaction + Transaction.current + end + end + end + end +end diff --git a/lib/gitlab/redis.rb b/lib/gitlab/redis.rb new file mode 100644 index 00000000000..319447669dc --- /dev/null +++ b/lib/gitlab/redis.rb @@ -0,0 +1,48 @@ +module Gitlab + class Redis + CACHE_NAMESPACE = 'cache:gitlab' + + attr_reader :url + + # To be thread-safe we must be careful when writing the class instance + # variables @url and @pool. Because @pool depends on @url we need two + # mutexes to prevent deadlock. + URL_MUTEX = Mutex.new + POOL_MUTEX = Mutex.new + private_constant :URL_MUTEX, :POOL_MUTEX + + def self.url + @url || URL_MUTEX.synchronize { @url = new.url } + end + + def self.with + if @pool.nil? + POOL_MUTEX.synchronize do + @pool = ConnectionPool.new { ::Redis.new(url: url) } + end + end + @pool.with { |redis| yield redis } + end + + def self.redis_store_options + url = new.url + redis_config_hash = ::Redis::Store::Factory.extract_host_options_from_uri(url) + # Redis::Store does not handle Unix sockets well, so let's do it for them + redis_uri = URI.parse(url) + if redis_uri.scheme == 'unix' + redis_config_hash[:path] = redis_uri.path + end + redis_config_hash + end + + def initialize(rails_env=nil) + rails_env ||= Rails.env + config_file = File.expand_path('../../../config/resque.yml', __FILE__) + + @url = "redis://localhost:6379" + if File.exists?(config_file) + @url =YAML.load_file(config_file)[rails_env] + end + end + end +end diff --git a/lib/gitlab/redis_config.rb b/lib/gitlab/redis_config.rb deleted file mode 100644 index 4949c6db539..00000000000 --- a/lib/gitlab/redis_config.rb +++ /dev/null @@ -1,30 +0,0 @@ -module Gitlab - class RedisConfig - attr_reader :url - - def self.url - new.url - end - - def self.redis_store_options - url = new.url - redis_config_hash = Redis::Store::Factory.extract_host_options_from_uri(url) - # Redis::Store does not handle Unix sockets well, so let's do it for them - redis_uri = URI.parse(url) - if redis_uri.scheme == 'unix' - redis_config_hash[:path] = redis_uri.path - end - redis_config_hash - end - - def initialize(rails_env=nil) - rails_env ||= Rails.env - config_file = File.expand_path('../../../config/resque.yml', __FILE__) - - @url = "redis://localhost:6379" - if File.exists?(config_file) - @url =YAML.load_file(config_file)[rails_env] - end - end - end -end diff --git a/lib/tasks/cache.rake b/lib/tasks/cache.rake index 51e746ef923..2214f855200 100644 --- a/lib/tasks/cache.rake +++ b/lib/tasks/cache.rake @@ -4,18 +4,19 @@ namespace :cache do desc "GitLab | Clear redis cache" task :clear => :environment do - redis = Redis.new(url: Gitlab::RedisConfig.url) - cursor = REDIS_SCAN_START_STOP - loop do - cursor, keys = redis.scan( - cursor, - match: "#{Gitlab::REDIS_CACHE_NAMESPACE}*", - count: CLEAR_BATCH_SIZE - ) - - redis.del(*keys) if keys.any? - - break if cursor == REDIS_SCAN_START_STOP + Gitlab::Redis.with do |redis| + cursor = REDIS_SCAN_START_STOP + loop do + cursor, keys = redis.scan( + cursor, + match: "#{Gitlab::Redis::CACHE_NAMESPACE}*", + count: CLEAR_BATCH_SIZE + ) + + redis.del(*keys) if keys.any? + + break if cursor == REDIS_SCAN_START_STOP + end end end end diff --git a/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb b/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb new file mode 100644 index 00000000000..e01b0b4bd21 --- /dev/null +++ b/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb @@ -0,0 +1,71 @@ +require 'spec_helper' + +describe Gitlab::Metrics::Subscribers::RailsCache do + let(:transaction) { Gitlab::Metrics::Transaction.new } + let(:subscriber) { described_class.new } + + let(:event) { double(:event, duration: 15.2) } + + describe '#cache_read' do + it 'increments the cache_read duration' do + expect(subscriber).to receive(:increment). + with(:cache_read_duration, event.duration) + + subscriber.cache_read(event) + end + end + + describe '#cache_write' do + it 'increments the cache_write duration' do + expect(subscriber).to receive(:increment). + with(:cache_write_duration, event.duration) + + subscriber.cache_write(event) + end + end + + describe '#cache_delete' do + it 'increments the cache_delete duration' do + expect(subscriber).to receive(:increment). + with(:cache_delete_duration, event.duration) + + subscriber.cache_delete(event) + end + end + + describe '#cache_exist?' do + it 'increments the cache_exists duration' do + expect(subscriber).to receive(:increment). + with(:cache_exists_duration, event.duration) + + subscriber.cache_exist?(event) + end + end + + describe '#increment' do + context 'without a transaction' do + it 'returns' do + expect(transaction).not_to receive(:increment) + + subscriber.increment(:foo, 15.2) + end + end + + context 'with a transaction' do + before do + allow(subscriber).to receive(:current_transaction). + and_return(transaction) + end + + it 'increments the total and specific cache duration' do + expect(transaction).to receive(:increment). + with(:cache_duration, event.duration) + + expect(transaction).to receive(:increment). + with(:cache_delete_duration, event.duration) + + subscriber.increment(:cache_delete_duration, event.duration) + end + end + end +end |