diff options
-rw-r--r-- | app/assets/javascripts/tracking.js | 1 | ||||
-rw-r--r-- | app/views/admin/application_settings/_pendo.html.haml | 24 | ||||
-rw-r--r-- | app/views/admin/application_settings/integrations.html.haml | 2 | ||||
-rw-r--r-- | app/views/layouts/_head.html.haml | 2 | ||||
-rw-r--r-- | app/views/layouts/_pendo.html.haml | 17 | ||||
-rw-r--r-- | app/views/layouts/_snowplow.html.haml | 2 | ||||
-rw-r--r-- | changelogs/unreleased/sh-clean-duplicate-ci-trigger-request-indexes.yml | 5 | ||||
-rw-r--r-- | db/migrate/20191016072826_replace_ci_trigger_requests_index.rb | 13 | ||||
-rw-r--r-- | doc/development/README.md | 1 | ||||
-rw-r--r-- | doc/development/creating_enums.md | 15 | ||||
-rw-r--r-- | lib/gitlab/tracking.rb | 5 | ||||
-rw-r--r-- | spec/db/schema_spec.rb | 46 | ||||
-rw-r--r-- | spec/frontend/tracking_spec.js | 5 | ||||
-rw-r--r-- | spec/lib/gitlab/tracking_spec.rb | 7 | ||||
-rw-r--r-- | spec/support/matchers/db_schema_matchers.rb | 32 | ||||
-rw-r--r-- | spec/views/layouts/_head.html.haml_spec.rb | 22 |
16 files changed, 119 insertions, 80 deletions
diff --git a/app/assets/javascripts/tracking.js b/app/assets/javascripts/tracking.js index 2566c0fb273..7c0097fbe37 100644 --- a/app/assets/javascripts/tracking.js +++ b/app/assets/javascripts/tracking.js @@ -102,7 +102,6 @@ export function initUserTracking() { window.snowplow('enableActivityTracking', 30, 30); window.snowplow('trackPageView'); // must be after enableActivityTracking - if (opts.userId) window.snowplow('setUserId', opts.userId); if (opts.formTracking) window.snowplow('enableFormTracking'); if (opts.linkClickTracking) window.snowplow('enableLinkClickTracking'); diff --git a/app/views/admin/application_settings/_pendo.html.haml b/app/views/admin/application_settings/_pendo.html.haml deleted file mode 100644 index c165610b332..00000000000 --- a/app/views/admin/application_settings/_pendo.html.haml +++ /dev/null @@ -1,24 +0,0 @@ -- expanded = true if !@application_setting.valid? && @application_setting.errors.any? { |k| k.to_s.start_with?('pendo_') } -%section.settings.as-pendo.no-animate#js-pendo-settings{ class: ('expanded' if expanded) } - .settings-header - %h4 - = _('Pendo') - %button.btn.btn-default.js-settings-toggle{ type: 'button' } - = expanded ? _('Collapse') : _('Expand') - %p - = _('Configure the %{link} integration.').html_safe % { link: link_to('Pendo', 'https://www.pendo.io/', target: '_blank') } - .settings-content - - = form_for @application_setting, url: integrations_admin_application_settings_path(anchor: 'js-pendo-settings'), html: { class: 'fieldset-form' } do |f| - = form_errors(@application_setting) if expanded - - %fieldset - .form-group - .form-check - = f.check_box :pendo_enabled, class: 'form-check-input' - = f.label :pendo_enabled, _('Enable pendo tracking'), class: 'form-check-label' - .form-group - = f.label :pendo_url, _('Pendo endpoint'), class: 'label-light' - = f.text_field :pendo_url, class: 'form-control', placeholder: 'https://cdn.pendo.io/agent/static/your-api-key/pendo.js' - - = f.submit _('Save changes'), class: 'btn btn-success' diff --git a/app/views/admin/application_settings/integrations.html.haml b/app/views/admin/application_settings/integrations.html.haml index 399f3566cef..e1079e5650f 100644 --- a/app/views/admin/application_settings/integrations.html.haml +++ b/app/views/admin/application_settings/integrations.html.haml @@ -29,4 +29,4 @@ = render 'third_party_offers', application_setting: @application_setting = render 'admin/application_settings/snowplow', expanded: expanded_by_default? -= render 'admin/application_settings/pendo' += render_if_exists 'admin/application_settings/pendo' diff --git a/app/views/layouts/_head.html.haml b/app/views/layouts/_head.html.haml index d7319b18092..6aebe42b74d 100644 --- a/app/views/layouts/_head.html.haml +++ b/app/views/layouts/_head.html.haml @@ -90,4 +90,4 @@ = render 'layouts/google_analytics' if extra_config.has_key?('google_analytics_id') = render 'layouts/piwik' if extra_config.has_key?('piwik_url') && extra_config.has_key?('piwik_site_id') = render 'layouts/snowplow' - = render 'layouts/pendo' + = render_if_exists 'layouts/pendo' diff --git a/app/views/layouts/_pendo.html.haml b/app/views/layouts/_pendo.html.haml deleted file mode 100644 index 08b1acbe3d3..00000000000 --- a/app/views/layouts/_pendo.html.haml +++ /dev/null @@ -1,17 +0,0 @@ -- return unless Gitlab::CurrentSettings.pendo_enabled? - -= javascript_tag nonce: true do - :plain - ;var trackable = !['1', 'yes'].includes(window.doNotTrack || navigator.doNotTrack || navigator.msDoNotTrack); - if (trackable){ - (function(p,e,n,d,o){var v,w,x,y,z;o=p[d]=p[d]||{};o._q=[]; - v=['initialize','identify','updateOptions','pageLoad'];for(w=0,x=v.length;w<x;++w)(function(m){ - o[m]=o[m]||function(){o._q[m===v[0]?'unshift':'push']([m].concat([].slice.call(arguments,0)));};})(v[w]); - y=e.createElement(n);y.async=!0;y.src='#{Gitlab::CurrentSettings.pendo_url}'; - z=e.getElementsByTagName(n)[0];z.parentNode.insertBefore(y,z);})(window,document,'script','pendo'); - - pendo.initialize({ - visitor: { - id: '#{current_user&.id}', - } - });}; diff --git a/app/views/layouts/_snowplow.html.haml b/app/views/layouts/_snowplow.html.haml index 3c06bca0f95..d7ff5ad1094 100644 --- a/app/views/layouts/_snowplow.html.haml +++ b/app/views/layouts/_snowplow.html.haml @@ -7,4 +7,4 @@ };p[i].q=p[i].q||[];n=l.createElement(o);g=l.getElementsByTagName(o)[0];n.async=1; n.src=w;g.parentNode.insertBefore(n,g)}}(window,document,"script","#{asset_url('snowplow/sp.js')}","snowplow")); - window.snowplowOptions = #{Gitlab::Tracking.snowplow_options(@group, current_user).to_json} + window.snowplowOptions = #{Gitlab::Tracking.snowplow_options(@group).to_json} diff --git a/changelogs/unreleased/sh-clean-duplicate-ci-trigger-request-indexes.yml b/changelogs/unreleased/sh-clean-duplicate-ci-trigger-request-indexes.yml new file mode 100644 index 00000000000..1c86e325558 --- /dev/null +++ b/changelogs/unreleased/sh-clean-duplicate-ci-trigger-request-indexes.yml @@ -0,0 +1,5 @@ +--- +title: Clean up duplicate indexes on ci_trigger_requests +merge_request: 19053 +author: +type: fixed diff --git a/db/migrate/20191016072826_replace_ci_trigger_requests_index.rb b/db/migrate/20191016072826_replace_ci_trigger_requests_index.rb index d7c9524806e..8faa6c3847f 100644 --- a/db/migrate/20191016072826_replace_ci_trigger_requests_index.rb +++ b/db/migrate/20191016072826_replace_ci_trigger_requests_index.rb @@ -10,7 +10,12 @@ class ReplaceCiTriggerRequestsIndex < ActiveRecord::Migration[5.2] def up add_concurrent_index :ci_trigger_requests, [:trigger_id, :id], order: { id: :desc } - remove_concurrent_index :ci_trigger_requests, [:trigger_id] + # Some installations have legacy, duplicate indexes on + # ci_trigger_requests.trigger_id. Rails won't drop them without an + # explicit name: https://gitlab.com/gitlab-org/gitlab/issues/34818 + old_index_names.each do |name| + remove_concurrent_index :ci_trigger_requests, [:trigger_id], name: name + end end def down @@ -18,4 +23,10 @@ class ReplaceCiTriggerRequestsIndex < ActiveRecord::Migration[5.2] remove_concurrent_index :ci_trigger_requests, [:trigger_id, :id], order: { id: :desc } end + + private + + def old_index_names + indexes(:ci_trigger_requests).select { |i| i.columns == ['trigger_id'] }.map(&:name) + end end diff --git a/doc/development/README.md b/doc/development/README.md index e3ec460a6fe..66df6f46e86 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -118,6 +118,7 @@ description: 'Learn how to contribute to GitLab.' - [Query Count Limits](query_count_limits.md) - [Database helper modules](database_helpers.md) - [Code comments](code_comments.md) +- [Creating enums](creating_enums.md) ### Case studies diff --git a/doc/development/creating_enums.md b/doc/development/creating_enums.md new file mode 100644 index 00000000000..64385a2ea79 --- /dev/null +++ b/doc/development/creating_enums.md @@ -0,0 +1,15 @@ +# Creating enums + +When creating a new enum, it should use the database type `SMALLINT`. +The `SMALLINT` type size is 2 bytes, which is sufficient for an enum. +This would help to save space in the database. + +To use this type, add `limit: 2` to the migration that creates the column. + +Example: + +```rb +def change + add_column :ci_job_artifacts, :file_format, :integer, limit: 2 +end +``` diff --git a/lib/gitlab/tracking.rb b/lib/gitlab/tracking.rb index d254d64dad2..2470685bc00 100644 --- a/lib/gitlab/tracking.rb +++ b/lib/gitlab/tracking.rb @@ -39,7 +39,7 @@ module Gitlab snowplow.track_self_describing_event(event_json, context, Time.now.to_i) end - def snowplow_options(group, user) + def snowplow_options(group) additional_features = Feature.enabled?(:additional_snowplow_tracking, group) { namespace: SNOWPLOW_NAMESPACE, @@ -47,8 +47,7 @@ module Gitlab cookie_domain: Gitlab::CurrentSettings.snowplow_cookie_domain, app_id: Gitlab::CurrentSettings.snowplow_site_id, form_tracking: additional_features, - link_click_tracking: additional_features, - user_id: user&.id + link_click_tracking: additional_features }.transform_keys! { |key| key.to_s.camelize(:lower).to_sym } end diff --git a/spec/db/schema_spec.rb b/spec/db/schema_spec.rb index 53f4a261092..e4eddb87858 100644 --- a/spec/db/schema_spec.rb +++ b/spec/db/schema_spec.rb @@ -120,9 +120,55 @@ describe 'Database schema' do end end + # These pre-existing enums have limits > 2 bytes + IGNORED_LIMIT_ENUMS = { + 'Analytics::CycleAnalytics::GroupStage' => %w[start_event_identifier end_event_identifier], + 'Analytics::CycleAnalytics::ProjectStage' => %w[start_event_identifier end_event_identifier], + 'Ci::Bridge' => %w[failure_reason], + 'Ci::Build' => %w[failure_reason], + 'Ci::BuildMetadata' => %w[timeout_source], + 'Ci::BuildTraceChunk' => %w[data_store], + 'Ci::JobArtifact' => %w[file_type], + 'Ci::Pipeline' => %w[source config_source failure_reason], + 'Ci::Runner' => %w[access_level], + 'Ci::Stage' => %w[status], + 'Clusters::Applications::Ingress' => %w[ingress_type], + 'Clusters::Cluster' => %w[platform_type provider_type], + 'CommitStatus' => %w[failure_reason], + 'GenericCommitStatus' => %w[failure_reason], + 'Gitlab::DatabaseImporters::CommonMetrics::PrometheusMetric' => %w[group], + 'InternalId' => %w[usage], + 'List' => %w[list_type], + 'NotificationSetting' => %w[level], + 'Project' => %w[auto_cancel_pending_pipelines], + 'ProjectAutoDevops' => %w[deploy_strategy], + 'PrometheusMetric' => %w[group], + 'ResourceLabelEvent' => %w[action], + 'User' => %w[layout dashboard project_view], + 'UserCallout' => %w[feature_name], + 'PrometheusAlert' => %w[operator] + }.freeze + + context 'for enums' do + ApplicationRecord.descendants.each do |model| + describe model do + let(:ignored_enums) { ignored_limit_enums(model.name) } + let(:enums) { model.defined_enums.keys - ignored_enums } + + it 'uses smallint for enums' do + expect(model).to use_smallint_for_enums(enums) + end + end + end + end + private def ignored_fk_columns(column) IGNORED_FK_COLUMNS.fetch(column, []) end + + def ignored_limit_enums(model) + IGNORED_LIMIT_ENUMS.fetch(model, []) + end end diff --git a/spec/frontend/tracking_spec.js b/spec/frontend/tracking_spec.js index d9cc7104139..964f8b8787e 100644 --- a/spec/frontend/tracking_spec.js +++ b/spec/frontend/tracking_spec.js @@ -11,7 +11,6 @@ describe('Tracking', () => { namespace: '_namespace_', hostname: 'app.gitfoo.com', cookieDomain: '.gitfoo.com', - userId: null, }; snowplowSpy = jest.spyOn(window, 'snowplow'); }); @@ -35,7 +34,6 @@ describe('Tracking', () => { contexts: { webPage: true }, formTracking: false, linkClickTracking: false, - userId: null, }); }); @@ -43,18 +41,15 @@ describe('Tracking', () => { initUserTracking(); expect(snowplowSpy).toHaveBeenCalledWith('enableActivityTracking', 30, 30); expect(snowplowSpy).toHaveBeenCalledWith('trackPageView'); - expect(snowplowSpy).not.toHaveBeenCalledWith('setUserId'); expect(snowplowSpy).not.toHaveBeenCalledWith('enableFormTracking'); expect(snowplowSpy).not.toHaveBeenCalledWith('enableLinkClickTracking'); window.snowplowOptions = Object.assign({}, window.snowplowOptions, { formTracking: true, linkClickTracking: true, - userId: '1', }); initUserTracking(); - expect(snowplowSpy).toHaveBeenCalledWith('setUserId', '1'); expect(snowplowSpy).toHaveBeenCalledWith('enableFormTracking'); expect(snowplowSpy).toHaveBeenCalledWith('enableLinkClickTracking'); }); diff --git a/spec/lib/gitlab/tracking_spec.rb b/spec/lib/gitlab/tracking_spec.rb index 4615599be7e..3ceaf159b3f 100644 --- a/spec/lib/gitlab/tracking_spec.rb +++ b/spec/lib/gitlab/tracking_spec.rb @@ -19,11 +19,10 @@ describe Gitlab::Tracking do cookieDomain: '.gitfoo.com', appId: '_abc123_', formTracking: true, - linkClickTracking: true, - userId: nil + linkClickTracking: true } - expect(subject.snowplow_options(nil, nil)).to match(expected_fields) + expect(subject.snowplow_options(nil)).to match(expected_fields) end it 'enables features using feature flags' do @@ -37,7 +36,7 @@ describe Gitlab::Tracking do linkClickTracking: false } - expect(subject.snowplow_options('_group_', nil)).to include(addition_feature_fields) + expect(subject.snowplow_options('_group_')).to include(addition_feature_fields) end end diff --git a/spec/support/matchers/db_schema_matchers.rb b/spec/support/matchers/db_schema_matchers.rb new file mode 100644 index 00000000000..55843b7bb49 --- /dev/null +++ b/spec/support/matchers/db_schema_matchers.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +EXPECTED_SMALLINT_LIMIT = 2 + +RSpec::Matchers.define :use_smallint_for_enums do |enums| + match do |actual| + @failing_enums = enums.select do |enum| + enum_type = actual.type_for_attribute(enum) + actual_limit = enum_type.send(:subtype).limit + actual_limit != EXPECTED_SMALLINT_LIMIT + end + @failing_enums.empty? + end + + failure_message do + <<~FAILURE_MESSAGE + Expected #{actual.name} enums: #{failing_enums.join(', ')} to use the smallint type. + + The smallint type is 2 bytes which is more than sufficient for an enum. + Using the smallint type would help us save space in the database. + To fix this, please add `limit: 2` in the migration file, for example: + + def change + add_column :ci_job_artifacts, :file_format, :integer, limit: 2 + end + FAILURE_MESSAGE + end + + def failing_enums + @failing_enums ||= [] + end +end diff --git a/spec/views/layouts/_head.html.haml_spec.rb b/spec/views/layouts/_head.html.haml_spec.rb index 41e685f185a..f181e18e53d 100644 --- a/spec/views/layouts/_head.html.haml_spec.rb +++ b/spec/views/layouts/_head.html.haml_spec.rb @@ -92,28 +92,6 @@ describe 'layouts/_head' do end end - context 'when pendo is enabled' do - it 'adds a pendo initialization snippet with url', :aggregate_failures do - allow(Gitlab::CurrentSettings).to receive(:pendo_enabled?).and_return(true) - allow(Gitlab::CurrentSettings).to receive(:pendo_url).and_return('www.pen.do') - - render - - expect(rendered).to match('pendo.initialize') - expect(rendered).to match('www.pen.do') - end - end - - context 'when pendo is not enabled' do - it 'do not add pendo snippet' do - allow(Gitlab::CurrentSettings).to receive(:pendo_enabled?).and_return(false) - - render - - expect(rendered).not_to match('pendo.initialize') - end - end - context 'when a Piwik config is set' do let(:piwik_host) { 'piwik.example.com' } |