From 3a4cb6d6759abbfd16e048413e8545d1d94e7d9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Tue, 23 Jul 2019 09:30:00 +0000 Subject: Bring backward compatibility for request profiles It seems that we missed the backward compatibility support for profiles in the existing folder. This commit also fixes some specs to be idempotent and work in a temporary directory which not always seems to be the case. This commit also brings the profile_spec.rb which seems to be missing. --- .../admin/requests_profiles_controller.rb | 4 +- lib/gitlab/request_profiler.rb | 15 ++++++ lib/gitlab/request_profiler/profile.rb | 29 ++++------- .../admin/requests_profiles_controller_spec.rb | 2 +- .../features/admin/admin_requests_profiles_spec.rb | 12 ++++- spec/lib/gitlab/request_profiler/profile_spec.rb | 59 ++++++++++++++++++++++ spec/lib/gitlab/request_profiler_spec.rb | 41 ++++++++++++--- 7 files changed, 130 insertions(+), 32 deletions(-) create mode 100644 spec/lib/gitlab/request_profiler/profile_spec.rb diff --git a/app/controllers/admin/requests_profiles_controller.rb b/app/controllers/admin/requests_profiles_controller.rb index 1d14812186a..24383455064 100644 --- a/app/controllers/admin/requests_profiles_controller.rb +++ b/app/controllers/admin/requests_profiles_controller.rb @@ -3,12 +3,12 @@ class Admin::RequestsProfilesController < Admin::ApplicationController def index @profile_token = Gitlab::RequestProfiler.profile_token - @profiles = Gitlab::RequestProfiler::Profile.all.group_by(&:request_path) + @profiles = Gitlab::RequestProfiler.all.group_by(&:request_path) end def show clean_name = Rack::Utils.clean_path_info(params[:name]) - profile = Gitlab::RequestProfiler::Profile.find(clean_name) + profile = Gitlab::RequestProfiler.find(clean_name) unless profile && profile.content_type return redirect_to admin_requests_profiles_path, alert: 'Profile not found' diff --git a/lib/gitlab/request_profiler.rb b/lib/gitlab/request_profiler.rb index 64593153686..033e451dbee 100644 --- a/lib/gitlab/request_profiler.rb +++ b/lib/gitlab/request_profiler.rb @@ -6,6 +6,21 @@ module Gitlab module RequestProfiler PROFILES_DIR = "#{Gitlab.config.shared.path}/tmp/requests_profiles".freeze + def all + Dir["#{PROFILES_DIR}/*.{html,txt}"].map do |path| + Profile.new(File.basename(path)) + end.select(&:valid?) + end + module_function :all + + def find(name) + file_path = File.join(PROFILES_DIR, name) + return unless File.exist?(file_path) + + Profile.new(name) + end + module_function :find + def profile_token Rails.cache.fetch('profile-token') do Devise.friendly_token diff --git a/lib/gitlab/request_profiler/profile.rb b/lib/gitlab/request_profiler/profile.rb index 74f2ec1d083..76c675658b1 100644 --- a/lib/gitlab/request_profiler/profile.rb +++ b/lib/gitlab/request_profiler/profile.rb @@ -7,19 +7,6 @@ module Gitlab alias_method :to_param, :name - def self.all - Dir["#{PROFILES_DIR}/*.{html,txt}"].map do |path| - new(File.basename(path)) - end - end - - def self.find(name) - file_path = File.join(PROFILES_DIR, name) - return unless File.exist?(file_path) - - new(name) - end - def initialize(name) @name = name @file_path = File.join(PROFILES_DIR, name) @@ -27,8 +14,8 @@ module Gitlab set_attributes end - def content - File.read("#{PROFILES_DIR}/#{name}") + def valid? + @request_path.present? end def content_type @@ -43,11 +30,13 @@ module Gitlab private def set_attributes - _, path, timestamp, profile_mode, type = name.split(/(.*)_(\d+)_(.*)\.(html|txt)$/) - @request_path = path.tr('|', '/') - @time = Time.at(timestamp.to_i).utc - @profile_mode = profile_mode - @type = type + matches = name.match(/^(?.*)_(?\d+)(_(?\w+))?\.(?html|txt)$/) + return unless matches + + @request_path = matches[:path].tr('|', '/') + @time = Time.at(matches[:timestamp].to_i).utc + @profile_mode = matches[:profile_mode] || 'unknown' + @type = matches[:type] end end end diff --git a/spec/controllers/admin/requests_profiles_controller_spec.rb b/spec/controllers/admin/requests_profiles_controller_spec.rb index 289bb58c5a8..345f7720c25 100644 --- a/spec/controllers/admin/requests_profiles_controller_spec.rb +++ b/spec/controllers/admin/requests_profiles_controller_spec.rb @@ -23,7 +23,7 @@ describe Admin::RequestsProfilesController do end after do - File.unlink(test_file) + FileUtils.rm_rf(tmpdir) end context 'when loading HTML profile' do diff --git a/spec/features/admin/admin_requests_profiles_spec.rb b/spec/features/admin/admin_requests_profiles_spec.rb index a962af4952b..e8764d0a79c 100644 --- a/spec/features/admin/admin_requests_profiles_spec.rb +++ b/spec/features/admin/admin_requests_profiles_spec.rb @@ -1,13 +1,15 @@ require 'spec_helper' describe 'Admin::RequestsProfilesController' do + let(:tmpdir) { Dir.mktmpdir('profiler-test') } + before do - FileUtils.mkdir_p(Gitlab::RequestProfiler::PROFILES_DIR) + stub_const('Gitlab::RequestProfiler::PROFILES_DIR', tmpdir) sign_in(create(:admin)) end after do - Gitlab::RequestProfiler.remove_all_profiles + FileUtils.rm_rf(tmpdir) end describe 'GET /admin/requests_profiles' do @@ -60,6 +62,12 @@ describe 'Admin::RequestsProfilesController' do name: "|gitlab-org|infrastructure_#{time2.to_i}_memory.html", created: time2, profile_mode: 'Memory' + }, + { + request_path: '/gitlab-org/infrastructure', + name: "|gitlab-org|infrastructure_#{time2.to_i}.html", + created: time2, + profile_mode: 'Unknown' } ] end diff --git a/spec/lib/gitlab/request_profiler/profile_spec.rb b/spec/lib/gitlab/request_profiler/profile_spec.rb new file mode 100644 index 00000000000..b37ee558e1a --- /dev/null +++ b/spec/lib/gitlab/request_profiler/profile_spec.rb @@ -0,0 +1,59 @@ +require 'fast_spec_helper' + +describe Gitlab::RequestProfiler::Profile do + let(:profile) { described_class.new(filename) } + + describe '.new' do + context 'using old filename' do + let(:filename) { '|api|v4|version.txt_1562854738.html' } + + it 'returns valid data' do + expect(profile).to be_valid + expect(profile.request_path).to eq('/api/v4/version.txt') + expect(profile.time).to eq(Time.at(1562854738).utc) + expect(profile.type).to eq('html') + end + end + + context 'using new filename' do + let(:filename) { '|api|v4|version.txt_1563547949_execution.html' } + + it 'returns valid data' do + expect(profile).to be_valid + expect(profile.request_path).to eq('/api/v4/version.txt') + expect(profile.profile_mode).to eq('execution') + expect(profile.time).to eq(Time.at(1563547949).utc) + expect(profile.type).to eq('html') + end + end + end + + describe '#content_type' do + context 'when using html file' do + let(:filename) { '|api|v4|version.txt_1562854738_memory.html' } + + it 'returns valid data' do + expect(profile).to be_valid + expect(profile.content_type).to eq('text/html') + end + end + + context 'when using text file' do + let(:filename) { '|api|v4|version.txt_1562854738_memory.txt' } + + it 'returns valid data' do + expect(profile).to be_valid + expect(profile.content_type).to eq('text/plain') + end + end + + context 'when file is unknown' do + let(:filename) { '|api|v4|version.txt_1562854738_memory.xxx' } + + it 'returns valid data' do + expect(profile).not_to be_valid + expect(profile.content_type).to be_nil + end + end + end +end diff --git a/spec/lib/gitlab/request_profiler_spec.rb b/spec/lib/gitlab/request_profiler_spec.rb index fd8cbf39bce..498c045b6cd 100644 --- a/spec/lib/gitlab/request_profiler_spec.rb +++ b/spec/lib/gitlab/request_profiler_spec.rb @@ -13,15 +13,42 @@ describe Gitlab::RequestProfiler do end end - describe '.remove_all_profiles' do - it 'removes Gitlab::RequestProfiler::PROFILES_DIR directory' do - dir = described_class::PROFILES_DIR - FileUtils.mkdir_p(dir) + context 'with temporary PROFILES_DIR' do + let(:tmpdir) { Dir.mktmpdir('profiler-test') } + let(:profile_name) { '|api|v4|version.txt_1562854738_memory.html' } + let(:profile_path) { File.join(tmpdir, profile_name) } - expect(Dir.exist?(dir)).to be true + before do + stub_const('Gitlab::RequestProfiler::PROFILES_DIR', tmpdir) + FileUtils.touch(profile_path) + end + + after do + FileUtils.rm_rf(tmpdir) + end + + describe '.remove_all_profiles' do + it 'removes Gitlab::RequestProfiler::PROFILES_DIR directory' do + described_class.remove_all_profiles + + expect(Dir.exist?(tmpdir)).to be false + end + end + + describe '.all' do + subject { described_class.all } + + it 'returns all profiles' do + expect(subject.map(&:name)).to contain_exactly(profile_name) + end + end + + describe '.find' do + subject { described_class.find(profile_name) } - described_class.remove_all_profiles - expect(Dir.exist?(dir)).to be false + it 'returns all profiles' do + expect(subject.name).to eq(profile_name) + end end end end -- cgit v1.2.1