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.

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'

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

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(/^(?<path>.*)_(?<timestamp>\d+)(_(?<profile_mode>\w+))?\.(?<type>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

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

spec/features/admin/admin_requests_profiles_spec.rb

 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

spec/lib/gitlab/request_profiler/profile_spec.rb

+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

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