From 02e3896f981d8998e3c78ed7d9259e022063e975 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Mon, 22 Jan 2024 15:04:45 -0500 Subject: [PATCH 01/34] Add vernier backend support --- .github/workflows/ci.yml | 2 +- Gemfile | 1 + Gemfile.lock | 2 + README.md | 20 +- dev.yml | 2 +- lib/app_profiler.rb | 30 ++- lib/app_profiler/backend.rb | 33 +++ lib/app_profiler/backend/stackprof.rb | 89 ++++++++ lib/app_profiler/backend/vernier.rb | 101 +++++++++ lib/app_profiler/middleware.rb | 25 +++ lib/app_profiler/middleware/base_action.rb | 2 +- lib/app_profiler/parameters.rb | 11 +- lib/app_profiler/profile.rb | 53 +++-- lib/app_profiler/profile/stackprof.rb | 36 ++++ lib/app_profiler/profile/vernier.rb | 44 ++++ lib/app_profiler/profiler.rb | 86 -------- lib/app_profiler/railtie.rb | 1 + lib/app_profiler/request_parameters.rb | 20 +- test/app_profiler/backend.rb | 20 ++ .../backend/stackprof_backend_test.rb | 193 +++++++++++++++++ .../backend/vernier_backend_test.rb | 202 ++++++++++++++++++ .../middleware/upload_action_test.rb | 4 +- .../middleware/view_action_test.rb | 2 +- test/app_profiler/middleware_test.rb | 57 ++++- .../stackprof_test.rb} | 40 ++-- test/app_profiler/profile/vernier_test.rb | 161 ++++++++++++++ test/app_profiler/profiler_test.rb | 185 ---------------- test/app_profiler/request_parameters_test.rb | 7 +- test/app_profiler/run_test.rb | 10 +- test/test_helper.rb | 30 +++ 30 files changed, 1121 insertions(+), 348 deletions(-) create mode 100644 lib/app_profiler/backend.rb create mode 100644 lib/app_profiler/backend/stackprof.rb create mode 100644 lib/app_profiler/backend/vernier.rb create mode 100644 lib/app_profiler/profile/stackprof.rb create mode 100644 lib/app_profiler/profile/vernier.rb delete mode 100644 lib/app_profiler/profiler.rb create mode 100644 test/app_profiler/backend.rb create mode 100644 test/app_profiler/backend/stackprof_backend_test.rb create mode 100644 test/app_profiler/backend/vernier_backend_test.rb rename test/app_profiler/{profile_test.rb => profile/stackprof_test.rb} (78%) create mode 100644 test/app_profiler/profile/vernier_test.rb delete mode 100644 test/app_profiler/profiler_test.rb diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 30426faa..b8f26562 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,6 +12,7 @@ jobs: - "2.7" - "3.0" - "3.1" + - "3.2.1" steps: - uses: actions/checkout@v3 - name: Set up Ruby ${{ matrix.ruby }} @@ -20,7 +21,6 @@ jobs: ruby-version: ${{ matrix.ruby }} - name: Build run: | - gem install bundler bundle install --jobs 4 --retry 3 - name: Test on ${{ matrix.ruby }} run: | diff --git a/Gemfile b/Gemfile index c89a6423..7b40c54a 100644 --- a/Gemfile +++ b/Gemfile @@ -6,6 +6,7 @@ gemspec # Specify the same dependency sources as the application Gemfile gem("activesupport", "~> 5.2") gem("railties", "~> 5.2") +gem("vernier", "~> 0.3.1") if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new("3.2.1") gem("google-cloud-storage", "~> 1.21") gem("rubocop", require: false) diff --git a/Gemfile.lock b/Gemfile.lock index eb0db76e..e61e3030 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -179,6 +179,7 @@ GEM thread_safe (~> 0.1) uber (0.1.0) unicode-display_width (2.1.0) + vernier (0.3.1) webrick (1.7.0) PLATFORMS @@ -197,6 +198,7 @@ DEPENDENCIES rubocop rubocop-performance rubocop-shopify + vernier (~> 0.3.1) BUNDLED WITH 2.4.18 diff --git a/README.md b/README.md index 8925458b..5bf72f2d 100644 --- a/README.md +++ b/README.md @@ -76,12 +76,14 @@ Rails.application.config.app_profiler.profile_header = "X-Profile" | Key | Value | Notes | | --- | ----- | ----- | -| profile/mode | Supported profiling modes: `cpu`, `wall`, `object`. | Use `profile` in (1), and `mode` in (2). | +| profile/mode | Supported profiling modes: `cpu`, `wall`, `object` for stackprof. | Use `profile` in (1), and `mode` in (2). Vernier backend only supports `wall` and `retained` at present time| | async | Upload profile in a background thread. When this is set, profile redirect headers are not present in the response. | interval | Sampling interval in microseconds. | | | ignore_gc | Ignore garbage collection frames | | | autoredirect | Redirect request automatically to Speedscope's page after profiling. | | | context | Directory within the specified bucket in the selected storage where raw profile data should be written. | Only supported in (2). Defaults to `Rails.env` if not specified. | +| backend | Profiler to use, either `stackprof` or `vernier`. Defaults to stackprof. Available only vernier is installed in the app, and Ruby version is at least 3.2.1 | + Note that the `autoredirect` feature can be turned on for all requests by doing the following: @@ -312,6 +314,22 @@ Note that in `development` and `test` modes the file isn't uploaded. Instead, it Rails.application.config.app_profiler.middleware_action = AppProfiler::Middleware::UploadAction ``` +## Profiler backends + +It is possible to configure app_profiler to use the [`vernier`](https://github.com/jhawthorn/vernier) or stackprof. To use vernier, it must be added separately in the application Gemfile. + +The backend can be selected at dynamicall runtime using the `backend` parameter. The default backend to use when this parameter is not specified can be configured with: + +```ruby +AppProfiler.profiler_backend = AppProfiler::StackprofBackend # or AppProfiler::VernierBackend +# OR +Rails.application.config.app_profiler.profiler_backend = AppProfiler::StackprofBackend # or AppProfiler::VernierBackend +``` + +By default, the stackprof backend will be used. + +In local development, changing the backend will change whether the profile is viewed in speedscope, or firefox-profiler. + ## Running tests ``` diff --git a/dev.yml b/dev.yml index 9c95af0c..1e16d909 100644 --- a/dev.yml +++ b/dev.yml @@ -3,7 +3,7 @@ name: app-profiler type: ruby up: - - ruby: 3.1.2 + - ruby: 3.2.1 - bundler commands: diff --git a/lib/app_profiler.rb b/lib/app_profiler.rb index 49405faa..24a739c2 100644 --- a/lib/app_profiler.rb +++ b/lib/app_profiler.rb @@ -32,8 +32,8 @@ module Viewer require "app_profiler/middleware" require "app_profiler/parameters" require "app_profiler/request_parameters" - require "app_profiler/profiler" require "app_profiler/profile" + require "app_profiler/backend" require "app_profiler/server" mattr_accessor :logger, default: Logger.new($stdout) @@ -47,6 +47,7 @@ module Viewer mattr_accessor :context, default: nil mattr_reader :profile_url_formatter, default: DefaultProfileFormatter + mattr_accessor :profiler_backend, default: AppProfiler::StackprofBackend mattr_accessor :storage, default: Storage::FileStorage mattr_accessor :viewer, default: Viewer::SpeedscopeViewer @@ -61,16 +62,35 @@ module Viewer class << self def run(*args, &block) - Profiler.run(*args, &block) + profiler.run(*args, &block) end def start(*args) - Profiler.start(*args) + profiler.start(*args) end def stop - Profiler.stop - Profiler.results + profiler.stop + profiler.results + end + + def running? + @backend&.running? + end + + def profiler + if @backend && !@backend.is_a?(profiler_backend) + raise ConfigurationError if @backend.running? + + clear + end + + @backend ||= profiler_backend.new + end + + def clear + @backend.stop if @backend&.running? + @backend = nil end def profile_header=(profile_header) diff --git a/lib/app_profiler/backend.rb b/lib/app_profiler/backend.rb new file mode 100644 index 00000000..ddee219e --- /dev/null +++ b/lib/app_profiler/backend.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module AppProfiler + class Backend + def run(params = {}, &block) + raise NotImplementedError + end + + def start(params = {}) + raise NotImplementedError + end + + def stop + raise NotImplementedError + end + + def results + raise NotImplementedError + end + + def running? + raise NotImplementedError + end + end +end + +require "app_profiler/backend/stackprof" + +begin + require "app_profiler/backend/vernier" +rescue LoadError + warn("Vernier is not supported.") +end diff --git a/lib/app_profiler/backend/stackprof.rb b/lib/app_profiler/backend/stackprof.rb new file mode 100644 index 00000000..1e43b5ad --- /dev/null +++ b/lib/app_profiler/backend/stackprof.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require "stackprof" + +module AppProfiler + class StackprofBackend < Backend + NAME = "stackprof" + DEFAULTS = { + mode: :cpu, + raw: true, + }.freeze + + AVAILABLE_MODES = [ + :wall, + :cpu, + :object, + ].freeze + + def run(params = {}) + started = start(params) + + yield + + return unless started + + stop + results + ensure + # Only stop the profiler if profiling was started in this context. + stop if started + end + + def start(params = {}) + # Do not start the profiler if StackProf was started somewhere else. + return false if running? + + clear + + ::StackProf.start(**DEFAULTS, **params) + rescue => error + AppProfiler.logger.info( + "[Profiler] failed to start the profiler error_class=#{error.class} error_message=#{error.message}" + ) + # This is a boolean instead of nil because StackProf#start returns a + # boolean as well. + false + end + + def stop + ::StackProf.stop + end + + def results + stackprof_profile = backend_results + + return unless stackprof_profile + + AppProfiler::Profile.from_stackprof(stackprof_profile) + rescue => error + AppProfiler.logger.info( + "[Profiler] failed to obtain the profile error_class=#{error.class} error_message=#{error.message}" + ) + nil + end + + def running? + ::StackProf.running? + end + + private + + def backend_results + ::StackProf.results + end + + # Clears the previous profiling session. + # + # StackProf will attempt to reuse frames from the previous profiling + # session if the results are not collected. This is usually called before + # StackProf#start is invoked to ensure that new profiling sessions do + # not reuse previous frames if they exist. + # + # Ref: https://github.com/tmm1/stackprof/blob/0ded6c/ext/stackprof/stackprof.c#L118-L123 + # + def clear + backend_results + end + end +end diff --git a/lib/app_profiler/backend/vernier.rb b/lib/app_profiler/backend/vernier.rb new file mode 100644 index 00000000..3627e9f0 --- /dev/null +++ b/lib/app_profiler/backend/vernier.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +begin + gem("vernier", ">= 0.3.1") + require "vernier" +rescue LoadError + warn("Vernier profiling support requires the vernier gem, version 0.3.1 or later." \ + "Please add it to your Gemfile: `gem \"vernier\", \">= 0.3.1\"`") + raise +end + +module AppProfiler + class VernierBackend < Backend + NAME = "vernier" + + DEFAULTS = { + mode: :wall, + }.freeze + + AVAILABLE_MODES = [ + :wall, + :retained, + ].freeze + + def run(params = {}) + started = start(params) + + yield + + return unless started + + stop + results + ensure + # Only stop the profiler if profiling was started in this context. + stop if started + end + + def start(params = {}) + # Do not start the profiler if we already have a collector started somewhere else. + return false if running? + + @mode = params.delete(:mode) || DEFAULTS[:mode] + raise ArgumentError unless AVAILABLE_MODES.include?(@mode) + + @metadata = params.delete(:metadata) + clear + + @collector ||= ::Vernier::Collector.new(@mode, **params) + @collector.start + rescue => error + AppProfiler.logger.info( + "[Profiler] failed to start the profiler error_class=#{error.class} error_message=#{error.message}" + ) + # This is a boolean instead of nil to be consistent with the stackprof backend behaviour + # boolean as well. + false + end + + def stop + return false unless running? + + @results = @collector&.stop + @collector = nil + !@results.nil? + end + + def results + vernier_profile = backend_results + clear + + return unless vernier_profile + + vernier_profile.meta[:mode] = @mode # TODO: https://github.com/jhawthorn/vernier/issues/30 + vernier_profile.meta.merge!(@metadata) if @metadata + @mode = nil + @metadata = nil + + AppProfiler::Profile.from_vernier(vernier_profile) + rescue => error + AppProfiler.logger.info( + "[Profiler] failed to obtain the profile error_class=#{error.class} error_message=#{error.message}" + ) + nil + end + + def running? + @collector != nil + end + + private + + def backend_results + @results + end + + def clear + @results = nil + end + end +end diff --git a/lib/app_profiler/middleware.rb b/lib/app_profiler/middleware.rb index 232ead57..42f39c75 100644 --- a/lib/app_profiler/middleware.rb +++ b/lib/app_profiler/middleware.rb @@ -12,6 +12,7 @@ class Middleware def initialize(app) @app = app + @backend_lock = Mutex.new end def call(env, params = AppProfiler::RequestParameters.new(Rack::Request.new(env))) @@ -24,6 +25,7 @@ def call(env, params = AppProfiler::RequestParameters.new(Rack::Request.new(env) def profile(env, params) response = nil + orig_backend = nil return yield unless params.valid? @@ -31,6 +33,23 @@ def profile(env, params) return yield unless before_profile(env, params_hash) + @backend_lock.synchronize do + if params.backend && + AppProfiler.profiler_backend.name.split("::").last.downcase.gsub("backend", "") != params.backend + raise ArgumentError if AppProfiler.running? + + orig_backend = AppProfiler.profiler_backend + if defined?(AppProfiler::VernierBackend) && + params.backend == AppProfiler::VernierBackend::NAME + AppProfiler.profiler_backend = AppProfiler::VernierBackend + elsif params.backend == AppProfiler::StackprofBackend::NAME + AppProfiler.profiler_backend = AppProfiler::StackprofBackend + else + raise ArgumentError + end + end + end + profile = AppProfiler.run(params_hash) do response = yield end @@ -45,6 +64,12 @@ def profile(env, params) ) response + ensure + if orig_backend + @backend_lock.synchronize do + AppProfiler.profiler_backend = orig_backend + end + end end def before_profile(_env, _params) diff --git a/lib/app_profiler/middleware/base_action.rb b/lib/app_profiler/middleware/base_action.rb index 7fdb3970..69d6713c 100644 --- a/lib/app_profiler/middleware/base_action.rb +++ b/lib/app_profiler/middleware/base_action.rb @@ -9,7 +9,7 @@ def call(_profile, _params = {}) end def cleanup - profile = Profiler.results + profile = AppProfiler.profiler.results call(profile) if profile end end diff --git a/lib/app_profiler/parameters.rb b/lib/app_profiler/parameters.rb index ffffaf9e..42c197bc 100644 --- a/lib/app_profiler/parameters.rb +++ b/lib/app_profiler/parameters.rb @@ -4,17 +4,18 @@ module AppProfiler class Parameters - DEFAULT_INTERVALS = { "cpu" => 1000, "wall" => 1000, "object" => 2000 }.freeze - MIN_INTERVALS = { "cpu" => 200, "wall" => 200, "object" => 400 }.freeze - MODES = DEFAULT_INTERVALS.keys.freeze + DEFAULT_INTERVALS = { "cpu" => 1000, "wall" => 1000, "object" => 2000, "retained" => 0 }.freeze + MIN_INTERVALS = { "cpu" => 200, "wall" => 200, "object" => 400, "retained" => 0 }.freeze - attr_reader :autoredirect, :async + attr_reader :autoredirect, :async, :backend - def initialize(mode: :wall, interval: nil, ignore_gc: false, autoredirect: false, async: false, metadata: {}) + def initialize(mode: :wall, interval: nil, ignore_gc: false, autoredirect: false, + async: false, backend: nil, metadata: {}) @mode = mode.to_sym @interval = [interval&.to_i || DEFAULT_INTERVALS.fetch(@mode.to_s), MIN_INTERVALS.fetch(@mode.to_s)].max @ignore_gc = !!ignore_gc @autoredirect = autoredirect + @backend = backend @metadata = { context: AppProfiler.context }.merge(metadata) @async = async end diff --git a/lib/app_profiler/profile.rb b/lib/app_profiler/profile.rb index 9fa323bb..11e5f66a 100644 --- a/lib/app_profiler/profile.rb +++ b/lib/app_profiler/profile.rb @@ -6,37 +6,35 @@ class Profile private_constant :INTERNAL_METADATA_KEYS class UnsafeFilename < StandardError; end - delegate :[], to: :@data attr_reader :id, :context # This function should not be called if `StackProf.results` returns nil. def self.from_stackprof(data) options = INTERNAL_METADATA_KEYS.map { |key| [key, data[:metadata]&.delete(key)] }.to_h - new(data, **options).tap do |profile| + StackprofProfile.new(data, **options).tap do |profile| raise ArgumentError, "invalid profile data" unless profile.valid? end end - # `data` is assumed to be a Hash. + def self.from_vernier(data) + # FIXME: we don't delete here, that is causing a segfault in vernier. Divergent behaviour from stackprof, + # as the special metadata keys "id" and "context" are preserved into the metadata, but maybe that isn't so bad. + options = INTERNAL_METADATA_KEYS.map { |key| [key, data.meta.clone[key]] }.to_h + + VernierProfile.new(data, **options).tap do |profile| + raise ArgumentError, "invalid profile data" unless profile.valid? + end + end + + # `data` is assumed to be a Hash for Stackprof, + # a vernier "result" object for vernier def initialize(data, id: nil, context: nil) @id = id.presence || SecureRandom.hex @context = context @data = data end - def valid? - mode.present? - end - - def mode - @data[:mode] - end - - def view(params = {}) - AppProfiler.viewer.view(self, **params) - end - def upload AppProfiler.storage.upload(self).tap do |upload| if upload && defined?(upload.url) @@ -61,14 +59,27 @@ def enqueue_upload end def file - @file ||= path.tap do |p| - p.dirname.mkpath - p.write(JSON.dump(@data)) - end + raise NotImplementedError end def to_h - @data + raise NotImplementedError + end + + def valid? + raise NotImplementedError + end + + def mode + raise NotImplementedError + end + + def format + raise NotImplementedError + end + + def view(params = {}) + raise NotImplementedError end private @@ -79,7 +90,7 @@ def path mode, id, Socket.gethostname, - ].compact.join("-") << ".json" + ].compact.join("-") << format raise UnsafeFilename if /[^0-9A-Za-z.\-\_]/.match?(filename) diff --git a/lib/app_profiler/profile/stackprof.rb b/lib/app_profiler/profile/stackprof.rb new file mode 100644 index 00000000..1d08c259 --- /dev/null +++ b/lib/app_profiler/profile/stackprof.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module AppProfiler + class StackprofProfile < Profile + FILE_EXTENSION = ".stackprof.json" + + delegate :[], to: :@data + + def file + @file ||= path.tap do |p| + p.dirname.mkpath + p.write(JSON.dump(@data)) + end + end + + def to_h + @data + end + + def valid? + mode.present? + end + + def mode + @data[:mode] + end + + def format + FILE_EXTENSION + end + + def view(params = {}) + Viewer::SpeedscopeViewer.view(self, **params) + end + end +end diff --git a/lib/app_profiler/profile/vernier.rb b/lib/app_profiler/profile/vernier.rb new file mode 100644 index 00000000..975650a8 --- /dev/null +++ b/lib/app_profiler/profile/vernier.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module AppProfiler + class VernierProfile < Profile + FILE_EXTENSION = ".gecko.json" + + delegate :[], to: :@meta + + attr_reader :data + + def initialize(data, id: nil, context: nil) + @meta = data.meta + super + end + + # https://github.com/jhawthorn/vernier/blob/main/lib/vernier/result.rb#L27-L29 + def file + @file ||= path.tap do |p| + p.dirname.mkpath + @data.write(out: p) + end + end + + def valid? + !@data.nil? + end + + def to_h + @data.to_h + end + + def mode + @meta[:mode] + end + + def format + FILE_EXTENSION + end + + def view(params = {}) + Viewer::FirefoxViewer.view(self, **params) + end + end +end diff --git a/lib/app_profiler/profiler.rb b/lib/app_profiler/profiler.rb deleted file mode 100644 index bbb922df..00000000 --- a/lib/app_profiler/profiler.rb +++ /dev/null @@ -1,86 +0,0 @@ -# frozen_string_literal: true - -require "stackprof" - -module AppProfiler - module Profiler - DEFAULTS = { - mode: :cpu, - raw: true, - }.freeze - - class << self - def run(params = {}) - started = start(params) - - yield - - return unless started - - stop - results - ensure - # Only stop the profiler if profiling was started in this context. - stop if started - end - - def start(params = {}) - # Do not start the profiler if StackProf was started somewhere else. - return false if running? - - clear - - StackProf.start(**DEFAULTS, **params) - rescue => error - AppProfiler.logger.info( - "[Profiler] failed to start the profiler error_class=#{error.class} error_message=#{error.message}" - ) - # This is a boolean instead of nil because StackProf#start returns a - # boolean as well. - false - end - - def stop - StackProf.stop - end - - def results - stackprof_profile = stackprof_results - - return unless stackprof_profile - - Profile.from_stackprof(stackprof_profile) - rescue => error - AppProfiler.logger.info( - "[Profiler] failed to obtain the profile error_class=#{error.class} error_message=#{error.message}" - ) - nil - end - - private - - def stackprof_results - StackProf.results - end - - # Clears the previous profiling session. - # - # StackProf will attempt to reuse frames from the previous profiling - # session if the results are not collected. This is usually called before - # StackProf#start is invoked to ensure that new profiling sessions do - # not reuse previous frames if they exist. - # - # Ref: https://github.com/tmm1/stackprof/blob/0ded6c/ext/stackprof/stackprof.c#L118-L123 - # - def clear - stackprof_results - end - - def running? - StackProf.running? - end - end - end - - private_constant :Profiler -end diff --git a/lib/app_profiler/railtie.rb b/lib/app_profiler/railtie.rb index 98ad041d..42b5562d 100644 --- a/lib/app_profiler/railtie.rb +++ b/lib/app_profiler/railtie.rb @@ -40,6 +40,7 @@ class Railtie < Rails::Railtie AppProfiler.profile_enqueue_success = app.config.app_profiler.profile_enqueue_success AppProfiler.profile_enqueue_failure = app.config.app_profiler.profile_enqueue_failure AppProfiler.after_process_queue = app.config.app_profiler.after_process_queue + AppProfiler.profiler_backend = app.config.app_profiler.profiler_backend end initializer "app_profiler.add_middleware" do |app| diff --git a/lib/app_profiler/request_parameters.rb b/lib/app_profiler/request_parameters.rb index 40afcc99..8d43ad53 100644 --- a/lib/app_profiler/request_parameters.rb +++ b/lib/app_profiler/request_parameters.rb @@ -16,17 +16,29 @@ def async query_param("async") end + def backend + query_param("backend") || profile_header_param("backend") || + AppProfiler.profiler_backend.name.split("::").last.downcase.gsub("backend", "") + end + def valid? if mode.blank? return false end - unless Parameters::MODES.include?(mode) - AppProfiler.logger.info("[Profiler] unsupported profiling mode=#{mode}") + return false if backend != AppProfiler::StackprofBackend::NAME && !defined?(AppProfiler::VernierBackend) + + if defined?(AppProfiler::VernierBackend) && backend == AppProfiler::VernierBackend::NAME && + !AppProfiler::VernierBackend::AVAILABLE_MODES.include?(mode.to_sym) + AppProfiler.logger.info("[AppProfiler] unsupported profiling mode=#{mode} for backend #{backend}") + return false + elsif backend == AppProfiler::StackprofBackend::NAME && + !AppProfiler::StackprofBackend::AVAILABLE_MODES.include?(mode.to_sym) + AppProfiler.logger.info("[AppProfiler] unsupported profiling mode=#{mode} for backend #{backend}") return false end - if interval.to_i < Parameters::MIN_INTERVALS[mode] + if interval.to_i < Parameters::MIN_INTERVALS[mode.to_s] return false end @@ -56,7 +68,7 @@ def ignore_gc end def interval - query_param("interval") || profile_header_param("interval") || Parameters::DEFAULT_INTERVALS[mode] + query_param("interval") || profile_header_param("interval") || Parameters::DEFAULT_INTERVALS[mode.to_s] end def request_id diff --git a/test/app_profiler/backend.rb b/test/app_profiler/backend.rb new file mode 100644 index 00000000..efaee3f3 --- /dev/null +++ b/test/app_profiler/backend.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +require "test_helper" + +module AppProfiler + class BackendTest < TestCase + test ".clear is required to change backends" do + AppProfiler.profiler_backend = AppProfiler::StackprofBackend + assert_instance_of(StackprofBackend, AppProfiler.profiler) + + AppProfiler.profiler_backend = AppProfiler::VernierBackend + assert_raises(ConfigurationError) { AppProfiler.profiler } + + AppProfiler.clear + + AppProfiler.profiler_backend = AppProfiler::VernierBackend + assert_instance_of(VernierBackend, AppProfiler.profiler) + end + end +end diff --git a/test/app_profiler/backend/stackprof_backend_test.rb b/test/app_profiler/backend/stackprof_backend_test.rb new file mode 100644 index 00000000..9ed41f50 --- /dev/null +++ b/test/app_profiler/backend/stackprof_backend_test.rb @@ -0,0 +1,193 @@ +# frozen_string_literal: true + +require "test_helper" + +module AppProfiler + class StackprofBackendTest < TestCase + def setup + AppProfiler.profiler_backend = AppProfiler::StackprofBackend + end + + def teardown + AppProfiler.clear + end + + test ".run prints error when failed" do + AppProfiler.logger.expects(:info).with { |value| value =~ /failed to start the profiler/ } + profile = AppProfiler.run(mode: :unsupported) do + sleep(0.1) + end + + assert_nil(profile) + end + + test ".run raises when yield raises" do + error = StandardError.new("An error occurred.") + exception = assert_raises(StandardError) do + AppProfiler.profiler.run(stackprof_profile) do + assert_predicate(AppProfiler.profiler, :running?) + raise error + end + end + + assert_equal(error, exception) + assert_not_predicate(AppProfiler.profiler, :running?) + end + + test ".run does not stop the profiler when it is already running" do + AppProfiler.logger.expects(:info).never + + assert_equal(true, AppProfiler.profiler.send(:start, stackprof_profile)) + + profile = AppProfiler.profiler.run(stackprof_profile) do + sleep(0.1) + end + + assert_nil(profile) + assert_predicate(AppProfiler.profiler, :running?) + ensure + StackProf.stop + end + + test ".run uses cpu profile by default" do + profile = AppProfiler.profiler.run(stackprof_profile) do + sleep(0.1) + end + + assert_instance_of(AppProfiler::StackprofProfile, profile) + assert_equal(:cpu, profile[:mode]) + assert_equal(1000, profile[:interval]) + end + + test ".run assigns metadata to profiles" do + profile = AppProfiler.profiler.run(stackprof_profile(metadata: { id: "wowza", context: "bar" })) do + sleep(0.1) + end + + assert_instance_of(AppProfiler::StackprofProfile, profile) + assert_equal("wowza", profile.id) + assert_equal("bar", profile.context) + end + + test ".run cpu profile" do + profile = AppProfiler.profiler.run(stackprof_profile(mode: :cpu, interval: 2000)) do + sleep(0.1) + end + + assert_instance_of(AppProfiler::StackprofProfile, profile) + assert_equal(:cpu, profile[:mode]) + assert_equal(2000, profile[:interval]) + end + + test ".run wall profile" do + profile = AppProfiler.profiler.run(stackprof_profile(mode: :wall, interval: 2000)) do + sleep(0.1) + end + + assert_instance_of(AppProfiler::StackprofProfile, profile) + assert_equal(:wall, profile[:mode]) + assert_equal(2000, profile[:interval]) + end + + test ".run object profile" do + profile = AppProfiler.profiler.run(stackprof_profile(mode: :object, interval: 2)) do + sleep(0.1) + end + + assert_instance_of(AppProfiler::StackprofProfile, profile) + assert_equal(:object, profile[:mode]) + assert_equal(2, profile[:interval]) + end + + test ".start uses cpu profile by default" do + AppProfiler.profiler.start(stackprof_profile) + AppProfiler.profiler.stop + + profile = AppProfiler.profiler.results + + assert_instance_of(AppProfiler::StackprofProfile, profile) + assert_equal(:cpu, profile[:mode]) + assert_equal(1000, profile[:interval]) + end + + test ".start assigns metadata to profiles" do + AppProfiler.profiler.start(stackprof_profile(metadata: { id: "wowza", context: "bar" })) + AppProfiler.profiler.stop + + profile = AppProfiler.profiler.results + + assert_instance_of(AppProfiler::StackprofProfile, profile) + assert_equal("wowza", profile.id) + assert_equal("bar", profile.context) + end + + test ".start cpu profile" do + AppProfiler.profiler.start(stackprof_profile(mode: :cpu, interval: 2000)) + AppProfiler.profiler.stop + + profile = AppProfiler.profiler.results + + assert_instance_of(AppProfiler::StackprofProfile, profile) + assert_equal(:cpu, profile[:mode]) + assert_equal(2000, profile[:interval]) + end + + test ".start wall profile" do + AppProfiler.profiler.start(stackprof_profile(mode: :wall, interval: 2000)) + AppProfiler.profiler.stop + + profile = AppProfiler.profiler.results + + assert_instance_of(AppProfiler::StackprofProfile, profile) + assert_equal(:wall, profile[:mode]) + assert_equal(2000, profile[:interval]) + end + + test ".start object profile" do + AppProfiler.profiler.start(stackprof_profile(mode: :object, interval: 2)) + AppProfiler.profiler.stop + + profile = AppProfiler.profiler.results + + assert_instance_of(AppProfiler::StackprofProfile, profile) + assert_equal(:object, profile[:mode]) + assert_equal(2, profile[:interval]) + end + + test ".stop" do + StackProf.expects(:stop) + AppProfiler.stop + end + + test ".results prints error when failed" do + AppProfiler.profiler.expects(:backend_results).returns({}) + AppProfiler.logger.expects(:info).with { |value| value =~ /failed to obtain the profile/ } + + assert_nil(AppProfiler.profiler.results) + end + + test ".results returns nil when profiling is still active" do + AppProfiler.profiler.run(stackprof_profile) do + assert_nil(AppProfiler.profiler.results) + end + end + + test ".start, .stop, and .results interact well" do + AppProfiler.logger.expects(:info).never + + assert_equal(true, AppProfiler.profiler.start(stackprof_profile)) + assert_equal(false, AppProfiler.profiler.start(stackprof_profile)) + assert_equal(true, AppProfiler.profiler.send(:running?)) + assert_nil(AppProfiler.profiler.results) + assert_equal(true, AppProfiler.profiler.stop) + assert_equal(false, AppProfiler.profiler.stop) + assert_equal(false, AppProfiler.profiler.send(:running?)) + + profile = AppProfiler.profiler.results + assert_instance_of(AppProfiler::StackprofProfile, profile) + assert_predicate(profile, :valid?) + + assert_nil(AppProfiler.profiler.results) + end + end +end diff --git a/test/app_profiler/backend/vernier_backend_test.rb b/test/app_profiler/backend/vernier_backend_test.rb new file mode 100644 index 00000000..25c95bc1 --- /dev/null +++ b/test/app_profiler/backend/vernier_backend_test.rb @@ -0,0 +1,202 @@ +# frozen_string_literal: true + +require "test_helper" + +return unless defined?(AppProfiler::VernierBackend) + +module AppProfiler + class VernierBackendTest < TestCase + def setup + AppProfiler.clear + @orig_backend = AppProfiler.profiler_backend + AppProfiler.profiler_backend = AppProfiler::VernierBackend + end + + def teardown + AppProfiler.profiler_backend = @orig_backend + AppProfiler.clear + end + + test ".run prints error when failed" do + AppProfiler.logger.expects(:info).with { |value| value =~ /failed to start the profiler/ } + profile = AppProfiler.profiler.run(mode: :unsupported) do + sleep(0.1) + end + + assert_nil(profile) + end + + test ".run raises when yield raises" do + error = StandardError.new("An error occurred.") + exception = assert_raises(StandardError) do + AppProfiler.profiler.run(vernier_params) do + assert_predicate(AppProfiler.profiler, :running?) + raise error + end + end + + assert_equal(error, exception) + assert_not_predicate(AppProfiler.profiler, :running?) + end + + test ".run does not stop the profiler when it is already running" do + AppProfiler.logger.expects(:info).never + + assert_equal(true, AppProfiler.profiler.send(:start, vernier_params)) + + profile = AppProfiler.profiler.run(vernier_params) do + sleep(0.1) + end + + assert_nil(profile) + assert_predicate(AppProfiler.profiler, :running?) + ensure + AppProfiler.profiler.stop + end + + test ".run uses wall profile by default" do + profile = AppProfiler.profiler.run do + sleep(0.1) + end + + assert_instance_of(AppProfiler::VernierProfile, profile) + assert_equal(:wall, profile[:mode]) + # assert_equal(1000, profile[:interval]) # TODO https://github.com/jhawthorn/vernier/issues/30 + end + + test ".run assigns metadata to profiles" do + profile = AppProfiler.profiler.run(vernier_params(metadata: { id: "wowza", context: "bar" })) do + sleep(0.1) + end + + assert_instance_of(AppProfiler::VernierProfile, profile) + assert_equal("wowza", profile.id) + assert_equal("bar", profile.context) + end + + test ".run wall profile" do + profile = AppProfiler.profiler.run(vernier_params(mode: :wall, interval: 2000)) do + sleep(0.1) + end + + assert_instance_of(AppProfiler::VernierProfile, profile) + assert_equal(:wall, profile[:mode]) + # assert_equal(2000, profile[:interval]) # TODO as above + end + + test ".run retained profile" do + retained = [] + objects = 10 + profile = AppProfiler.profiler.run(vernier_params(mode: :retained)) do + objects.times do + retained << Object.new + end + end + + assert_instance_of(AppProfiler::VernierProfile, profile) + assert_equal(:retained, profile[:mode]) + + weights = [] + profile.data.each_sample do |_, weight| + weights << weight + end + assert_operator(weights.size, :>=, objects) + end + + test ".run works for supported modes" do + profile = AppProfiler.profiler.run(vernier_params(mode: :wall)) do + sleep(0.1) + end + refute_equal(false, profile) + + profile = AppProfiler.profiler.run(vernier_params(mode: :retained)) do + sleep(0.1) + end + refute_equal(false, profile) + end + + test ".run fails for unsupported modes" do + unsupported_modes = [:cpu, :object, :garbage, :unsupported] + + unsupported_modes.each do |unsupported| + profile = AppProfiler.profiler.run(vernier_params(mode: unsupported)) do + sleep(0.1) + end + assert_nil(profile) + end + end + + test ".start uses wall profile by default" do + AppProfiler.profiler.start + AppProfiler.profiler.stop + + profile = AppProfiler.profiler.results + + assert_instance_of(AppProfiler::VernierProfile, profile) + assert_equal(:wall, profile[:mode]) + # assert_equal(1000, profile[:interval]) + end + + test ".start assigns metadata to profiles" do + AppProfiler.profiler.start(vernier_params(metadata: { id: "wowza", context: "bar" })) + AppProfiler.profiler.stop + + profile = AppProfiler.profiler.results + + assert_instance_of(AppProfiler::VernierProfile, profile) + assert_equal("wowza", profile.id) + assert_equal("bar", profile.context) + end + + test ".start wall profile" do + AppProfiler.profiler.start(vernier_params(mode: :wall, interval: 2000)) + AppProfiler.profiler.stop + + profile = AppProfiler.profiler.results + + assert_instance_of(AppProfiler::VernierProfile, profile) + assert_equal(:wall, profile[:mode]) + # assert_equal(2000, profile[:interval]) + end + + test ".stop" do + AppProfiler.start + AppProfiler::VernierBackend.any_instance.expects(:stop) + AppProfiler.stop + ensure + AppProfiler::VernierBackend.any_instance.unstub(:stop) + AppProfiler.stop + end + + test ".results prints error when failed" do + AppProfiler.profiler.expects(:backend_results).returns({}) + AppProfiler.logger.expects(:info).with { |value| value =~ /failed to obtain the profile/ } + + assert_nil(AppProfiler.profiler.results) + end + + test ".results returns nil when profiling is still active" do + AppProfiler.profiler.run do + assert_nil(AppProfiler.profiler.results) + end + end + + test ".start, .stop, and .results interact well" do + AppProfiler.logger.expects(:info).never + + assert_equal(true, AppProfiler.profiler.start) + assert_equal(false, AppProfiler.profiler.start) + assert_equal(true, AppProfiler.profiler.send(:running?)) + assert_nil(AppProfiler.profiler.results) + assert_equal(true, AppProfiler.profiler.stop) + assert_equal(false, AppProfiler.profiler.stop) + assert_equal(false, AppProfiler.profiler.send(:running?)) + + profile = AppProfiler.profiler.results + assert_instance_of(AppProfiler::VernierProfile, profile) + assert_predicate(profile, :valid?) + + assert_nil(AppProfiler.profiler.results) + end + end +end diff --git a/test/app_profiler/middleware/upload_action_test.rb b/test/app_profiler/middleware/upload_action_test.rb index f17f7953..003bf36b 100644 --- a/test/app_profiler/middleware/upload_action_test.rb +++ b/test/app_profiler/middleware/upload_action_test.rb @@ -6,12 +6,12 @@ module AppProfiler class Middleware class UploadActionTest < AppProfiler::TestCase setup do - @profile = Profile.new(stackprof_profile) + @profile = StackprofProfile.new(stackprof_profile) @response = [200, {}, ["OK"]] end test ".cleanup" do - Profiler.expects(:results).returns(@profile) + AppProfiler.profiler.expects(:results).returns(@profile) assert_nothing_raised do UploadAction.cleanup end diff --git a/test/app_profiler/middleware/view_action_test.rb b/test/app_profiler/middleware/view_action_test.rb index ed9fc984..b6cba7fe 100644 --- a/test/app_profiler/middleware/view_action_test.rb +++ b/test/app_profiler/middleware/view_action_test.rb @@ -11,7 +11,7 @@ class ViewActionTest < AppProfiler::TestCase end test ".cleanup" do - Profiler.expects(:results).returns(@profile) + AppProfiler.profiler.expects(:results).returns(@profile) @profile.expects(:view) ViewAction.cleanup diff --git a/test/app_profiler/middleware_test.rb b/test/app_profiler/middleware_test.rb index c5a80301..ca730dc9 100644 --- a/test/app_profiler/middleware_test.rb +++ b/test/app_profiler/middleware_test.rb @@ -20,8 +20,8 @@ class MiddlewareTest < TestCase end end - AppProfiler::Parameters::MODES.each do |mode| - test "profile mode #{mode} is supported" do + AppProfiler::StackprofBackend::AVAILABLE_MODES.each do |mode| + test "profile mode #{mode} is supported by stackprof backend" do assert_profiles_dumped do assert_profiles_uploaded do middleware = AppProfiler::Middleware.new(app_env) @@ -31,6 +31,43 @@ class MiddlewareTest < TestCase end end + if defined?(AppProfiler::VernierBackend) + AppProfiler::VernierBackend::AVAILABLE_MODES.each do |mode| + test "profile mode #{mode} is supported by vernier backend" do + assert_profiles_dumped do + assert_profiles_uploaded do + middleware = AppProfiler::Middleware.new(app_env) + middleware.call(mock_request_env(path: "/?profile=#{mode}&backend=vernier")) + end + end + end + end + end + + if defined?(AppProfiler::VernierBackend) + test "the backend can be toggled between requests" do + assert_profiles_dumped(3) do + assert_profiles_uploaded do + middleware = AppProfiler::Middleware.new(app_env) + middleware.call(mock_request_env(path: "/?profile=wall&backend=stackprof")) + end + + assert_profiles_uploaded do + middleware = AppProfiler::Middleware.new(app_env) + middleware.call(mock_request_env(path: "/?profile=wall&backend=vernier")) + end + + assert_profiles_uploaded do + middleware = AppProfiler::Middleware.new(app_env) + middleware.call(mock_request_env(path: "/?profile=wall&backend=stackprof")) + end + + assert_equal(2, tmp_profiles.count { |p| p.to_s =~ /#{AppProfiler::StackprofProfile::FILE_EXTENSION}$/ }) + assert_equal(1, tmp_profiles.count { |p| p.to_s =~ /#{AppProfiler::VernierProfile::FILE_EXTENSION}$/ }) + end + end + end + test "profile interval is supported" do assert_profiles_dumped do assert_profiles_uploaded do @@ -145,7 +182,7 @@ class MiddlewareTest < TestCase end end - AppProfiler::Parameters::MODES.each do |mode| + AppProfiler::StackprofBackend::AVAILABLE_MODES.each do |mode| test "profile mode #{mode} through headers is supported" do assert_profiles_dumped do assert_profiles_uploaded do @@ -157,6 +194,20 @@ class MiddlewareTest < TestCase end end + if defined?(AppProfiler::VernierBackend) + AppProfiler::VernierBackend::AVAILABLE_MODES.each do |mode| + test "profile mode #{mode} is supported through headers by vernier backend" do + assert_profiles_dumped do + assert_profiles_uploaded do + middleware = AppProfiler::Middleware.new(app_env) + opt = { AppProfiler.request_profile_header => "mode=#{mode};backend=vernier" } + middleware.call(mock_request_env(opt: opt)) + end + end + end + end + end + test "profile interval through headers is supported" do assert_profiles_dumped do assert_profiles_uploaded do diff --git a/test/app_profiler/profile_test.rb b/test/app_profiler/profile/stackprof_test.rb similarity index 78% rename from test/app_profiler/profile_test.rb rename to test/app_profiler/profile/stackprof_test.rb index d66f34e6..ad55a693 100644 --- a/test/app_profiler/profile_test.rb +++ b/test/app_profiler/profile/stackprof_test.rb @@ -3,7 +3,7 @@ require "test_helper" module AppProfiler - class ProfileTest < TestCase + class StackprofProfileTest < TestCase test ".from_stackprof raises ArgumentError when mode is not present" do error = assert_raises(ArgumentError) do profile_without_mode = stackprof_profile.tap { |data| data.delete(:mode) } @@ -36,7 +36,7 @@ class ProfileTest < TestCase end test "#id" do - profile = Profile.new(stackprof_profile, id: "pass") + profile = StackprofProfile.new(stackprof_profile, id: "pass") assert_equal("pass", profile.id) end @@ -44,7 +44,7 @@ class ProfileTest < TestCase test "#id is random hex by default" do SecureRandom.expects(:hex).returns("mock") - profile = Profile.new(stackprof_profile) + profile = StackprofProfile.new(stackprof_profile) assert_equal("mock", profile.id) end @@ -52,46 +52,45 @@ class ProfileTest < TestCase test "#id is random hex when passed as empty string" do SecureRandom.expects(:hex).returns("mock") - profile = Profile.new(stackprof_profile, id: "") + profile = StackprofProfile.new(stackprof_profile, id: "") assert_equal("mock", profile.id) end test "#context" do - profile = Profile.new(stackprof_profile, context: "development") + profile = StackprofProfile.new(stackprof_profile, context: "development") assert_equal("development", profile.context) end test "#valid? is false when mode is not present" do - profile = Profile.new({}) + profile = StackprofProfile.new({}) assert_not_predicate(profile, :valid?) end test "#valid? is true when mode is present" do - profile = Profile.new({ mode: :cpu }) + profile = StackprofProfile.new({ mode: :cpu }) assert_predicate(profile, :valid?) end test "#mode" do - profile = Profile.new(stackprof_profile(mode: "object")) + profile = StackprofProfile.new(stackprof_profile(mode: "object")) assert_equal("object", profile.mode) end test "#view" do - profile = Profile.new(stackprof_profile) + profile = StackprofProfile.new(stackprof_profile) - AppProfiler.stubs(:viewer).returns(Viewer::SpeedscopeViewer) Viewer::SpeedscopeViewer.expects(:view).with(profile) profile.view end test "#upload" do - profile = Profile.new(stackprof_profile) + profile = StackprofProfile.new(stackprof_profile) AppProfiler.stubs(:storage).returns(MockStorage) MockStorage.expects(:upload).with(profile).returns("some data") @@ -100,7 +99,7 @@ class ProfileTest < TestCase end test "#upload returns nil if an error was raised" do - profile = Profile.new(stackprof_profile) + profile = StackprofProfile.new(stackprof_profile) AppProfiler.storage.stubs(:upload).raises(StandardError, "upload error") @@ -109,14 +108,14 @@ class ProfileTest < TestCase test "#file creates json file" do profile_data = stackprof_profile(mode: "wall") - profile = Profile.new(profile_data) + profile = StackprofProfile.new(profile_data) assert_match(/.*\.json/, profile.file.to_s) assert_equal(profile_data, JSON.parse(profile.file.read, symbolize_names: true)) end test "#file creates file only once" do - profile = Profile.new(stackprof_profile) + profile = StackprofProfile.new(stackprof_profile) assert_predicate(profile.file, :exist?) @@ -125,15 +124,8 @@ class ProfileTest < TestCase assert_not_predicate(profile.file, :exist?) end - test "#to_h returns profile data" do - profile_data = stackprof_profile - profile = Profile.new(profile_data) - - assert_equal(profile_data, profile.to_h) - end - test "#[] forwards to profile data" do - profile = Profile.new(stackprof_profile(interval: 10_000)) + profile = StackprofProfile.new(stackprof_profile(interval: 10_000)) assert_equal(10_000, profile[:interval]) end @@ -146,7 +138,7 @@ class ProfileTest < TestCase end test "#file uses custom profile_file_prefix block when provided" do - profile = Profile.new(stackprof_profile) + profile = StackprofProfile.new(stackprof_profile) AppProfiler.stubs(:profile_file_prefix).returns(-> { "want-something-different" }) assert_match(/want-something-different-/, File.basename(profile.file.to_s)) @@ -154,7 +146,7 @@ class ProfileTest < TestCase test "#file uses default prefix format when no custom profile_file_prefix block is provided" do travel_to Time.zone.local(2022, 10, 06, 12, 11, 10) do - profile = Profile.new(stackprof_profile) + profile = StackprofProfile.new(stackprof_profile) assert_match(/^20221006-121110/, File.basename(profile.file.to_s)) end end diff --git a/test/app_profiler/profile/vernier_test.rb b/test/app_profiler/profile/vernier_test.rb new file mode 100644 index 00000000..abb5c7de --- /dev/null +++ b/test/app_profiler/profile/vernier_test.rb @@ -0,0 +1,161 @@ +# frozen_string_literal: true + +require "test_helper" + +module AppProfiler + class VernierProfileTest < TestCase + # test ".from_vernier raises ArgumentError when mode is not present" do + # error = assert_raises(ArgumentError) do + # profile_without_mode = vernier_profile.tap { |data| data.data.delete(:mode) } + # Profile.from_vernier(profile_without_mode) + # end + # assert_equal("invalid profile data", error.message) + # end + + test ".from_vernier assigns id and context metadata" do + profile = Profile.from_vernier(vernier_profile(metadata: { id: "foo", context: "bar" })) + + assert_equal("foo", profile.id) + assert_equal("bar", profile.context) + end + + test ".from_vernier assigns random id when id is not present" do + SecureRandom.expects(:hex).returns("mock") + + params_without_id = vernier_profile(vernier_params).tap { |data| data.meta.delete(:id) } + profile = Profile.from_vernier(params_without_id) + + assert_equal("mock", profile.id) + end + + # test ".from_vernier removes id and context metadata from profile data" do + # profile = Profile.from_vernier(vernier_profile(metadata: { id: "foo", context: "bar" })) + + # assert_not_operator(profile[:metadata], :key?, :id) + # assert_not_operator(profile[:metadata], :key?, :context) + # end + + test "#id" do + profile = VernierProfile.new(vernier_profile, id: "pass") + + assert_equal("pass", profile.id) + end + + test "#id is random hex by default" do + SecureRandom.expects(:hex).returns("mock") + + profile = VernierProfile.new(vernier_profile) + + assert_equal("mock", profile.id) + end + + test "#id is random hex when passed as empty string" do + SecureRandom.expects(:hex).returns("mock") + + profile = VernierProfile.new(vernier_profile, id: "") + + assert_equal("mock", profile.id) + end + + test "#context" do + profile = VernierProfile.new(vernier_profile, context: "development") + + assert_equal("development", profile.context) + end + + # test "#valid? is false when mode is not present" do + # profile = VernierProfile.new(vernier_profile) + + # assert_not_predicate(profile, :valid?) + # end + + test "#valid? is true when mode is present" do + profile = VernierProfile.new(vernier_profile({ mode: :cpu })) + + assert_predicate(profile, :valid?) + end + + test "#mode" do + profile = VernierProfile.new(vernier_profile(metadata: { mode: "retained" })) + + assert_equal("retained", profile.mode) + end + + test "#view" do + profile = VernierProfile.new(vernier_profile) + + Viewer::FirefoxViewer.expects(:view).with(profile) + + profile.view + end + + test "#upload" do + profile = VernierProfile.new(vernier_profile) + + AppProfiler.stubs(:storage).returns(MockStorage) + MockStorage.expects(:upload).with(profile).returns("some data") + + assert_equal("some data", profile.upload) + end + + test "#upload returns nil if an error was raised" do + profile = VernierProfile.new(vernier_profile) + + AppProfiler.storage.stubs(:upload).raises(StandardError, "upload error") + + assert_nil(profile.upload) + end + + test "#file creates json file" do + profile_data = vernier_profile(mode: "wall") + profile = VernierProfile.new(profile_data) + + assert_match(/.*\.json/, profile.file.to_s) + assert_equal(profile_data.to_h, JSON.parse(profile.file.read, symbolize_names: true)) + end + + test "#file creates file only once" do + profile = VernierProfile.new(vernier_profile) + + assert_predicate(profile.file, :exist?) + + profile.file.delete + + assert_not_predicate(profile.file, :exist?) + end + + test "#to_h returns profile data" do + profile_data = vernier_profile + profile = VernierProfile.new(profile_data) + + assert_equal(profile_data.to_h, profile.to_h) + end + + test "#[] forwards to profile metadata" do + profile = VernierProfile.new(vernier_profile(metadata: { interval: 10_000 })) + + assert_equal(10_000, profile[:interval]) + end + + test "#path raises an UnsafeFilename exception given chars not in allow list" do + assert_raises(AppProfiler::Profile::UnsafeFilename) do + profile = Profile.from_vernier(vernier_profile(metadata: { id: "|`@${}", context: "bar" })) + profile.file + end + end + + test "#file uses custom profile_file_prefix block when provided" do + profile = VernierProfile.new(vernier_profile) + + AppProfiler.stubs(:profile_file_prefix).returns(-> { "want-something-different" }) + assert_match(/want-something-different-/, File.basename(profile.file.to_s)) + end + + test "#file uses default prefix format when no custom profile_file_prefix block is provided" do + travel_to Time.zone.local(2022, 10, 06, 12, 11, 10) do + profile = VernierProfile.new(vernier_profile) + assert_match(/^20221006-121110/, File.basename(profile.file.to_s)) + end + end + end +end diff --git a/test/app_profiler/profiler_test.rb b/test/app_profiler/profiler_test.rb deleted file mode 100644 index c902729a..00000000 --- a/test/app_profiler/profiler_test.rb +++ /dev/null @@ -1,185 +0,0 @@ -# frozen_string_literal: true - -require "test_helper" - -module AppProfiler - class ProfilerTest < TestCase - test ".run prints error when failed" do - AppProfiler.logger.expects(:info).with { |value| value =~ /failed to start the profiler/ } - profile = Profiler.run(mode: :unsupported) do - sleep(0.1) - end - - assert_nil(profile) - end - - test ".run raises when yield raises" do - error = StandardError.new("An error occurred.") - exception = assert_raises(StandardError) do - Profiler.run(stackprof_profile) do - assert_predicate(Profiler, :running?) - raise error - end - end - - assert_equal(error, exception) - assert_not_predicate(Profiler, :running?) - end - - test ".run does not stop the profiler when it is already running" do - AppProfiler.logger.expects(:info).never - - assert_equal(true, Profiler.send(:start, stackprof_profile)) - - profile = Profiler.run(stackprof_profile) do - sleep(0.1) - end - - assert_nil(profile) - assert_predicate(Profiler, :running?) - ensure - StackProf.stop - end - - test ".run uses cpu profile by default" do - profile = Profiler.run(stackprof_profile) do - sleep(0.1) - end - - assert_instance_of(AppProfiler::Profile, profile) - assert_equal(:cpu, profile[:mode]) - assert_equal(1000, profile[:interval]) - end - - test ".run assigns metadata to profiles" do - profile = Profiler.run(stackprof_profile(metadata: { id: "wowza", context: "bar" })) do - sleep(0.1) - end - - assert_instance_of(AppProfiler::Profile, profile) - assert_equal("wowza", profile.id) - assert_equal("bar", profile.context) - end - - test ".run cpu profile" do - profile = Profiler.run(stackprof_profile(mode: :cpu, interval: 2000)) do - sleep(0.1) - end - - assert_instance_of(AppProfiler::Profile, profile) - assert_equal(:cpu, profile[:mode]) - assert_equal(2000, profile[:interval]) - end - - test ".run wall profile" do - profile = Profiler.run(stackprof_profile(mode: :wall, interval: 2000)) do - sleep(0.1) - end - - assert_instance_of(AppProfiler::Profile, profile) - assert_equal(:wall, profile[:mode]) - assert_equal(2000, profile[:interval]) - end - - test ".run object profile" do - profile = Profiler.run(stackprof_profile(mode: :object, interval: 2)) do - sleep(0.1) - end - - assert_instance_of(AppProfiler::Profile, profile) - assert_equal(:object, profile[:mode]) - assert_equal(2, profile[:interval]) - end - - test ".start uses cpu profile by default" do - Profiler.start(stackprof_profile) - Profiler.stop - - profile = Profiler.results - - assert_instance_of(AppProfiler::Profile, profile) - assert_equal(:cpu, profile[:mode]) - assert_equal(1000, profile[:interval]) - end - - test ".start assigns metadata to profiles" do - Profiler.start(stackprof_profile(metadata: { id: "wowza", context: "bar" })) - Profiler.stop - - profile = Profiler.results - - assert_instance_of(AppProfiler::Profile, profile) - assert_equal("wowza", profile.id) - assert_equal("bar", profile.context) - end - - test ".start cpu profile" do - Profiler.start(stackprof_profile(mode: :cpu, interval: 2000)) - Profiler.stop - - profile = Profiler.results - - assert_instance_of(AppProfiler::Profile, profile) - assert_equal(:cpu, profile[:mode]) - assert_equal(2000, profile[:interval]) - end - - test ".start wall profile" do - Profiler.start(stackprof_profile(mode: :wall, interval: 2000)) - Profiler.stop - - profile = Profiler.results - - assert_instance_of(AppProfiler::Profile, profile) - assert_equal(:wall, profile[:mode]) - assert_equal(2000, profile[:interval]) - end - - test ".start object profile" do - Profiler.start(stackprof_profile(mode: :object, interval: 2)) - Profiler.stop - - profile = Profiler.results - - assert_instance_of(AppProfiler::Profile, profile) - assert_equal(:object, profile[:mode]) - assert_equal(2, profile[:interval]) - end - - test ".stop" do - StackProf.expects(:stop) - AppProfiler.stop - end - - test ".results prints error when failed" do - Profiler.expects(:stackprof_results).returns({}) - AppProfiler.logger.expects(:info).with { |value| value =~ /failed to obtain the profile/ } - - assert_nil(Profiler.results) - end - - test ".results returns nil when profiling is still active" do - Profiler.run(stackprof_profile) do - assert_nil(Profiler.results) - end - end - - test ".start, .stop, and .results interact well" do - AppProfiler.logger.expects(:info).never - - assert_equal(true, Profiler.start(stackprof_profile)) - assert_equal(false, Profiler.start(stackprof_profile)) - assert_equal(true, Profiler.send(:running?)) - assert_nil(Profiler.results) - assert_equal(true, Profiler.stop) - assert_equal(false, Profiler.stop) - assert_equal(false, Profiler.send(:running?)) - - profile = Profiler.results - assert_instance_of(AppProfiler::Profile, profile) - assert_predicate(profile, :valid?) - - assert_nil(Profiler.results) - end - end -end diff --git a/test/app_profiler/request_parameters_test.rb b/test/app_profiler/request_parameters_test.rb index 544dc2f2..19215305 100644 --- a/test/app_profiler/request_parameters_test.rb +++ b/test/app_profiler/request_parameters_test.rb @@ -19,8 +19,8 @@ class RequestParametersTest < TestCase test "#valid? returns false when interval is less than allowed" do AppProfiler.logger.expects(:info).never - AppProfiler::Parameters::MIN_INTERVALS.each do |mode, interval| - interval -= 1 + AppProfiler::StackprofBackend::AVAILABLE_MODES.each do |mode| + interval = AppProfiler::Parameters::MIN_INTERVALS[mode.to_s] - 1 params = request_params(headers: { AppProfiler.request_profile_header => "mode=#{mode};interval=#{interval}", }) @@ -50,7 +50,8 @@ class RequestParametersTest < TestCase end test "#to_h return correct hash when request parameters are ok" do - AppProfiler::Parameters::DEFAULT_INTERVALS.each do |mode, interval| + AppProfiler::StackprofBackend::AVAILABLE_MODES.each do |mode| + interval = AppProfiler::Parameters::DEFAULT_INTERVALS[mode.to_s] params = request_params(headers: { AppProfiler.request_profile_header => "mode=#{mode};interval=#{interval};context=test;ignore_gc=1", "HTTP_X_REQUEST_ID" => "123", diff --git a/test/app_profiler/run_test.rb b/test/app_profiler/run_test.rb index b0d1a04f..62535787 100644 --- a/test/app_profiler/run_test.rb +++ b/test/app_profiler/run_test.rb @@ -4,16 +4,16 @@ module AppProfiler class RunTest < TestCase - test ".run delegates to Profiler.run" do - Profiler.expects(:run) + test ".run delegates to profiler.run" do + AppProfiler.profiler.expects(:run) AppProfiler.run do sleep 0.1 end end - test ".start delegates to Profiler.start" do - Profiler.expects(:start) + test ".start delegates to profiler.start" do + AppProfiler.profiler.expects(:start) AppProfiler.start end @@ -23,7 +23,7 @@ class RunTest < TestCase sleep 0.1 profile = AppProfiler.stop - assert_instance_of(Profile, profile) + assert_instance_of(StackprofProfile, profile) end end end diff --git a/test/test_helper.rb b/test/test_helper.rb index b3548601..9bf39499 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -37,6 +37,28 @@ config.profile_root = TMP_ROOT end +module Vernier + class FakeResult + attr_accessor :meta, :data + + def initialize(data) + @meta = data.delete(:metadata) || {} + @data = data + end + + def to_h + { + meta: @meta, + data: @data, + } + end + + def write(out:) + File.write(out, JSON.dump(to_h)) + end + end +end + module AppProfiler class Dummy < Rails::Application; end @@ -53,6 +75,14 @@ def stackprof_profile(params = {}) { mode: :cpu, interval: 1000, frames: [], metadata: { id: "foo" } }.merge(params) end + def vernier_profile(params = {}) + ::Vernier::FakeResult.new(params) + end + + def vernier_params(params = {}) + { mode: :wall, interval: 1000, metadata: { id: "foo" } }.merge(params) + end + def with_yarn_setup old_yarn_setup = Yarn::Command.yarn_setup Yarn::Command.yarn_setup = true From 5fbaa1d1b066482f29243b2a1136e961536c5825 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Mon, 22 Jan 2024 15:19:50 -0500 Subject: [PATCH 02/34] Fixup --- lib/app_profiler/profile.rb | 3 +++ test/app_profiler/profile/vernier_test.rb | 8 -------- .../speedscope_remote_viewer/middleware_test.rb | 10 +++++----- .../viewer/speedscope_remote_viewer_test.rb | 6 +++--- test/app_profiler/viewer/speedscope_viewer_test.rb | 12 ++++++------ 5 files changed, 17 insertions(+), 22 deletions(-) diff --git a/lib/app_profiler/profile.rb b/lib/app_profiler/profile.rb index 11e5f66a..eadeb112 100644 --- a/lib/app_profiler/profile.rb +++ b/lib/app_profiler/profile.rb @@ -1,6 +1,9 @@ # frozen_string_literal: true module AppProfiler + autoload :StackprofProfile, "app_profiler/profile/stackprof" + autoload :VernierProfile, "app_profiler/profile/vernier" + class Profile INTERNAL_METADATA_KEYS = [:id, :context] private_constant :INTERNAL_METADATA_KEYS diff --git a/test/app_profiler/profile/vernier_test.rb b/test/app_profiler/profile/vernier_test.rb index abb5c7de..75d45def 100644 --- a/test/app_profiler/profile/vernier_test.rb +++ b/test/app_profiler/profile/vernier_test.rb @@ -81,14 +81,6 @@ class VernierProfileTest < TestCase assert_equal("retained", profile.mode) end - test "#view" do - profile = VernierProfile.new(vernier_profile) - - Viewer::FirefoxViewer.expects(:view).with(profile) - - profile.view - end - test "#upload" do profile = VernierProfile.new(vernier_profile) diff --git a/test/app_profiler/viewer/speedscope_remote_viewer/middleware_test.rb b/test/app_profiler/viewer/speedscope_remote_viewer/middleware_test.rb index 4495f69d..18507c96 100644 --- a/test/app_profiler/viewer/speedscope_remote_viewer/middleware_test.rb +++ b/test/app_profiler/viewer/speedscope_remote_viewer/middleware_test.rb @@ -13,14 +13,14 @@ class MiddlewareTest < TestCase end test ".id" do - profile = Profile.new(stackprof_profile) + profile = Profile.from_stackprof(stackprof_profile) profile_id = profile.file.basename.to_s.delete_suffix(".json") assert_equal(profile_id, Middleware.id(profile.file)) end test "#call index" do - profiles = Array.new(3) { Profile.new(stackprof_profile).tap(&:file) } + profiles = Array.new(3) { Profile.from_stackprof(stackprof_profile).tap(&:file) } code, content_type, html = @app.call({ "PATH_INFO" => "/app_profiler" }) html = html.first @@ -34,7 +34,7 @@ class MiddlewareTest < TestCase end test "#call index with slash" do - profiles = Array.new(3) { Profile.new(stackprof_profile).tap(&:file) } + profiles = Array.new(3) { Profile.from_stackprof(stackprof_profile).tap(&:file) } code, content_type, html = @app.call({ "PATH_INFO" => "/app_profiler/" }) html = html.first @@ -48,7 +48,7 @@ class MiddlewareTest < TestCase end test "#call show" do - profile = Profile.new(stackprof_profile) + profile = Profile.from_stackprof(stackprof_profile) id = Middleware.id(profile.file) code, content_type, html = @app.call({ "PATH_INFO" => "/app_profiler/#{id}" }) @@ -62,7 +62,7 @@ class MiddlewareTest < TestCase test "#call show can serve huge payloads" do frames = { "1" => { name: "a" * 1e7 } } - profile = Profile.new(stackprof_profile(frames: frames)) + profile = Profile.from_stackprof(stackprof_profile(frames: frames)) id = Middleware.id(profile.file) _, _, html = @app.call({ "PATH_INFO" => "/app_profiler/#{id}" }) diff --git a/test/app_profiler/viewer/speedscope_remote_viewer_test.rb b/test/app_profiler/viewer/speedscope_remote_viewer_test.rb index 5b2cd71e..4982ca8d 100644 --- a/test/app_profiler/viewer/speedscope_remote_viewer_test.rb +++ b/test/app_profiler/viewer/speedscope_remote_viewer_test.rb @@ -8,12 +8,12 @@ class SpeedscopeRemoteViewerTest < TestCase test ".view initializes and calls #view" do SpeedscopeRemoteViewer.any_instance.expects(:view) - profile = Profile.new(stackprof_profile) + profile = Profile.from_stackprof(stackprof_profile) SpeedscopeRemoteViewer.view(profile) end test "#view logs middleware URL" do - profile = Profile.new(stackprof_profile) + profile = Profile.from_stackprof(stackprof_profile) viewer = SpeedscopeRemoteViewer.new(profile) id = SpeedscopeRemoteViewer::Middleware.id(profile.file) @@ -27,7 +27,7 @@ class SpeedscopeRemoteViewerTest < TestCase test "#view with response redirects to URL" do response = [200, {}, ["OK"]] - profile = Profile.new(stackprof_profile) + profile = Profile.from_stackprof(stackprof_profile) viewer = SpeedscopeRemoteViewer.new(profile) id = SpeedscopeRemoteViewer::Middleware.id(profile.file) diff --git a/test/app_profiler/viewer/speedscope_viewer_test.rb b/test/app_profiler/viewer/speedscope_viewer_test.rb index d2ce4dc4..0745af62 100644 --- a/test/app_profiler/viewer/speedscope_viewer_test.rb +++ b/test/app_profiler/viewer/speedscope_viewer_test.rb @@ -8,12 +8,12 @@ class SpeedscopeViewerTest < TestCase test ".view initializes and calls #view" do SpeedscopeViewer.any_instance.expects(:view) - profile = Profile.new(stackprof_profile) + profile = Profile.from_stackprof(stackprof_profile) SpeedscopeViewer.view(profile) end test "#view opens profile in speedscope and sets up yarn" do - profile = Profile.new(stackprof_profile) + profile = Profile.from_stackprof(stackprof_profile) viewer = SpeedscopeViewer.new(profile) viewer.expects(:system).with("which", "yarn", out: File::NULL).returns(true) @@ -31,7 +31,7 @@ class SpeedscopeViewerTest < TestCase end test "#view doesn't init when package.json exists" do - profile = Profile.new(stackprof_profile) + profile = Profile.from_stackprof(stackprof_profile) AppProfiler.root.mkpath AppProfiler.root.join("package.json").write("{}") @@ -52,7 +52,7 @@ class SpeedscopeViewerTest < TestCase end test "#view doesn't add when speedscope exists" do - profile = Profile.new(stackprof_profile) + profile = Profile.from_stackprof(stackprof_profile) AppProfiler.root.mkpath AppProfiler.root.join("node_modules/speedscope").mkpath @@ -71,7 +71,7 @@ class SpeedscopeViewerTest < TestCase end test "#view only opens profile in speedscope if yarn is already setup" do - profile = Profile.new(stackprof_profile) + profile = Profile.from_stackprof(stackprof_profile) with_yarn_setup do viewer = SpeedscopeViewer.new(profile) @@ -82,7 +82,7 @@ class SpeedscopeViewerTest < TestCase end test "#view raises YarnError when yarn command fails" do - profile = Profile.new(stackprof_profile) + profile = Profile.from_stackprof(stackprof_profile) viewer = SpeedscopeViewer.new(profile) viewer.expects(:system).returns(false) From a1d39b071b55920278feb76176fc8395736b0098 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Tue, 23 Jan 2024 11:45:26 -0500 Subject: [PATCH 03/34] Apply suggestions from code review Co-authored-by: Bassam Mansoob --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 5bf72f2d..2040d880 100644 --- a/README.md +++ b/README.md @@ -82,7 +82,7 @@ Rails.application.config.app_profiler.profile_header = "X-Profile" | ignore_gc | Ignore garbage collection frames | | | autoredirect | Redirect request automatically to Speedscope's page after profiling. | | | context | Directory within the specified bucket in the selected storage where raw profile data should be written. | Only supported in (2). Defaults to `Rails.env` if not specified. | -| backend | Profiler to use, either `stackprof` or `vernier`. Defaults to stackprof. Available only vernier is installed in the app, and Ruby version is at least 3.2.1 | +| backend | Profiler to use, either `stackprof` or `vernier`. Defaults to stackprof. Vernier is only available when added separately in the application Gemfile, and the Ruby version is at least 3.2.1 | Note that the `autoredirect` feature can be turned on for all requests by doing the following: @@ -318,7 +318,7 @@ Rails.application.config.app_profiler.middleware_action = AppProfiler::Middlewar It is possible to configure app_profiler to use the [`vernier`](https://github.com/jhawthorn/vernier) or stackprof. To use vernier, it must be added separately in the application Gemfile. -The backend can be selected at dynamicall runtime using the `backend` parameter. The default backend to use when this parameter is not specified can be configured with: +The backend can be selected dynamically at runtime using the `backend` parameter. The default backend to use when this parameter is not specified can be configured with: ```ruby AppProfiler.profiler_backend = AppProfiler::StackprofBackend # or AppProfiler::VernierBackend @@ -328,7 +328,7 @@ Rails.application.config.app_profiler.profiler_backend = AppProfiler::StackprofB By default, the stackprof backend will be used. -In local development, changing the backend will change whether the profile is viewed in speedscope, or firefox-profiler. +In local development, changing the backend will change whether the profile is viewed in speedscope or firefox-profiler. ## Running tests From e55a148142b2bcf8817e10567bf40cf71cca09b2 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Tue, 23 Jan 2024 13:19:27 -0500 Subject: [PATCH 04/34] Move backend updating code into main AppProfiler module --- lib/app_profiler.rb | 12 ++++++++++++ lib/app_profiler/middleware.rb | 33 +++++++++++++-------------------- test/app_profiler/run_test.rb | 23 +++++++++++++++++++++++ 3 files changed, 48 insertions(+), 20 deletions(-) diff --git a/lib/app_profiler.rb b/lib/app_profiler.rb index 24a739c2..e6064625 100644 --- a/lib/app_profiler.rb +++ b/lib/app_profiler.rb @@ -88,6 +88,18 @@ def profiler @backend ||= profiler_backend.new end + def backend=(backend) + return true if @backend&.is_a?(backend) + + if running? + raise ArgumentError, + "cannot change backend to #{backend::NAME} while #{profiler_backend::NAME} backend is running" + end + + clear + self.profiler_backend = backend + end + def clear @backend.stop if @backend&.running? @backend = nil diff --git a/lib/app_profiler/middleware.rb b/lib/app_profiler/middleware.rb index 42f39c75..cef4b41c 100644 --- a/lib/app_profiler/middleware.rb +++ b/lib/app_profiler/middleware.rb @@ -12,7 +12,6 @@ class Middleware def initialize(app) @app = app - @backend_lock = Mutex.new end def call(env, params = AppProfiler::RequestParameters.new(Rack::Request.new(env))) @@ -33,20 +32,15 @@ def profile(env, params) return yield unless before_profile(env, params_hash) - @backend_lock.synchronize do - if params.backend && - AppProfiler.profiler_backend.name.split("::").last.downcase.gsub("backend", "") != params.backend - raise ArgumentError if AppProfiler.running? - - orig_backend = AppProfiler.profiler_backend - if defined?(AppProfiler::VernierBackend) && - params.backend == AppProfiler::VernierBackend::NAME - AppProfiler.profiler_backend = AppProfiler::VernierBackend - elsif params.backend == AppProfiler::StackprofBackend::NAME - AppProfiler.profiler_backend = AppProfiler::StackprofBackend - else - raise ArgumentError - end + if params.backend + orig_backend = AppProfiler.profiler_backend + if defined?(AppProfiler::VernierBackend) && + params.backend == AppProfiler::VernierBackend::NAME + return yield unless (AppProfiler.backend = AppProfiler::VernierBackend) + elsif params.backend == AppProfiler::StackprofBackend::NAME + return yield unless (AppProfiler.backend = AppProfiler::StackprofBackend) + else + raise ArgumentError end end @@ -65,11 +59,10 @@ def profile(env, params) response ensure - if orig_backend - @backend_lock.synchronize do - AppProfiler.profiler_backend = orig_backend - end - end + AppProfiler.backend = orig_backend if orig_backend + end + + def update_backend end def before_profile(_env, _params) diff --git a/test/app_profiler/run_test.rb b/test/app_profiler/run_test.rb index 62535787..2c77d904 100644 --- a/test/app_profiler/run_test.rb +++ b/test/app_profiler/run_test.rb @@ -25,5 +25,28 @@ class RunTest < TestCase assert_instance_of(StackprofProfile, profile) end + + test ".backend= fails to update the backend if already profiling" do + skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend) + assert(AppProfiler.backend = AppProfiler::StackprofBackend) + AppProfiler.start + assert(AppProfiler.running?) + assert_raises(ArgumentError) { AppProfiler.backend = AppProfiler::VernierBackend } + ensure + AppProfiler.stop + end + + test ".backend= updates the backend if not already profiling" do + orig_backend = AppProfiler.profiler_backend + skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend) + refute(AppProfiler.running?) + assert(AppProfiler.backend = AppProfiler::StackprofBackend) + assert_equal(AppProfiler.profiler_backend, AppProfiler::StackprofBackend) + refute(AppProfiler.running?) + assert(AppProfiler.backend = AppProfiler::VernierBackend) + assert_equal(AppProfiler.profiler_backend, AppProfiler::VernierBackend) + ensure + AppProfiler.backend = orig_backend + end end end From 914334c93601fccd8619b282d2f50feba16e7200 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Wed, 24 Jan 2024 11:59:52 -0500 Subject: [PATCH 05/34] Refactor access to setting backend profiler_backend is now private, and can only be read and updated through: - AppProfiler.backend= - AppProfiler.backend A new helper method AppProfiler.with_backend is used to wrap singular access to the backend. --- lib/app_profiler.rb | 39 +++++++++++----- lib/app_profiler/backend.rb | 14 +++--- lib/app_profiler/middleware.rb | 22 ++-------- lib/app_profiler/railtie.rb | 2 +- lib/app_profiler/request_parameters.rb | 2 +- test/app_profiler/backend.rb | 20 --------- .../backend/stackprof_backend_test.rb | 2 +- .../backend/vernier_backend_test.rb | 6 +-- test/app_profiler/backend_test.rb | 44 +++++++++++++++++++ test/app_profiler/run_test.rb | 23 ---------- 10 files changed, 87 insertions(+), 87 deletions(-) delete mode 100644 test/app_profiler/backend.rb create mode 100644 test/app_profiler/backend_test.rb diff --git a/lib/app_profiler.rb b/lib/app_profiler.rb index e6064625..d87e8c08 100644 --- a/lib/app_profiler.rb +++ b/lib/app_profiler.rb @@ -47,7 +47,6 @@ module Viewer mattr_accessor :context, default: nil mattr_reader :profile_url_formatter, default: DefaultProfileFormatter - mattr_accessor :profiler_backend, default: AppProfiler::StackprofBackend mattr_accessor :storage, default: Storage::FileStorage mattr_accessor :viewer, default: Viewer::SpeedscopeViewer @@ -79,25 +78,41 @@ def running? end def profiler - if @backend && !@backend.is_a?(profiler_backend) - raise ConfigurationError if @backend.running? - - clear - end - - @backend ||= profiler_backend.new + @backend ||= backend.new end - def backend=(backend) - return true if @backend&.is_a?(backend) + def backend=(new_backend) + return true if backend&.is_a?(new_backend) if running? raise ArgumentError, - "cannot change backend to #{backend::NAME} while #{profiler_backend::NAME} backend is running" + "cannot change backend to #{new_backend::NAME} while #{backend::NAME} backend is running" end clear - self.profiler_backend = backend + @profiler_backend = new_backend + end + + def backend + @profiler_backend ||= DefaultBackend + end + + def with_backend(backend) + orig_backend = AppProfiler.backend + + if backend + if defined?(AppProfiler::VernierBackend) && + backend == AppProfiler::VernierBackend::NAME + return yield unless (AppProfiler.backend = AppProfiler::VernierBackend) + elsif backend == AppProfiler::StackprofBackend::NAME + return yield unless (AppProfiler.backend = AppProfiler::StackprofBackend) + else + return yield + end + end + yield + ensure + AppProfiler.backend = orig_backend end def clear diff --git a/lib/app_profiler/backend.rb b/lib/app_profiler/backend.rb index ddee219e..db73a92d 100644 --- a/lib/app_profiler/backend.rb +++ b/lib/app_profiler/backend.rb @@ -22,12 +22,12 @@ def running? raise NotImplementedError end end -end - -require "app_profiler/backend/stackprof" -begin - require "app_profiler/backend/vernier" -rescue LoadError - warn("Vernier is not supported.") + autoload :StackprofBackend, "app_profiler/backend/stackprof" + begin + autoload(:VernierBackend, "app_profiler/backend/vernier") + rescue LoadError + warn("Vernier is not supported.") + end + DefaultBackend = AppProfiler::StackprofBackend end diff --git a/lib/app_profiler/middleware.rb b/lib/app_profiler/middleware.rb index cef4b41c..84cfbbf0 100644 --- a/lib/app_profiler/middleware.rb +++ b/lib/app_profiler/middleware.rb @@ -24,7 +24,6 @@ def call(env, params = AppProfiler::RequestParameters.new(Rack::Request.new(env) def profile(env, params) response = nil - orig_backend = nil return yield unless params.valid? @@ -32,22 +31,12 @@ def profile(env, params) return yield unless before_profile(env, params_hash) - if params.backend - orig_backend = AppProfiler.profiler_backend - if defined?(AppProfiler::VernierBackend) && - params.backend == AppProfiler::VernierBackend::NAME - return yield unless (AppProfiler.backend = AppProfiler::VernierBackend) - elsif params.backend == AppProfiler::StackprofBackend::NAME - return yield unless (AppProfiler.backend = AppProfiler::StackprofBackend) - else - raise ArgumentError + profile = AppProfiler.with_backend(params.backend) do + AppProfiler.run(params_hash) do + response = yield end end - profile = AppProfiler.run(params_hash) do - response = yield - end - return response unless profile && after_profile(env, profile) action.call( @@ -58,11 +47,6 @@ def profile(env, params) ) response - ensure - AppProfiler.backend = orig_backend if orig_backend - end - - def update_backend end def before_profile(_env, _params) diff --git a/lib/app_profiler/railtie.rb b/lib/app_profiler/railtie.rb index 42b5562d..c816f2b1 100644 --- a/lib/app_profiler/railtie.rb +++ b/lib/app_profiler/railtie.rb @@ -40,7 +40,7 @@ class Railtie < Rails::Railtie AppProfiler.profile_enqueue_success = app.config.app_profiler.profile_enqueue_success AppProfiler.profile_enqueue_failure = app.config.app_profiler.profile_enqueue_failure AppProfiler.after_process_queue = app.config.app_profiler.after_process_queue - AppProfiler.profiler_backend = app.config.app_profiler.profiler_backend + AppProfiler.backend = app.config.app_profiler.profiler_backend || AppProfiler::DefaultBackend end initializer "app_profiler.add_middleware" do |app| diff --git a/lib/app_profiler/request_parameters.rb b/lib/app_profiler/request_parameters.rb index 8d43ad53..04f27847 100644 --- a/lib/app_profiler/request_parameters.rb +++ b/lib/app_profiler/request_parameters.rb @@ -18,7 +18,7 @@ def async def backend query_param("backend") || profile_header_param("backend") || - AppProfiler.profiler_backend.name.split("::").last.downcase.gsub("backend", "") + AppProfiler.backend::NAME end def valid? diff --git a/test/app_profiler/backend.rb b/test/app_profiler/backend.rb deleted file mode 100644 index efaee3f3..00000000 --- a/test/app_profiler/backend.rb +++ /dev/null @@ -1,20 +0,0 @@ -# frozen_string_literal: true - -require "test_helper" - -module AppProfiler - class BackendTest < TestCase - test ".clear is required to change backends" do - AppProfiler.profiler_backend = AppProfiler::StackprofBackend - assert_instance_of(StackprofBackend, AppProfiler.profiler) - - AppProfiler.profiler_backend = AppProfiler::VernierBackend - assert_raises(ConfigurationError) { AppProfiler.profiler } - - AppProfiler.clear - - AppProfiler.profiler_backend = AppProfiler::VernierBackend - assert_instance_of(VernierBackend, AppProfiler.profiler) - end - end -end diff --git a/test/app_profiler/backend/stackprof_backend_test.rb b/test/app_profiler/backend/stackprof_backend_test.rb index 9ed41f50..88e54877 100644 --- a/test/app_profiler/backend/stackprof_backend_test.rb +++ b/test/app_profiler/backend/stackprof_backend_test.rb @@ -5,7 +5,7 @@ module AppProfiler class StackprofBackendTest < TestCase def setup - AppProfiler.profiler_backend = AppProfiler::StackprofBackend + AppProfiler.backend = AppProfiler::StackprofBackend end def teardown diff --git a/test/app_profiler/backend/vernier_backend_test.rb b/test/app_profiler/backend/vernier_backend_test.rb index 25c95bc1..0d5f8f7d 100644 --- a/test/app_profiler/backend/vernier_backend_test.rb +++ b/test/app_profiler/backend/vernier_backend_test.rb @@ -8,12 +8,12 @@ module AppProfiler class VernierBackendTest < TestCase def setup AppProfiler.clear - @orig_backend = AppProfiler.profiler_backend - AppProfiler.profiler_backend = AppProfiler::VernierBackend + @orig_backend = AppProfiler.backend + AppProfiler.backend = AppProfiler::VernierBackend end def teardown - AppProfiler.profiler_backend = @orig_backend + AppProfiler.backend = @orig_backend AppProfiler.clear end diff --git a/test/app_profiler/backend_test.rb b/test/app_profiler/backend_test.rb new file mode 100644 index 00000000..4d8ec163 --- /dev/null +++ b/test/app_profiler/backend_test.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require "test_helper" + +module AppProfiler + class BackendTest < TestCase + test ".backend= fails to update the backend if already profiling" do + skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend) + assert(AppProfiler.backend = AppProfiler::StackprofBackend) + AppProfiler.start + assert(AppProfiler.running?) + assert_raises(ArgumentError) { AppProfiler.backend = AppProfiler::VernierBackend } + ensure + AppProfiler.stop + end + + test ".backend= updates the backend if not already profiling" do + orig_backend = AppProfiler.backend + skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend) + refute(AppProfiler.running?) + assert(AppProfiler.backend = AppProfiler::StackprofBackend) + assert_equal(AppProfiler.backend, AppProfiler::StackprofBackend) + refute(AppProfiler.running?) + assert(AppProfiler.backend = AppProfiler::VernierBackend) + assert_equal(AppProfiler.backend, AppProfiler::VernierBackend) + ensure + AppProfiler.backend = orig_backend + end + + test ".with_backend sets the backend then returns to the previous value" do + orig_backend = AppProfiler.backend + skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend) + + assert_equal(AppProfiler.backend, AppProfiler::StackprofBackend) + refute(AppProfiler.running?) + AppProfiler.with_backend(AppProfiler::VernierBackend::NAME) do + assert_equal(AppProfiler::VernierBackend, AppProfiler.backend) + end + assert_equal(AppProfiler.backend, AppProfiler::StackprofBackend) + ensure + AppProfiler.backend = orig_backend + end + end +end diff --git a/test/app_profiler/run_test.rb b/test/app_profiler/run_test.rb index 2c77d904..62535787 100644 --- a/test/app_profiler/run_test.rb +++ b/test/app_profiler/run_test.rb @@ -25,28 +25,5 @@ class RunTest < TestCase assert_instance_of(StackprofProfile, profile) end - - test ".backend= fails to update the backend if already profiling" do - skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend) - assert(AppProfiler.backend = AppProfiler::StackprofBackend) - AppProfiler.start - assert(AppProfiler.running?) - assert_raises(ArgumentError) { AppProfiler.backend = AppProfiler::VernierBackend } - ensure - AppProfiler.stop - end - - test ".backend= updates the backend if not already profiling" do - orig_backend = AppProfiler.profiler_backend - skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend) - refute(AppProfiler.running?) - assert(AppProfiler.backend = AppProfiler::StackprofBackend) - assert_equal(AppProfiler.profiler_backend, AppProfiler::StackprofBackend) - refute(AppProfiler.running?) - assert(AppProfiler.backend = AppProfiler::VernierBackend) - assert_equal(AppProfiler.profiler_backend, AppProfiler::VernierBackend) - ensure - AppProfiler.backend = orig_backend - end end end From 9aeae92928501f21180d8bf1c098b53b55d4b254 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Wed, 24 Jan 2024 14:01:50 -0500 Subject: [PATCH 06/34] Fix vernier load error --- Gemfile | 2 +- Gemfile.lock | 2 -- lib/app_profiler/backend.rb | 2 +- lib/app_profiler/backend/vernier.rb | 6 +++--- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/Gemfile b/Gemfile index 7b40c54a..b524710b 100644 --- a/Gemfile +++ b/Gemfile @@ -6,7 +6,7 @@ gemspec # Specify the same dependency sources as the application Gemfile gem("activesupport", "~> 5.2") gem("railties", "~> 5.2") -gem("vernier", "~> 0.3.1") if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new("3.2.1") +gem("vernier", "~> 0.4.0") if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new("3.2.1") gem("google-cloud-storage", "~> 1.21") gem("rubocop", require: false) diff --git a/Gemfile.lock b/Gemfile.lock index e61e3030..eb0db76e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -179,7 +179,6 @@ GEM thread_safe (~> 0.1) uber (0.1.0) unicode-display_width (2.1.0) - vernier (0.3.1) webrick (1.7.0) PLATFORMS @@ -198,7 +197,6 @@ DEPENDENCIES rubocop rubocop-performance rubocop-shopify - vernier (~> 0.3.1) BUNDLED WITH 2.4.18 diff --git a/lib/app_profiler/backend.rb b/lib/app_profiler/backend.rb index db73a92d..324d402f 100644 --- a/lib/app_profiler/backend.rb +++ b/lib/app_profiler/backend.rb @@ -25,7 +25,7 @@ def running? autoload :StackprofBackend, "app_profiler/backend/stackprof" begin - autoload(:VernierBackend, "app_profiler/backend/vernier") + require "app_profiler/backend/vernier" rescue LoadError warn("Vernier is not supported.") end diff --git a/lib/app_profiler/backend/vernier.rb b/lib/app_profiler/backend/vernier.rb index 3627e9f0..9843a688 100644 --- a/lib/app_profiler/backend/vernier.rb +++ b/lib/app_profiler/backend/vernier.rb @@ -1,11 +1,11 @@ # frozen_string_literal: true begin - gem("vernier", ">= 0.3.1") + gem("vernier", ">= 0.4.0") require "vernier" rescue LoadError - warn("Vernier profiling support requires the vernier gem, version 0.3.1 or later." \ - "Please add it to your Gemfile: `gem \"vernier\", \">= 0.3.1\"`") + warn("Vernier profiling support requires the vernier gem, version 0.4.0 or later." \ + "Please add it to your Gemfile: `gem \"vernier\", \">= 0.4.0\"`") raise end From 3b3c746a20a00e6196cd9468dfd6c64df7b8c1ee Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Wed, 24 Jan 2024 14:02:42 -0500 Subject: [PATCH 07/34] Raise when viewing vernier --- lib/app_profiler/profile/vernier.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/app_profiler/profile/vernier.rb b/lib/app_profiler/profile/vernier.rb index 975650a8..2fce2742 100644 --- a/lib/app_profiler/profile/vernier.rb +++ b/lib/app_profiler/profile/vernier.rb @@ -38,7 +38,7 @@ def format end def view(params = {}) - Viewer::FirefoxViewer.view(self, **params) + raise NotImplementedError end end end From 76558a92d77cfe5eb47afed49d8c4e4cc20263e6 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Wed, 24 Jan 2024 14:04:41 -0500 Subject: [PATCH 08/34] Readme fix --- README.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/README.md b/README.md index 2040d880..93515a42 100644 --- a/README.md +++ b/README.md @@ -328,8 +328,6 @@ Rails.application.config.app_profiler.profiler_backend = AppProfiler::StackprofB By default, the stackprof backend will be used. -In local development, changing the backend will change whether the profile is viewed in speedscope or firefox-profiler. - ## Running tests ``` From 9ea3d0f05fe759810f027d9b34911dc585934879 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Wed, 24 Jan 2024 15:18:47 -0500 Subject: [PATCH 09/34] Remove commented code --- Gemfile.lock | 2 ++ lib/app_profiler/profile.rb | 2 +- test/app_profiler/profile/vernier_test.rb | 21 --------------------- 3 files changed, 3 insertions(+), 22 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index eb0db76e..e5fbff96 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -179,6 +179,7 @@ GEM thread_safe (~> 0.1) uber (0.1.0) unicode-display_width (2.1.0) + vernier (0.4.0) webrick (1.7.0) PLATFORMS @@ -197,6 +198,7 @@ DEPENDENCIES rubocop rubocop-performance rubocop-shopify + vernier (~> 0.4.0) BUNDLED WITH 2.4.18 diff --git a/lib/app_profiler/profile.rb b/lib/app_profiler/profile.rb index eadeb112..0d99530b 100644 --- a/lib/app_profiler/profile.rb +++ b/lib/app_profiler/profile.rb @@ -21,7 +21,7 @@ def self.from_stackprof(data) end def self.from_vernier(data) - # FIXME: we don't delete here, that is causing a segfault in vernier. Divergent behaviour from stackprof, + # NB: we don't delete here, that is causing a segfault in vernier. Divergent behaviour from stackprof, # as the special metadata keys "id" and "context" are preserved into the metadata, but maybe that isn't so bad. options = INTERNAL_METADATA_KEYS.map { |key| [key, data.meta.clone[key]] }.to_h diff --git a/test/app_profiler/profile/vernier_test.rb b/test/app_profiler/profile/vernier_test.rb index 75d45def..77f80f10 100644 --- a/test/app_profiler/profile/vernier_test.rb +++ b/test/app_profiler/profile/vernier_test.rb @@ -4,14 +4,6 @@ module AppProfiler class VernierProfileTest < TestCase - # test ".from_vernier raises ArgumentError when mode is not present" do - # error = assert_raises(ArgumentError) do - # profile_without_mode = vernier_profile.tap { |data| data.data.delete(:mode) } - # Profile.from_vernier(profile_without_mode) - # end - # assert_equal("invalid profile data", error.message) - # end - test ".from_vernier assigns id and context metadata" do profile = Profile.from_vernier(vernier_profile(metadata: { id: "foo", context: "bar" })) @@ -28,13 +20,6 @@ class VernierProfileTest < TestCase assert_equal("mock", profile.id) end - # test ".from_vernier removes id and context metadata from profile data" do - # profile = Profile.from_vernier(vernier_profile(metadata: { id: "foo", context: "bar" })) - - # assert_not_operator(profile[:metadata], :key?, :id) - # assert_not_operator(profile[:metadata], :key?, :context) - # end - test "#id" do profile = VernierProfile.new(vernier_profile, id: "pass") @@ -63,12 +48,6 @@ class VernierProfileTest < TestCase assert_equal("development", profile.context) end - # test "#valid? is false when mode is not present" do - # profile = VernierProfile.new(vernier_profile) - - # assert_not_predicate(profile, :valid?) - # end - test "#valid? is true when mode is present" do profile = VernierProfile.new(vernier_profile({ mode: :cpu })) From a1c938c743e5ffde10c6878b80ab742511c79c76 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Thu, 25 Jan 2024 11:17:30 -0500 Subject: [PATCH 10/34] Pull with_backend code into AppProfiler.run --- lib/app_profiler.rb | 58 +++++++++++++++++++------------ lib/app_profiler/middleware.rb | 6 ++-- test/app_profiler/backend_test.rb | 35 +++++++++++++++---- test/app_profiler/run_test.rb | 14 ++++++++ 4 files changed, 80 insertions(+), 33 deletions(-) diff --git a/lib/app_profiler.rb b/lib/app_profiler.rb index d87e8c08..c0cdd535 100644 --- a/lib/app_profiler.rb +++ b/lib/app_profiler.rb @@ -9,6 +9,9 @@ module AppProfiler class ConfigurationError < StandardError end + class BackendError < StandardError + end + DefaultProfileFormatter = proc do |upload| "#{AppProfiler.speedscope_host}#profileURL=#{upload.url}" end @@ -60,8 +63,16 @@ module Viewer mattr_reader :after_process_queue, default: nil class << self - def run(*args, &block) - profiler.run(*args, &block) + def run(*args, with_backend: nil, **kwargs, &block) + orig_backend = backend + begin + self.backend = with_backend if with_backend + profiler.run(*args, **kwargs, &block) + rescue BackendError + yield + end + ensure + AppProfiler.backend = orig_backend end def start(*args) @@ -82,37 +93,38 @@ def profiler end def backend=(new_backend) - return true if backend&.is_a?(new_backend) + new_profiler_backend = if new_backend.is_a?(String) + backend_for(new_backend) + elsif new_backend < Backend + new_backend + else + raise BackendError, "unsupportend backend type #{new_backend.class}" + end if running? - raise ArgumentError, + raise BackendError, "cannot change backend to #{new_backend::NAME} while #{backend::NAME} backend is running" end + return if @profiler_backend == new_backend + clear - @profiler_backend = new_backend + @profiler_backend = new_profiler_backend end - def backend - @profiler_backend ||= DefaultBackend + def backend_for(backend_name) + if defined?(AppProfiler::VernierBackend) && + backend_name == AppProfiler::VernierBackend::NAME + AppProfiler::VernierBackend + elsif backend_name == AppProfiler::StackprofBackend::NAME + AppProfiler::StackprofBackend + else + raise BackendError, "unknown backend #{backend_name}" + end end - def with_backend(backend) - orig_backend = AppProfiler.backend - - if backend - if defined?(AppProfiler::VernierBackend) && - backend == AppProfiler::VernierBackend::NAME - return yield unless (AppProfiler.backend = AppProfiler::VernierBackend) - elsif backend == AppProfiler::StackprofBackend::NAME - return yield unless (AppProfiler.backend = AppProfiler::StackprofBackend) - else - return yield - end - end - yield - ensure - AppProfiler.backend = orig_backend + def backend + @profiler_backend ||= DefaultBackend end def clear diff --git a/lib/app_profiler/middleware.rb b/lib/app_profiler/middleware.rb index 84cfbbf0..53eff30d 100644 --- a/lib/app_profiler/middleware.rb +++ b/lib/app_profiler/middleware.rb @@ -31,10 +31,8 @@ def profile(env, params) return yield unless before_profile(env, params_hash) - profile = AppProfiler.with_backend(params.backend) do - AppProfiler.run(params_hash) do - response = yield - end + profile = AppProfiler.run(params_hash, with_backend: params.backend) do + response = yield end return response unless profile && after_profile(env, profile) diff --git a/test/app_profiler/backend_test.rb b/test/app_profiler/backend_test.rb index 4d8ec163..68a950d2 100644 --- a/test/app_profiler/backend_test.rb +++ b/test/app_profiler/backend_test.rb @@ -9,7 +9,7 @@ class BackendTest < TestCase assert(AppProfiler.backend = AppProfiler::StackprofBackend) AppProfiler.start assert(AppProfiler.running?) - assert_raises(ArgumentError) { AppProfiler.backend = AppProfiler::VernierBackend } + assert_raises(BackendError) { AppProfiler.backend = AppProfiler::VernierBackend } ensure AppProfiler.stop end @@ -27,18 +27,41 @@ class BackendTest < TestCase AppProfiler.backend = orig_backend end - test ".with_backend sets the backend then returns to the previous value" do + test ".backend= accepts a string with the backend name" do orig_backend = AppProfiler.backend skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend) - + refute(AppProfiler.running?) + assert(AppProfiler.backend = AppProfiler::StackprofBackend::NAME) assert_equal(AppProfiler.backend, AppProfiler::StackprofBackend) refute(AppProfiler.running?) - AppProfiler.with_backend(AppProfiler::VernierBackend::NAME) do - assert_equal(AppProfiler::VernierBackend, AppProfiler.backend) - end + assert(AppProfiler.backend = AppProfiler::VernierBackend::NAME) + assert_equal(AppProfiler.backend, AppProfiler::VernierBackend) + ensure + AppProfiler.backend = orig_backend + end + + test ".backend= accepts a backend class" do + orig_backend = AppProfiler.backend + skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend) + refute(AppProfiler.running?) + assert(AppProfiler.backend = AppProfiler::StackprofBackend) assert_equal(AppProfiler.backend, AppProfiler::StackprofBackend) + refute(AppProfiler.running?) + assert(AppProfiler.backend = AppProfiler::VernierBackend) + assert_equal(AppProfiler.backend, AppProfiler::VernierBackend) ensure AppProfiler.backend = orig_backend end + + test ".backend_for= provides the backend class given a string" do + assert_equal(AppProfiler::StackprofBackend, AppProfiler.backend_for(AppProfiler::StackprofBackend::NAME)) + return unless defined?(AppProfiler::VernierBackend) + + assert_equal(AppProfiler::VernierBackend, AppProfiler.backend_for(AppProfiler::VernierBackend::NAME)) + end + + test ".backend_for= raises if an unknown backend is requested" do + assert_raises(BackendError) { AppProfiler.backend_for("not a real backend") } + end end end diff --git a/test/app_profiler/run_test.rb b/test/app_profiler/run_test.rb index 62535787..5b95936d 100644 --- a/test/app_profiler/run_test.rb +++ b/test/app_profiler/run_test.rb @@ -25,5 +25,19 @@ class RunTest < TestCase assert_instance_of(StackprofProfile, profile) end + + test ".run sets the backend then returns to the previous value" do + orig_backend = AppProfiler.backend + skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend) + + assert_equal(AppProfiler.backend, AppProfiler::StackprofBackend) + refute(AppProfiler.running?) + AppProfiler.run(with_backend: AppProfiler::VernierBackend) do + assert_equal(AppProfiler::VernierBackend, AppProfiler.backend) + end + assert_equal(AppProfiler.backend, AppProfiler::StackprofBackend) + ensure + AppProfiler.backend = orig_backend + end end end From 29e278b7588f44b2dc36aaecb6c6e86a58d82c75 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Thu, 25 Jan 2024 13:10:35 -0500 Subject: [PATCH 11/34] Lock around AppProfiler.run --- lib/app_profiler.rb | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/app_profiler.rb b/lib/app_profiler.rb index c0cdd535..f8791ac7 100644 --- a/lib/app_profiler.rb +++ b/lib/app_profiler.rb @@ -64,6 +64,7 @@ module Viewer class << self def run(*args, with_backend: nil, **kwargs, &block) + yield unless acquire_run_lock orig_backend = backend begin self.backend = with_backend if with_backend @@ -73,6 +74,7 @@ def run(*args, with_backend: nil, **kwargs, &block) end ensure AppProfiler.backend = orig_backend + release_run_lock end def start(*args) @@ -95,7 +97,7 @@ def profiler def backend=(new_backend) new_profiler_backend = if new_backend.is_a?(String) backend_for(new_backend) - elsif new_backend < Backend + elsif new_backend&.< Backend new_backend else raise BackendError, "unsupportend backend type #{new_backend.class}" @@ -179,6 +181,22 @@ def profile_url(upload) AppProfiler.profile_url_formatter.call(upload) end + + private + + def acquire_run_lock + run_lock.try_lock + end + + def release_run_lock + run_lock.unlock + rescue ThreadError + logger.warn("[AppProfiler] run lock not released as it was never acquired") + end + + def run_lock + @run_lock ||= Mutex.new + end end require "app_profiler/railtie" if defined?(Rails::Railtie) From d7940d32bbb884f0e68a19502a3539549d1049a5 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Tue, 30 Jan 2024 09:53:07 -0500 Subject: [PATCH 12/34] Fix default viewer --- lib/app_profiler/profile/stackprof.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/app_profiler/profile/stackprof.rb b/lib/app_profiler/profile/stackprof.rb index 1d08c259..bb16ff4e 100644 --- a/lib/app_profiler/profile/stackprof.rb +++ b/lib/app_profiler/profile/stackprof.rb @@ -30,7 +30,7 @@ def format end def view(params = {}) - Viewer::SpeedscopeViewer.view(self, **params) + AppProfiler.viewer.view(self, **params) end end end From 7d1a737a258210df16e6c60773605a00fb083da9 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Wed, 7 Feb 2024 14:54:44 -0500 Subject: [PATCH 13/34] Update README.md Co-authored-by: Gannon McGibbon --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 93515a42..564bc609 100644 --- a/README.md +++ b/README.md @@ -316,7 +316,7 @@ Rails.application.config.app_profiler.middleware_action = AppProfiler::Middlewar ## Profiler backends -It is possible to configure app_profiler to use the [`vernier`](https://github.com/jhawthorn/vernier) or stackprof. To use vernier, it must be added separately in the application Gemfile. +It is possible to configure AppProfiler to use the [`vernier`](https://github.com/jhawthorn/vernier) or [`stackprof`](https://github.com/tmm1/stackprof). To use `vernier`, it must be added separately in the application Gemfile. The backend can be selected dynamically at runtime using the `backend` parameter. The default backend to use when this parameter is not specified can be configured with: From 4cef38ab2e69ad0c5c3a2ae98f472756b2d5facf Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Wed, 7 Feb 2024 14:54:53 -0500 Subject: [PATCH 14/34] Update lib/app_profiler.rb Co-authored-by: Gannon McGibbon --- lib/app_profiler.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/app_profiler.rb b/lib/app_profiler.rb index f8791ac7..14b90462 100644 --- a/lib/app_profiler.rb +++ b/lib/app_profiler.rb @@ -63,7 +63,7 @@ module Viewer mattr_reader :after_process_queue, default: nil class << self - def run(*args, with_backend: nil, **kwargs, &block) + def run(*args, backend: nil, **kwargs, &block) yield unless acquire_run_lock orig_backend = backend begin From 857a79f2f2fb7fb9dbe350521e11c9dc3fc2359e Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Wed, 7 Feb 2024 14:55:03 -0500 Subject: [PATCH 15/34] Update .github/workflows/ci.yml Co-authored-by: Gannon McGibbon --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b8f26562..52b31aa5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,7 +12,7 @@ jobs: - "2.7" - "3.0" - "3.1" - - "3.2.1" + - "3.2" steps: - uses: actions/checkout@v3 - name: Set up Ruby ${{ matrix.ruby }} From 04d41523c97bdf925d16d4a707c6cbf542d9d398 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Wed, 7 Feb 2024 14:55:20 -0500 Subject: [PATCH 16/34] Update README.md Co-authored-by: Gannon McGibbon --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 564bc609..0290cb5f 100644 --- a/README.md +++ b/README.md @@ -82,7 +82,7 @@ Rails.application.config.app_profiler.profile_header = "X-Profile" | ignore_gc | Ignore garbage collection frames | | | autoredirect | Redirect request automatically to Speedscope's page after profiling. | | | context | Directory within the specified bucket in the selected storage where raw profile data should be written. | Only supported in (2). Defaults to `Rails.env` if not specified. | -| backend | Profiler to use, either `stackprof` or `vernier`. Defaults to stackprof. Vernier is only available when added separately in the application Gemfile, and the Ruby version is at least 3.2.1 | +| backend | Profiler to use, either `stackprof` or `vernier`. Defaults to `stackprof`. Note that Vernier requires Ruby 3.2.1+ | Note that the `autoredirect` feature can be turned on for all requests by doing the following: From 26a393283fe68795201675593415d7b7c041c183 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Wed, 7 Feb 2024 15:15:54 -0500 Subject: [PATCH 17/34] Rename with_backend to backend, avoid variable shadowing with explicit reference to self --- lib/app_profiler.rb | 4 ++-- lib/app_profiler/middleware.rb | 2 +- test/app_profiler/run_test.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/app_profiler.rb b/lib/app_profiler.rb index 14b90462..e39dac18 100644 --- a/lib/app_profiler.rb +++ b/lib/app_profiler.rb @@ -65,9 +65,9 @@ module Viewer class << self def run(*args, backend: nil, **kwargs, &block) yield unless acquire_run_lock - orig_backend = backend + orig_backend = self.backend begin - self.backend = with_backend if with_backend + self.backend = backend if backend profiler.run(*args, **kwargs, &block) rescue BackendError yield diff --git a/lib/app_profiler/middleware.rb b/lib/app_profiler/middleware.rb index 53eff30d..63947e1d 100644 --- a/lib/app_profiler/middleware.rb +++ b/lib/app_profiler/middleware.rb @@ -31,7 +31,7 @@ def profile(env, params) return yield unless before_profile(env, params_hash) - profile = AppProfiler.run(params_hash, with_backend: params.backend) do + profile = AppProfiler.run(params_hash, backend: params.backend) do response = yield end diff --git a/test/app_profiler/run_test.rb b/test/app_profiler/run_test.rb index 5b95936d..0f9e1750 100644 --- a/test/app_profiler/run_test.rb +++ b/test/app_profiler/run_test.rb @@ -32,7 +32,7 @@ class RunTest < TestCase assert_equal(AppProfiler.backend, AppProfiler::StackprofBackend) refute(AppProfiler.running?) - AppProfiler.run(with_backend: AppProfiler::VernierBackend) do + AppProfiler.run(backend: AppProfiler::VernierBackend) do assert_equal(AppProfiler::VernierBackend, AppProfiler.backend) end assert_equal(AppProfiler.backend, AppProfiler::StackprofBackend) From ababe066e82209ed2b30408f9d2e530335d59d67 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Wed, 7 Feb 2024 15:33:25 -0500 Subject: [PATCH 18/34] Move vernier to be autoloaded, remove rescue blocks around loading --- lib/app_profiler.rb | 2 +- lib/app_profiler/backend.rb | 6 +----- lib/app_profiler/backend/vernier.rb | 10 ++-------- lib/app_profiler/request_parameters.rb | 4 ++-- test/app_profiler/backend/vernier_backend_test.rb | 2 +- test/app_profiler/backend_test.rb | 10 +++++----- test/app_profiler/middleware_test.rb | 6 +++--- test/app_profiler/run_test.rb | 2 +- 8 files changed, 16 insertions(+), 26 deletions(-) diff --git a/lib/app_profiler.rb b/lib/app_profiler.rb index e39dac18..45285894 100644 --- a/lib/app_profiler.rb +++ b/lib/app_profiler.rb @@ -115,7 +115,7 @@ def backend=(new_backend) end def backend_for(backend_name) - if defined?(AppProfiler::VernierBackend) && + if defined?(AppProfiler::VernierBackend::NAME) && backend_name == AppProfiler::VernierBackend::NAME AppProfiler::VernierBackend elsif backend_name == AppProfiler::StackprofBackend::NAME diff --git a/lib/app_profiler/backend.rb b/lib/app_profiler/backend.rb index 324d402f..fb038c56 100644 --- a/lib/app_profiler/backend.rb +++ b/lib/app_profiler/backend.rb @@ -24,10 +24,6 @@ def running? end autoload :StackprofBackend, "app_profiler/backend/stackprof" - begin - require "app_profiler/backend/vernier" - rescue LoadError - warn("Vernier is not supported.") - end + autoload :VernierBackend, "app_profiler/backend/vernier" DefaultBackend = AppProfiler::StackprofBackend end diff --git a/lib/app_profiler/backend/vernier.rb b/lib/app_profiler/backend/vernier.rb index 9843a688..dbbf035e 100644 --- a/lib/app_profiler/backend/vernier.rb +++ b/lib/app_profiler/backend/vernier.rb @@ -1,13 +1,7 @@ # frozen_string_literal: true -begin - gem("vernier", ">= 0.4.0") - require "vernier" -rescue LoadError - warn("Vernier profiling support requires the vernier gem, version 0.4.0 or later." \ - "Please add it to your Gemfile: `gem \"vernier\", \">= 0.4.0\"`") - raise -end +gem("vernier", ">= 0.4.0") +require "vernier" module AppProfiler class VernierBackend < Backend diff --git a/lib/app_profiler/request_parameters.rb b/lib/app_profiler/request_parameters.rb index 04f27847..377d7155 100644 --- a/lib/app_profiler/request_parameters.rb +++ b/lib/app_profiler/request_parameters.rb @@ -26,9 +26,9 @@ def valid? return false end - return false if backend != AppProfiler::StackprofBackend::NAME && !defined?(AppProfiler::VernierBackend) + return false if backend != AppProfiler::StackprofBackend::NAME && !defined?(AppProfiler::VernierBackend::NAME) - if defined?(AppProfiler::VernierBackend) && backend == AppProfiler::VernierBackend::NAME && + if defined?(AppProfiler::VernierBackend::NAME) && backend == AppProfiler::VernierBackend::NAME && !AppProfiler::VernierBackend::AVAILABLE_MODES.include?(mode.to_sym) AppProfiler.logger.info("[AppProfiler] unsupported profiling mode=#{mode} for backend #{backend}") return false diff --git a/test/app_profiler/backend/vernier_backend_test.rb b/test/app_profiler/backend/vernier_backend_test.rb index 0d5f8f7d..ac1d3703 100644 --- a/test/app_profiler/backend/vernier_backend_test.rb +++ b/test/app_profiler/backend/vernier_backend_test.rb @@ -2,7 +2,7 @@ require "test_helper" -return unless defined?(AppProfiler::VernierBackend) +return unless defined?(AppProfiler::VernierBackend::NAME) module AppProfiler class VernierBackendTest < TestCase diff --git a/test/app_profiler/backend_test.rb b/test/app_profiler/backend_test.rb index 68a950d2..02f11d6f 100644 --- a/test/app_profiler/backend_test.rb +++ b/test/app_profiler/backend_test.rb @@ -5,7 +5,7 @@ module AppProfiler class BackendTest < TestCase test ".backend= fails to update the backend if already profiling" do - skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend) + skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend::NAME) assert(AppProfiler.backend = AppProfiler::StackprofBackend) AppProfiler.start assert(AppProfiler.running?) @@ -16,7 +16,7 @@ class BackendTest < TestCase test ".backend= updates the backend if not already profiling" do orig_backend = AppProfiler.backend - skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend) + skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend::NAME) refute(AppProfiler.running?) assert(AppProfiler.backend = AppProfiler::StackprofBackend) assert_equal(AppProfiler.backend, AppProfiler::StackprofBackend) @@ -29,7 +29,7 @@ class BackendTest < TestCase test ".backend= accepts a string with the backend name" do orig_backend = AppProfiler.backend - skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend) + skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend::NAME) refute(AppProfiler.running?) assert(AppProfiler.backend = AppProfiler::StackprofBackend::NAME) assert_equal(AppProfiler.backend, AppProfiler::StackprofBackend) @@ -42,7 +42,7 @@ class BackendTest < TestCase test ".backend= accepts a backend class" do orig_backend = AppProfiler.backend - skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend) + skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend::NAME) refute(AppProfiler.running?) assert(AppProfiler.backend = AppProfiler::StackprofBackend) assert_equal(AppProfiler.backend, AppProfiler::StackprofBackend) @@ -55,7 +55,7 @@ class BackendTest < TestCase test ".backend_for= provides the backend class given a string" do assert_equal(AppProfiler::StackprofBackend, AppProfiler.backend_for(AppProfiler::StackprofBackend::NAME)) - return unless defined?(AppProfiler::VernierBackend) + return unless defined?(AppProfiler::VernierBackend::NAME) assert_equal(AppProfiler::VernierBackend, AppProfiler.backend_for(AppProfiler::VernierBackend::NAME)) end diff --git a/test/app_profiler/middleware_test.rb b/test/app_profiler/middleware_test.rb index ca730dc9..ce2ae9bd 100644 --- a/test/app_profiler/middleware_test.rb +++ b/test/app_profiler/middleware_test.rb @@ -31,7 +31,7 @@ class MiddlewareTest < TestCase end end - if defined?(AppProfiler::VernierBackend) + if defined?(AppProfiler::VernierBackend::NAME) AppProfiler::VernierBackend::AVAILABLE_MODES.each do |mode| test "profile mode #{mode} is supported by vernier backend" do assert_profiles_dumped do @@ -44,7 +44,7 @@ class MiddlewareTest < TestCase end end - if defined?(AppProfiler::VernierBackend) + if defined?(AppProfiler::VernierBackend::NAME) test "the backend can be toggled between requests" do assert_profiles_dumped(3) do assert_profiles_uploaded do @@ -194,7 +194,7 @@ class MiddlewareTest < TestCase end end - if defined?(AppProfiler::VernierBackend) + if defined?(AppProfiler::VernierBackend::NAME) AppProfiler::VernierBackend::AVAILABLE_MODES.each do |mode| test "profile mode #{mode} is supported through headers by vernier backend" do assert_profiles_dumped do diff --git a/test/app_profiler/run_test.rb b/test/app_profiler/run_test.rb index 0f9e1750..a7a713a4 100644 --- a/test/app_profiler/run_test.rb +++ b/test/app_profiler/run_test.rb @@ -28,7 +28,7 @@ class RunTest < TestCase test ".run sets the backend then returns to the previous value" do orig_backend = AppProfiler.backend - skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend) + skip("Vernier not supported") unless defined?(AppProfiler::VernierBackend::NAME) assert_equal(AppProfiler.backend, AppProfiler::StackprofBackend) refute(AppProfiler.running?) From b3e7a1585fc8ed06143f3bbef9f7cf0501374dc6 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Wed, 7 Feb 2024 15:58:17 -0500 Subject: [PATCH 19/34] Move run lock acquisition into concrete backends, put lock on base eigenclass --- lib/app_profiler.rb | 18 ------------------ lib/app_profiler/backend.rb | 18 ++++++++++++++++++ lib/app_profiler/backend/stackprof.rb | 4 ++++ lib/app_profiler/backend/vernier.rb | 4 ++++ .../backend/stackprof_backend_test.rb | 2 +- 5 files changed, 27 insertions(+), 19 deletions(-) diff --git a/lib/app_profiler.rb b/lib/app_profiler.rb index 45285894..74fa2c3c 100644 --- a/lib/app_profiler.rb +++ b/lib/app_profiler.rb @@ -64,7 +64,6 @@ module Viewer class << self def run(*args, backend: nil, **kwargs, &block) - yield unless acquire_run_lock orig_backend = self.backend begin self.backend = backend if backend @@ -74,7 +73,6 @@ def run(*args, backend: nil, **kwargs, &block) end ensure AppProfiler.backend = orig_backend - release_run_lock end def start(*args) @@ -181,22 +179,6 @@ def profile_url(upload) AppProfiler.profile_url_formatter.call(upload) end - - private - - def acquire_run_lock - run_lock.try_lock - end - - def release_run_lock - run_lock.unlock - rescue ThreadError - logger.warn("[AppProfiler] run lock not released as it was never acquired") - end - - def run_lock - @run_lock ||= Mutex.new - end end require "app_profiler/railtie" if defined?(Rails::Railtie) diff --git a/lib/app_profiler/backend.rb b/lib/app_profiler/backend.rb index fb038c56..300c0c62 100644 --- a/lib/app_profiler/backend.rb +++ b/lib/app_profiler/backend.rb @@ -21,6 +21,24 @@ def results def running? raise NotImplementedError end + + class << self + def run_lock + @run_lock ||= Mutex.new + end + end + + protected + + def acquire_run_lock + self.class.run_lock.try_lock + end + + def release_run_lock + self.class.run_lock.unlock + rescue ThreadError + AppProfiler.logger.warn("[AppProfiler] run lock not released as it was never acquired") + end end autoload :StackprofBackend, "app_profiler/backend/stackprof" diff --git a/lib/app_profiler/backend/stackprof.rb b/lib/app_profiler/backend/stackprof.rb index 1e43b5ad..3bc28f39 100644 --- a/lib/app_profiler/backend/stackprof.rb +++ b/lib/app_profiler/backend/stackprof.rb @@ -33,6 +33,7 @@ def run(params = {}) def start(params = {}) # Do not start the profiler if StackProf was started somewhere else. return false if running? + return false unless acquire_run_lock clear @@ -41,6 +42,7 @@ def start(params = {}) AppProfiler.logger.info( "[Profiler] failed to start the profiler error_class=#{error.class} error_message=#{error.message}" ) + release_run_lock # This is a boolean instead of nil because StackProf#start returns a # boolean as well. false @@ -48,6 +50,8 @@ def start(params = {}) def stop ::StackProf.stop + ensure + release_run_lock end def results diff --git a/lib/app_profiler/backend/vernier.rb b/lib/app_profiler/backend/vernier.rb index dbbf035e..3b5cea72 100644 --- a/lib/app_profiler/backend/vernier.rb +++ b/lib/app_profiler/backend/vernier.rb @@ -33,6 +33,7 @@ def run(params = {}) def start(params = {}) # Do not start the profiler if we already have a collector started somewhere else. return false if running? + return false unless acquire_run_lock @mode = params.delete(:mode) || DEFAULTS[:mode] raise ArgumentError unless AVAILABLE_MODES.include?(@mode) @@ -46,6 +47,7 @@ def start(params = {}) AppProfiler.logger.info( "[Profiler] failed to start the profiler error_class=#{error.class} error_message=#{error.message}" ) + release_run_lock # This is a boolean instead of nil to be consistent with the stackprof backend behaviour # boolean as well. false @@ -57,6 +59,8 @@ def stop @results = @collector&.stop @collector = nil !@results.nil? + ensure + release_run_lock end def results diff --git a/test/app_profiler/backend/stackprof_backend_test.rb b/test/app_profiler/backend/stackprof_backend_test.rb index 88e54877..605161fa 100644 --- a/test/app_profiler/backend/stackprof_backend_test.rb +++ b/test/app_profiler/backend/stackprof_backend_test.rb @@ -46,7 +46,7 @@ def teardown assert_nil(profile) assert_predicate(AppProfiler.profiler, :running?) ensure - StackProf.stop + AppProfiler.profiler.stop end test ".run uses cpu profile by default" do From c1f685d5045037f905ce44210716482327de1b11 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Thu, 8 Feb 2024 15:30:41 -0500 Subject: [PATCH 20/34] Move VernierProfile to using hash instead of Results object --- lib/app_profiler/backend/vernier.rb | 9 ++++--- lib/app_profiler/profile.rb | 21 ++++++++------- lib/app_profiler/profile/stackprof.rb | 17 ------------ lib/app_profiler/profile/vernier.rb | 27 +------------------ .../backend/vernier_backend_test.rb | 26 ++++++++++-------- test/app_profiler/profile/vernier_test.rb | 12 ++++----- test/test_helper.rb | 3 ++- 7 files changed, 42 insertions(+), 73 deletions(-) diff --git a/lib/app_profiler/backend/vernier.rb b/lib/app_profiler/backend/vernier.rb index 3b5cea72..98bc55f8 100644 --- a/lib/app_profiler/backend/vernier.rb +++ b/lib/app_profiler/backend/vernier.rb @@ -69,12 +69,15 @@ def results return unless vernier_profile - vernier_profile.meta[:mode] = @mode # TODO: https://github.com/jhawthorn/vernier/issues/30 - vernier_profile.meta.merge!(@metadata) if @metadata + # HACK: - "data" is private, but we want to avoid serializing to JSON then + # parsing back from JSON by just directly getting the hash + data = ::Vernier::Output::Firefox.new(vernier_profile).send(:data) + data[:meta][:mode] = @mode # TODO: https://github.com/jhawthorn/vernier/issues/30 + data[:meta].merge!(@metadata) if @metadata @mode = nil @metadata = nil - AppProfiler::Profile.from_vernier(vernier_profile) + AppProfiler::Profile.from_vernier(data) rescue => error AppProfiler.logger.info( "[Profiler] failed to obtain the profile error_class=#{error.class} error_message=#{error.message}" diff --git a/lib/app_profiler/profile.rb b/lib/app_profiler/profile.rb index 0d99530b..c0bdd52d 100644 --- a/lib/app_profiler/profile.rb +++ b/lib/app_profiler/profile.rb @@ -11,6 +11,8 @@ class UnsafeFilename < StandardError; end attr_reader :id, :context + delegate :[], to: :@data + # This function should not be called if `StackProf.results` returns nil. def self.from_stackprof(data) options = INTERNAL_METADATA_KEYS.map { |key| [key, data[:metadata]&.delete(key)] }.to_h @@ -21,9 +23,7 @@ def self.from_stackprof(data) end def self.from_vernier(data) - # NB: we don't delete here, that is causing a segfault in vernier. Divergent behaviour from stackprof, - # as the special metadata keys "id" and "context" are preserved into the metadata, but maybe that isn't so bad. - options = INTERNAL_METADATA_KEYS.map { |key| [key, data.meta.clone[key]] }.to_h + options = INTERNAL_METADATA_KEYS.map { |key| [key, data[:meta]&.delete(key)] }.to_h VernierProfile.new(data, **options).tap do |profile| raise ArgumentError, "invalid profile data" unless profile.valid? @@ -61,16 +61,19 @@ def enqueue_upload AppProfiler.storage.enqueue_upload(self) end - def file - raise NotImplementedError + def valid? + mode.present? end - def to_h - raise NotImplementedError + def file + @file ||= path.tap do |p| + p.dirname.mkpath + p.write(JSON.dump(@data)) + end end - def valid? - raise NotImplementedError + def to_h + @data end def mode diff --git a/lib/app_profiler/profile/stackprof.rb b/lib/app_profiler/profile/stackprof.rb index bb16ff4e..a708ecaf 100644 --- a/lib/app_profiler/profile/stackprof.rb +++ b/lib/app_profiler/profile/stackprof.rb @@ -4,23 +4,6 @@ module AppProfiler class StackprofProfile < Profile FILE_EXTENSION = ".stackprof.json" - delegate :[], to: :@data - - def file - @file ||= path.tap do |p| - p.dirname.mkpath - p.write(JSON.dump(@data)) - end - end - - def to_h - @data - end - - def valid? - mode.present? - end - def mode @data[:mode] end diff --git a/lib/app_profiler/profile/vernier.rb b/lib/app_profiler/profile/vernier.rb index 2fce2742..3fc5e2cd 100644 --- a/lib/app_profiler/profile/vernier.rb +++ b/lib/app_profiler/profile/vernier.rb @@ -4,33 +4,8 @@ module AppProfiler class VernierProfile < Profile FILE_EXTENSION = ".gecko.json" - delegate :[], to: :@meta - - attr_reader :data - - def initialize(data, id: nil, context: nil) - @meta = data.meta - super - end - - # https://github.com/jhawthorn/vernier/blob/main/lib/vernier/result.rb#L27-L29 - def file - @file ||= path.tap do |p| - p.dirname.mkpath - @data.write(out: p) - end - end - - def valid? - !@data.nil? - end - - def to_h - @data.to_h - end - def mode - @meta[:mode] + @data[:meta][:mode] end def format diff --git a/test/app_profiler/backend/vernier_backend_test.rb b/test/app_profiler/backend/vernier_backend_test.rb index ac1d3703..6f81f79f 100644 --- a/test/app_profiler/backend/vernier_backend_test.rb +++ b/test/app_profiler/backend/vernier_backend_test.rb @@ -60,18 +60,25 @@ def teardown end assert_instance_of(AppProfiler::VernierProfile, profile) - assert_equal(:wall, profile[:mode]) + assert_equal(:wall, profile[:meta][:mode]) # assert_equal(1000, profile[:interval]) # TODO https://github.com/jhawthorn/vernier/issues/30 end test ".run assigns metadata to profiles" do - profile = AppProfiler.profiler.run(vernier_params(metadata: { id: "wowza", context: "bar" })) do + profile = AppProfiler.profiler.run( + vernier_params(metadata: { + id: "wowza", + context: "bar", + extrameta: "spam", + }) + ) do sleep(0.1) end assert_instance_of(AppProfiler::VernierProfile, profile) assert_equal("wowza", profile.id) assert_equal("bar", profile.context) + assert_equal("spam", profile[:meta][:extrameta]) end test ".run wall profile" do @@ -80,7 +87,7 @@ def teardown end assert_instance_of(AppProfiler::VernierProfile, profile) - assert_equal(:wall, profile[:mode]) + assert_equal(:wall, profile[:meta][:mode]) # assert_equal(2000, profile[:interval]) # TODO as above end @@ -94,13 +101,10 @@ def teardown end assert_instance_of(AppProfiler::VernierProfile, profile) - assert_equal(:retained, profile[:mode]) + assert_equal(:retained, profile[:meta][:mode]) - weights = [] - profile.data.each_sample do |_, weight| - weights << weight - end - assert_operator(weights.size, :>=, objects) + num_samples = profile[:threads].flat_map { _1[:samples] }.sum { |s| s[:length] } + assert_operator(num_samples, :>=, objects) end test ".run works for supported modes" do @@ -133,7 +137,7 @@ def teardown profile = AppProfiler.profiler.results assert_instance_of(AppProfiler::VernierProfile, profile) - assert_equal(:wall, profile[:mode]) + assert_equal(:wall, profile[:meta][:mode]) # assert_equal(1000, profile[:interval]) end @@ -155,7 +159,7 @@ def teardown profile = AppProfiler.profiler.results assert_instance_of(AppProfiler::VernierProfile, profile) - assert_equal(:wall, profile[:mode]) + assert_equal(:wall, profile[:meta][:mode]) # assert_equal(2000, profile[:interval]) end diff --git a/test/app_profiler/profile/vernier_test.rb b/test/app_profiler/profile/vernier_test.rb index 77f80f10..dfbcffa1 100644 --- a/test/app_profiler/profile/vernier_test.rb +++ b/test/app_profiler/profile/vernier_test.rb @@ -5,7 +5,7 @@ module AppProfiler class VernierProfileTest < TestCase test ".from_vernier assigns id and context metadata" do - profile = Profile.from_vernier(vernier_profile(metadata: { id: "foo", context: "bar" })) + profile = Profile.from_vernier(vernier_profile(meta: { id: "foo", context: "bar" })) assert_equal("foo", profile.id) assert_equal("bar", profile.context) @@ -14,7 +14,7 @@ class VernierProfileTest < TestCase test ".from_vernier assigns random id when id is not present" do SecureRandom.expects(:hex).returns("mock") - params_without_id = vernier_profile(vernier_params).tap { |data| data.meta.delete(:id) } + params_without_id = vernier_profile(vernier_params).tap { |data| data[:meta].delete(:id) } profile = Profile.from_vernier(params_without_id) assert_equal("mock", profile.id) @@ -55,7 +55,7 @@ class VernierProfileTest < TestCase end test "#mode" do - profile = VernierProfile.new(vernier_profile(metadata: { mode: "retained" })) + profile = VernierProfile.new(vernier_profile(meta: { mode: "retained" })) assert_equal("retained", profile.mode) end @@ -103,14 +103,14 @@ class VernierProfileTest < TestCase end test "#[] forwards to profile metadata" do - profile = VernierProfile.new(vernier_profile(metadata: { interval: 10_000 })) + profile = VernierProfile.new(vernier_profile(meta: { interval: 10_000 })) - assert_equal(10_000, profile[:interval]) + assert_equal(10_000, profile[:meta][:interval]) end test "#path raises an UnsafeFilename exception given chars not in allow list" do assert_raises(AppProfiler::Profile::UnsafeFilename) do - profile = Profile.from_vernier(vernier_profile(metadata: { id: "|`@${}", context: "bar" })) + profile = Profile.from_vernier(vernier_profile(meta: { id: "|`@${}", context: "bar" })) profile.file end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 9bf39499..55cdced6 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -76,7 +76,8 @@ def stackprof_profile(params = {}) end def vernier_profile(params = {}) - ::Vernier::FakeResult.new(params) + { meta: { product: "Ruby/Vernier", mode: "wall", markerSchema: [], sampleUnits: {}, categories: [] }, libs: [], + threads: [], }.deep_merge(params) end def vernier_params(params = {}) From 88959872c6110b16b62453a8080bd8cde9428632 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Thu, 8 Feb 2024 16:09:59 -0500 Subject: [PATCH 21/34] Deprecate Profile in favour of AbstractProfile Callers should now either use VernierProfile or StackprofProfile to get an explicit base class. If loading from the eigenclass, they should use AbstractProfile.from_stackprof or AbstractProfile.from_vernier instead of Profile.from_X. --- lib/app_profiler/backend/stackprof.rb | 2 +- lib/app_profiler/backend/vernier.rb | 2 +- lib/app_profiler/profile.rb | 5 ++++- lib/app_profiler/profile/stackprof.rb | 2 +- lib/app_profiler/profile/vernier.rb | 2 +- test/app_profiler/middleware/view_action_test.rb | 2 +- test/app_profiler/middleware_test.rb | 4 ++-- test/app_profiler/profile/stackprof_test.rb | 12 ++++++------ test/app_profiler/profile/vernier_test.rb | 8 ++++---- .../storage/google_cloud_storage_test.rb | 6 +++--- .../speedscope_remote_viewer/middleware_test.rb | 10 +++++----- .../viewer/speedscope_remote_viewer_test.rb | 6 +++--- test/app_profiler/viewer/speedscope_viewer_test.rb | 12 ++++++------ 13 files changed, 38 insertions(+), 35 deletions(-) diff --git a/lib/app_profiler/backend/stackprof.rb b/lib/app_profiler/backend/stackprof.rb index 3bc28f39..edddcb11 100644 --- a/lib/app_profiler/backend/stackprof.rb +++ b/lib/app_profiler/backend/stackprof.rb @@ -59,7 +59,7 @@ def results return unless stackprof_profile - AppProfiler::Profile.from_stackprof(stackprof_profile) + AppProfiler::AbstractProfile.from_stackprof(stackprof_profile) rescue => error AppProfiler.logger.info( "[Profiler] failed to obtain the profile error_class=#{error.class} error_message=#{error.message}" diff --git a/lib/app_profiler/backend/vernier.rb b/lib/app_profiler/backend/vernier.rb index 98bc55f8..50557621 100644 --- a/lib/app_profiler/backend/vernier.rb +++ b/lib/app_profiler/backend/vernier.rb @@ -77,7 +77,7 @@ def results @mode = nil @metadata = nil - AppProfiler::Profile.from_vernier(data) + AppProfiler::AbstractProfile.from_vernier(data) rescue => error AppProfiler.logger.info( "[Profiler] failed to obtain the profile error_class=#{error.class} error_message=#{error.message}" diff --git a/lib/app_profiler/profile.rb b/lib/app_profiler/profile.rb index c0bdd52d..2fea1ba6 100644 --- a/lib/app_profiler/profile.rb +++ b/lib/app_profiler/profile.rb @@ -4,7 +4,7 @@ module AppProfiler autoload :StackprofProfile, "app_profiler/profile/stackprof" autoload :VernierProfile, "app_profiler/profile/vernier" - class Profile + class AbstractProfile INTERNAL_METADATA_KEYS = [:id, :context] private_constant :INTERNAL_METADATA_KEYS class UnsafeFilename < StandardError; end @@ -103,4 +103,7 @@ def path AppProfiler.profile_root.join(filename) end end + + Profile = AbstractProfile + deprecate_constant :Profile end diff --git a/lib/app_profiler/profile/stackprof.rb b/lib/app_profiler/profile/stackprof.rb index a708ecaf..daff1474 100644 --- a/lib/app_profiler/profile/stackprof.rb +++ b/lib/app_profiler/profile/stackprof.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module AppProfiler - class StackprofProfile < Profile + class StackprofProfile < AbstractProfile FILE_EXTENSION = ".stackprof.json" def mode diff --git a/lib/app_profiler/profile/vernier.rb b/lib/app_profiler/profile/vernier.rb index 3fc5e2cd..02f267b8 100644 --- a/lib/app_profiler/profile/vernier.rb +++ b/lib/app_profiler/profile/vernier.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module AppProfiler - class VernierProfile < Profile + class VernierProfile < AbstractProfile FILE_EXTENSION = ".gecko.json" def mode diff --git a/test/app_profiler/middleware/view_action_test.rb b/test/app_profiler/middleware/view_action_test.rb index b6cba7fe..4460af3d 100644 --- a/test/app_profiler/middleware/view_action_test.rb +++ b/test/app_profiler/middleware/view_action_test.rb @@ -6,7 +6,7 @@ module AppProfiler class Middleware class ViewActionTest < AppProfiler::TestCase setup do - @profile = Profile.new(stackprof_profile) + @profile = StackprofProfile.new(stackprof_profile) @response = [200, {}, ["OK"]] end diff --git a/test/app_profiler/middleware_test.rb b/test/app_profiler/middleware_test.rb index ce2ae9bd..cc25457c 100644 --- a/test/app_profiler/middleware_test.rb +++ b/test/app_profiler/middleware_test.rb @@ -310,7 +310,7 @@ class MiddlewareTest < TestCase test "#after_profile called with env and profile data" do request_env = mock_request_env(path: "/?profile=cpu") AppProfiler.middleware.any_instance.expects(:after_profile).with do |env, profile| - request_env == env && profile.is_a?(AppProfiler::Profile) + request_env == env && profile.is_a?(AppProfiler::AbstractProfile) end.returns(false) middleware = AppProfiler::Middleware.new(app_env) middleware.call(request_env) @@ -329,7 +329,7 @@ class MiddlewareTest < TestCase end.returns(true) AppProfiler.middleware.any_instance.expects(:after_profile).with do |env, profile| - return false unless request_env == env && profile.is_a?(AppProfiler::Profile) + return false unless request_env == env && profile.is_a?(AppProfiler::AbstractProfile) profile[:metadata][:test_key] == "test_value" end.returns(true) diff --git a/test/app_profiler/profile/stackprof_test.rb b/test/app_profiler/profile/stackprof_test.rb index ad55a693..264d91fa 100644 --- a/test/app_profiler/profile/stackprof_test.rb +++ b/test/app_profiler/profile/stackprof_test.rb @@ -7,13 +7,13 @@ class StackprofProfileTest < TestCase test ".from_stackprof raises ArgumentError when mode is not present" do error = assert_raises(ArgumentError) do profile_without_mode = stackprof_profile.tap { |data| data.delete(:mode) } - Profile.from_stackprof(profile_without_mode) + AbstractProfile.from_stackprof(profile_without_mode) end assert_equal("invalid profile data", error.message) end test ".from_stackprof assigns id and context metadata" do - profile = Profile.from_stackprof(stackprof_profile(metadata: { id: "foo", context: "bar" })) + profile = AbstractProfile.from_stackprof(stackprof_profile(metadata: { id: "foo", context: "bar" })) assert_equal("foo", profile.id) assert_equal("bar", profile.context) @@ -23,13 +23,13 @@ class StackprofProfileTest < TestCase SecureRandom.expects(:hex).returns("mock") params_without_id = stackprof_profile.tap { |data| data[:metadata].delete(:id) } - profile = Profile.from_stackprof(params_without_id) + profile = AbstractProfile.from_stackprof(params_without_id) assert_equal("mock", profile.id) end test ".from_stackprof removes id and context metadata from profile data" do - profile = Profile.from_stackprof(stackprof_profile(metadata: { id: "foo", context: "bar" })) + profile = AbstractProfile.from_stackprof(stackprof_profile(metadata: { id: "foo", context: "bar" })) assert_not_operator(profile[:metadata], :key?, :id) assert_not_operator(profile[:metadata], :key?, :context) @@ -131,8 +131,8 @@ class StackprofProfileTest < TestCase end test "#path raises an UnsafeFilename exception given chars not in allow list" do - assert_raises(AppProfiler::Profile::UnsafeFilename) do - profile = Profile.from_stackprof(stackprof_profile(metadata: { id: "|`@${}", context: "bar" })) + assert_raises(AppProfiler::AbstractProfile::UnsafeFilename) do + profile = AbstractProfile.from_stackprof(stackprof_profile(metadata: { id: "|`@${}", context: "bar" })) profile.file end end diff --git a/test/app_profiler/profile/vernier_test.rb b/test/app_profiler/profile/vernier_test.rb index dfbcffa1..8b5d9242 100644 --- a/test/app_profiler/profile/vernier_test.rb +++ b/test/app_profiler/profile/vernier_test.rb @@ -5,7 +5,7 @@ module AppProfiler class VernierProfileTest < TestCase test ".from_vernier assigns id and context metadata" do - profile = Profile.from_vernier(vernier_profile(meta: { id: "foo", context: "bar" })) + profile = AbstractProfile.from_vernier(vernier_profile(meta: { id: "foo", context: "bar" })) assert_equal("foo", profile.id) assert_equal("bar", profile.context) @@ -15,7 +15,7 @@ class VernierProfileTest < TestCase SecureRandom.expects(:hex).returns("mock") params_without_id = vernier_profile(vernier_params).tap { |data| data[:meta].delete(:id) } - profile = Profile.from_vernier(params_without_id) + profile = AbstractProfile.from_vernier(params_without_id) assert_equal("mock", profile.id) end @@ -109,8 +109,8 @@ class VernierProfileTest < TestCase end test "#path raises an UnsafeFilename exception given chars not in allow list" do - assert_raises(AppProfiler::Profile::UnsafeFilename) do - profile = Profile.from_vernier(vernier_profile(meta: { id: "|`@${}", context: "bar" })) + assert_raises(AppProfiler::AbstractProfile::UnsafeFilename) do + profile = AbstractProfile.from_vernier(vernier_profile(meta: { id: "|`@${}", context: "bar" })) profile.file end end diff --git a/test/app_profiler/storage/google_cloud_storage_test.rb b/test/app_profiler/storage/google_cloud_storage_test.rb index 073035cf..76192ae6 100644 --- a/test/app_profiler/storage/google_cloud_storage_test.rb +++ b/test/app_profiler/storage/google_cloud_storage_test.rb @@ -54,7 +54,7 @@ def teardown end test ".process_queue is a no-op when nothing to upload" do - Profile.any_instance.expects(:upload).never + StackprofProfile.any_instance.expects(:upload).never GoogleCloudStorage.send(:process_queue) end @@ -87,7 +87,7 @@ def teardown AppProfiler.upload_queue_max_length.times do GoogleCloudStorage.enqueue_upload(profile_from_stackprof) end - dropped_profile = Profile.from_stackprof(profile_from_stackprof) + dropped_profile = AbstractProfile.from_stackprof(profile_from_stackprof) AppProfiler.logger.expects(:info).with { |value| value =~ /upload queue is full/ } @called = false @@ -124,7 +124,7 @@ def with_stubbed_process_queue_thread end def profile_from_stackprof - Profile.from_stackprof(stackprof_profile(metadata: { id: "bar" })) + AbstractProfile.from_stackprof(stackprof_profile(metadata: { id: "bar" })) end def json_test_file diff --git a/test/app_profiler/viewer/speedscope_remote_viewer/middleware_test.rb b/test/app_profiler/viewer/speedscope_remote_viewer/middleware_test.rb index 18507c96..36a39c64 100644 --- a/test/app_profiler/viewer/speedscope_remote_viewer/middleware_test.rb +++ b/test/app_profiler/viewer/speedscope_remote_viewer/middleware_test.rb @@ -13,14 +13,14 @@ class MiddlewareTest < TestCase end test ".id" do - profile = Profile.from_stackprof(stackprof_profile) + profile = AbstractProfile.from_stackprof(stackprof_profile) profile_id = profile.file.basename.to_s.delete_suffix(".json") assert_equal(profile_id, Middleware.id(profile.file)) end test "#call index" do - profiles = Array.new(3) { Profile.from_stackprof(stackprof_profile).tap(&:file) } + profiles = Array.new(3) { AbstractProfile.from_stackprof(stackprof_profile).tap(&:file) } code, content_type, html = @app.call({ "PATH_INFO" => "/app_profiler" }) html = html.first @@ -34,7 +34,7 @@ class MiddlewareTest < TestCase end test "#call index with slash" do - profiles = Array.new(3) { Profile.from_stackprof(stackprof_profile).tap(&:file) } + profiles = Array.new(3) { AbstractProfile.from_stackprof(stackprof_profile).tap(&:file) } code, content_type, html = @app.call({ "PATH_INFO" => "/app_profiler/" }) html = html.first @@ -48,7 +48,7 @@ class MiddlewareTest < TestCase end test "#call show" do - profile = Profile.from_stackprof(stackprof_profile) + profile = AbstractProfile.from_stackprof(stackprof_profile) id = Middleware.id(profile.file) code, content_type, html = @app.call({ "PATH_INFO" => "/app_profiler/#{id}" }) @@ -62,7 +62,7 @@ class MiddlewareTest < TestCase test "#call show can serve huge payloads" do frames = { "1" => { name: "a" * 1e7 } } - profile = Profile.from_stackprof(stackprof_profile(frames: frames)) + profile = AbstractProfile.from_stackprof(stackprof_profile(frames: frames)) id = Middleware.id(profile.file) _, _, html = @app.call({ "PATH_INFO" => "/app_profiler/#{id}" }) diff --git a/test/app_profiler/viewer/speedscope_remote_viewer_test.rb b/test/app_profiler/viewer/speedscope_remote_viewer_test.rb index 4982ca8d..4bb5b3e5 100644 --- a/test/app_profiler/viewer/speedscope_remote_viewer_test.rb +++ b/test/app_profiler/viewer/speedscope_remote_viewer_test.rb @@ -8,12 +8,12 @@ class SpeedscopeRemoteViewerTest < TestCase test ".view initializes and calls #view" do SpeedscopeRemoteViewer.any_instance.expects(:view) - profile = Profile.from_stackprof(stackprof_profile) + profile = AbstractProfile.from_stackprof(stackprof_profile) SpeedscopeRemoteViewer.view(profile) end test "#view logs middleware URL" do - profile = Profile.from_stackprof(stackprof_profile) + profile = AbstractProfile.from_stackprof(stackprof_profile) viewer = SpeedscopeRemoteViewer.new(profile) id = SpeedscopeRemoteViewer::Middleware.id(profile.file) @@ -27,7 +27,7 @@ class SpeedscopeRemoteViewerTest < TestCase test "#view with response redirects to URL" do response = [200, {}, ["OK"]] - profile = Profile.from_stackprof(stackprof_profile) + profile = AbstractProfile.from_stackprof(stackprof_profile) viewer = SpeedscopeRemoteViewer.new(profile) id = SpeedscopeRemoteViewer::Middleware.id(profile.file) diff --git a/test/app_profiler/viewer/speedscope_viewer_test.rb b/test/app_profiler/viewer/speedscope_viewer_test.rb index 0745af62..029bdfa0 100644 --- a/test/app_profiler/viewer/speedscope_viewer_test.rb +++ b/test/app_profiler/viewer/speedscope_viewer_test.rb @@ -8,12 +8,12 @@ class SpeedscopeViewerTest < TestCase test ".view initializes and calls #view" do SpeedscopeViewer.any_instance.expects(:view) - profile = Profile.from_stackprof(stackprof_profile) + profile = AbstractProfile.from_stackprof(stackprof_profile) SpeedscopeViewer.view(profile) end test "#view opens profile in speedscope and sets up yarn" do - profile = Profile.from_stackprof(stackprof_profile) + profile = AbstractProfile.from_stackprof(stackprof_profile) viewer = SpeedscopeViewer.new(profile) viewer.expects(:system).with("which", "yarn", out: File::NULL).returns(true) @@ -31,7 +31,7 @@ class SpeedscopeViewerTest < TestCase end test "#view doesn't init when package.json exists" do - profile = Profile.from_stackprof(stackprof_profile) + profile = AbstractProfile.from_stackprof(stackprof_profile) AppProfiler.root.mkpath AppProfiler.root.join("package.json").write("{}") @@ -52,7 +52,7 @@ class SpeedscopeViewerTest < TestCase end test "#view doesn't add when speedscope exists" do - profile = Profile.from_stackprof(stackprof_profile) + profile = AbstractProfile.from_stackprof(stackprof_profile) AppProfiler.root.mkpath AppProfiler.root.join("node_modules/speedscope").mkpath @@ -71,7 +71,7 @@ class SpeedscopeViewerTest < TestCase end test "#view only opens profile in speedscope if yarn is already setup" do - profile = Profile.from_stackprof(stackprof_profile) + profile = AbstractProfile.from_stackprof(stackprof_profile) with_yarn_setup do viewer = SpeedscopeViewer.new(profile) @@ -82,7 +82,7 @@ class SpeedscopeViewerTest < TestCase end test "#view raises YarnError when yarn command fails" do - profile = Profile.from_stackprof(stackprof_profile) + profile = AbstractProfile.from_stackprof(stackprof_profile) viewer = SpeedscopeViewer.new(profile) viewer.expects(:system).returns(false) From c9f2da49ed6ca72854fcd2d50307c595444c665b Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Thu, 8 Feb 2024 16:59:34 -0500 Subject: [PATCH 22/34] Readme update --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 0290cb5f..147118af 100644 --- a/README.md +++ b/README.md @@ -321,9 +321,9 @@ It is possible to configure AppProfiler to use the [`vernier`](https://github.co The backend can be selected dynamically at runtime using the `backend` parameter. The default backend to use when this parameter is not specified can be configured with: ```ruby -AppProfiler.profiler_backend = AppProfiler::StackprofBackend # or AppProfiler::VernierBackend +AppProfiler.backend = AppProfiler::StackprofBackend # or AppProfiler::VernierBackend # OR -Rails.application.config.app_profiler.profiler_backend = AppProfiler::StackprofBackend # or AppProfiler::VernierBackend +Rails.application.config.app_profiler.backend = AppProfiler::StackprofBackend # or AppProfiler::VernierBackend ``` By default, the stackprof backend will be used. From ae0c49ddfb1e03e0699151a1c5d0deefa63a130e Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Mon, 22 Jan 2024 15:06:06 -0500 Subject: [PATCH 23/34] Add support for firefox profile viewer --- README.md | 2 +- lib/app_profiler.rb | 6 +- lib/app_profiler/railtie.rb | 7 +- lib/app_profiler/viewer/base_viewer.rb | 98 +++++++++++++++ ...speedscope_remote_viewer.rb => firefox.rb} | 9 +- lib/app_profiler/viewer/middleware/firefox.rb | 68 +++++++++++ .../viewer/middleware/speedscope.rb | 56 +++++++++ lib/app_profiler/viewer/speedscope.rb | 33 +++++ .../base_middleware.rb | 102 ---------------- .../speedscope_remote_viewer/middleware.rb | 58 --------- lib/app_profiler/viewer/speedscope_viewer.rb | 27 ---- lib/app_profiler/yarn/command.rb | 15 ++- .../yarn/with_firefox_profiler.rb | 63 ++++++++++ .../viewer/firefox_viewer/middleware_test.rb | 115 ++++++++++++++++++ .../viewer/firefox_viewer_test.rb | 42 +++++++ .../middleware_test.rb | 28 +++-- test/app_profiler/yarn/command_test.rb | 4 +- test/test_helper.rb | 8 +- 18 files changed, 522 insertions(+), 219 deletions(-) rename lib/app_profiler/viewer/{speedscope_remote_viewer.rb => firefox.rb} (71%) create mode 100644 lib/app_profiler/viewer/middleware/firefox.rb create mode 100644 lib/app_profiler/viewer/middleware/speedscope.rb create mode 100644 lib/app_profiler/viewer/speedscope.rb delete mode 100644 lib/app_profiler/viewer/speedscope_remote_viewer/base_middleware.rb delete mode 100644 lib/app_profiler/viewer/speedscope_remote_viewer/middleware.rb delete mode 100644 lib/app_profiler/viewer/speedscope_viewer.rb create mode 100644 lib/app_profiler/yarn/with_firefox_profiler.rb create mode 100644 test/app_profiler/viewer/firefox_viewer/middleware_test.rb create mode 100644 test/app_profiler/viewer/firefox_viewer_test.rb rename test/app_profiler/viewer/{speedscope_remote_viewer => speedscope_viewer}/middleware_test.rb (80%) diff --git a/README.md b/README.md index 147118af..2e6c079d 100644 --- a/README.md +++ b/README.md @@ -282,7 +282,7 @@ report = AppProfiler.run(mode: :cpu) do # ... end -report.view # opens the profile locally in speedscope. +report.view # opens the profile locally in speedscope or firefox profiler, as appropriate ``` Profile files can be found locally in your rails app at `tmp/app_profiler/*.json`. diff --git a/lib/app_profiler.rb b/lib/app_profiler.rb index 74fa2c3c..180f670e 100644 --- a/lib/app_profiler.rb +++ b/lib/app_profiler.rb @@ -28,8 +28,8 @@ module Storage module Viewer autoload :BaseViewer, "app_profiler/viewer/base_viewer" - autoload :SpeedscopeViewer, "app_profiler/viewer/speedscope_viewer" - autoload :SpeedscopeRemoteViewer, "app_profiler/viewer/speedscope_remote_viewer" + autoload :SpeedscopeViewer, "app_profiler/viewer/speedscope" + autoload :FirefoxViewer, "app_profiler/viewer/firefox" end require "app_profiler/middleware" @@ -51,8 +51,8 @@ module Viewer mattr_reader :profile_url_formatter, default: DefaultProfileFormatter + mattr_accessor :gecko_viewer_package, default: "https://github.com/firefox-devtools/profiler" mattr_accessor :storage, default: Storage::FileStorage - mattr_accessor :viewer, default: Viewer::SpeedscopeViewer mattr_accessor :middleware, default: Middleware mattr_accessor :server, default: Server mattr_accessor :upload_queue_max_length, default: 10 diff --git a/lib/app_profiler/railtie.rb b/lib/app_profiler/railtie.rb index c816f2b1..fb89fc00 100644 --- a/lib/app_profiler/railtie.rb +++ b/lib/app_profiler/railtie.rb @@ -11,7 +11,6 @@ class Railtie < Rails::Railtie AppProfiler.logger = app.config.app_profiler.logger || Rails.logger AppProfiler.root = app.config.app_profiler.root || Rails.root AppProfiler.storage = app.config.app_profiler.storage || Storage::FileStorage - AppProfiler.viewer = app.config.app_profiler.viewer || Viewer::SpeedscopeViewer AppProfiler.storage.bucket_name = app.config.app_profiler.storage_bucket_name || "profiles" AppProfiler.storage.credentials = app.config.app_profiler.storage_credentials || {} AppProfiler.middleware = app.config.app_profiler.middleware || Middleware @@ -41,13 +40,13 @@ class Railtie < Rails::Railtie AppProfiler.profile_enqueue_failure = app.config.app_profiler.profile_enqueue_failure AppProfiler.after_process_queue = app.config.app_profiler.after_process_queue AppProfiler.backend = app.config.app_profiler.profiler_backend || AppProfiler::DefaultBackend + AppProfiler.gecko_viewer_package = app.config.app_profiler.gecko_viewer_package || "https://github.com/firefox-devtools/profiler" end initializer "app_profiler.add_middleware" do |app| unless AppProfiler.middleware.disabled - if AppProfiler.viewer == Viewer::SpeedscopeRemoteViewer - app.middleware.insert_before(0, Viewer::SpeedscopeRemoteViewer::Middleware) - end + app.middleware.insert_before(0, Viewer::SpeedscopeViewer::Middleware) + app.middleware.insert_before(0, Viewer::FirefoxViewer::Middleware) app.middleware.insert_before(0, AppProfiler.middleware) end end diff --git a/lib/app_profiler/viewer/base_viewer.rb b/lib/app_profiler/viewer/base_viewer.rb index 98f06618..e1ddf99a 100644 --- a/lib/app_profiler/viewer/base_viewer.rb +++ b/lib/app_profiler/viewer/base_viewer.rb @@ -1,5 +1,8 @@ # frozen_string_literal: true +gem "rails-html-sanitizer", ">= 1.6.0" +require "rails-html-sanitizer" + module AppProfiler module Viewer class BaseViewer @@ -12,6 +15,101 @@ def view(profile, params = {}) def view(_params = {}) raise NotImplementedError end + + class BaseMiddleware + class Sanitizer < Rails::HTML::Sanitizer.best_supported_vendor.safe_list_sanitizer + self.allowed_tags = Set.new([ + "strong", "em", "b", "i", "p", "code", "pre", "tt", "samp", "kbd", "var", "sub", + "sup", "dfn", "cite", "big", "small", "address", "hr", "br", "div", "span", "h1", + "h2", "h3", "h4", "h5", "h6", "ul", "ol", "li", "dl", "dt", "dd", "abbr", "acronym", + "a", "img", "blockquote", "del", "ins", "script", + ]) + end + + private_constant(:Sanitizer) + + def self.id(file) + file.basename.to_s + end + + def initialize(app) + @app = app + end + + def call(env) + request = Rack::Request.new(env) + + return index(env) if %r(\A/app_profiler/?\z).match?(request.path_info) + + @app.call(env) + end + + protected + + def id(file) + self.class.id(file) + end + + def profile_files + AppProfiler.profile_root.glob("**/*.json") + end + + def render(html) + [ + 200, + { "Content-Type" => "text/html" }, + [ + +<<~HTML, + + + + App Profiler + + + #{sanitizer.sanitize(html)} + + + HTML + ], + ] + end + + def sanitizer + @sanitizer ||= Sanitizer.new + end + + def viewer(_env, path) + raise NotImplementedError + end + + def show(env, id) + raise NotImplementedError + end + + def index(_env) + render( + (+"").tap do |content| + content << "

Profiles

" + profile_files.each do |file| + viewer = if file.to_s.end_with?(AppProfiler::VernierProfile::FILE_EXTENSION) + AppProfiler::Viewer::FirefoxViewer::NAME + else + AppProfiler::Viewer::SpeedscopeViewer::NAME + end + content << <<~HTML +

+ + #{id(file)} + +

+ HTML + end + end + ) + end + end + + private_constant(:BaseMiddleware) end end end diff --git a/lib/app_profiler/viewer/speedscope_remote_viewer.rb b/lib/app_profiler/viewer/firefox.rb similarity index 71% rename from lib/app_profiler/viewer/speedscope_remote_viewer.rb rename to lib/app_profiler/viewer/firefox.rb index ced8bbce..05b3ba41 100644 --- a/lib/app_profiler/viewer/speedscope_remote_viewer.rb +++ b/lib/app_profiler/viewer/firefox.rb @@ -1,11 +1,12 @@ # frozen_string_literal: true -require "app_profiler/viewer/speedscope_remote_viewer/base_middleware" -require "app_profiler/viewer/speedscope_remote_viewer/middleware" +require "app_profiler/viewer/middleware/firefox" module AppProfiler module Viewer - class SpeedscopeRemoteViewer < BaseViewer + class FirefoxViewer < BaseViewer + NAME = "firefox" + class << self def view(profile, params = {}) new(profile).view(**params) @@ -21,7 +22,7 @@ def view(response: nil, autoredirect: nil, async: false) id = Middleware.id(@profile.file) if response && response[0].to_i < 500 - response[1]["Location"] = "/app_profiler/#{id}" + response[1]["Location"] = "/app_profiler/#{NAME}/viewer/#{id}" response[0] = 303 else AppProfiler.logger.info("[Profiler] Profile available at /app_profiler/#{id}\n") diff --git a/lib/app_profiler/viewer/middleware/firefox.rb b/lib/app_profiler/viewer/middleware/firefox.rb new file mode 100644 index 00000000..e11188bb --- /dev/null +++ b/lib/app_profiler/viewer/middleware/firefox.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require "app_profiler/yarn/command" +require "app_profiler/yarn/with_firefox_profiler" + +module AppProfiler + module Viewer + class FirefoxViewer < BaseViewer + class Middleware < BaseMiddleware + include Yarn::WithFirefoxProfile + + def initialize(app) + super + @firefox_profiler = Rack::File.new( + File.join(AppProfiler.root, "node_modules/firefox-profiler/dist") + ) + end + + def call(env) + request = Rack::Request.new(env) + @app.call(env) if request.path_info.end_with?(AppProfiler::StackprofProfile::FILE_EXTENSION) + # Firefox profiler *really* doesn't like for /from-url/ to be at any other mount point + # so with this enabled, we take over both /app_profiler and /from-url in the app in development. + return from(env, Regexp.last_match(1)) if request.path_info =~ %r(\A/from-url(.*)\z) + return viewer(env, Regexp.last_match(1)) if request.path_info =~ %r(\A/app_profiler/firefox/viewer/(.*)\z) + return show(env, Regexp.last_match(1)) if request.path_info =~ %r(\A/app_profiler/firefox/(.*)\z) + + super + end + + protected + + attr_reader(:firefox_profiler) + + def viewer(env, path) + setup_yarn unless yarn_setup + + if path.end_with?(AppProfiler::VernierProfile::FILE_EXTENSION) + proto = env["rack.url_scheme"] + host = env["HTTP_HOST"] + source = "#{proto}://#{host}/app_profiler/firefox/#{path}" + + target = "/from-url/#{CGI.escape(source)}" + + [302, { "Location" => target }, []] + else + env[Rack::PATH_INFO] = path.delete_prefix("/app_profiler") + firefox_profiler.call(env) + end + end + + def from(env, path) + setup_yarn unless yarn_setup + index = File.read(File.join(AppProfiler.root, "node_modules/firefox-profiler/dist/index.html")) + [200, { "Content-Type" => "text/html" }, [index]] + end + + def show(_env, name) + profile = profile_files.find do |file| + id(file) == name + end || raise(ArgumentError) + + [200, { "Content-Type" => "application/json" }, [profile.read]] + end + end + end + end +end diff --git a/lib/app_profiler/viewer/middleware/speedscope.rb b/lib/app_profiler/viewer/middleware/speedscope.rb new file mode 100644 index 00000000..a1a243cc --- /dev/null +++ b/lib/app_profiler/viewer/middleware/speedscope.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require "app_profiler/yarn/command" +require "app_profiler/yarn/with_speedscope" + +module AppProfiler + module Viewer + class SpeedscopeViewer < BaseViewer + class Middleware < BaseMiddleware + include Yarn::WithSpeedscope + + def initialize(app) + super + @speedscope = Rack::File.new( + File.join(AppProfiler.root, "node_modules/speedscope/dist/release") + ) + end + + def call(env) + request = Rack::Request.new(env) + @app.call(env) if request.path_info.end_with?(AppProfiler::VernierProfile::FILE_EXTENSION) + return viewer(env, Regexp.last_match(1)) if request.path_info =~ %r(\A/app_profiler/speedscope/viewer/(.*)\z) + return show(env, Regexp.last_match(1)) if request.path_info =~ %r(\A/app_profiler/speedscope/(.*)\z) + + super + end + + protected + + attr_reader(:speedscope) + + def viewer(env, path) + setup_yarn unless yarn_setup + + if path.end_with?(AppProfiler::StackprofProfile::FILE_EXTENSION) + source = "/app_profiler/speedscope/#{path}" + target = "/app_profiler/speedscope/viewer/index.html#profileURL=#{CGI.escape(source)}" + + [302, { "Location" => target }, []] + else + env[Rack::PATH_INFO] = path.delete_prefix("/app_profiler/speedscope") + speedscope.call(env) + end + end + + def show(_env, name) + profile = profile_files.find do |file| + id(file) == name + end || raise(ArgumentError) + + [200, { "Content-Type" => "application/json" }, [profile.read]] + end + end + end + end +end diff --git a/lib/app_profiler/viewer/speedscope.rb b/lib/app_profiler/viewer/speedscope.rb new file mode 100644 index 00000000..fcfe7b31 --- /dev/null +++ b/lib/app_profiler/viewer/speedscope.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require "app_profiler/viewer/middleware/speedscope" + +module AppProfiler + module Viewer + class SpeedscopeViewer < BaseViewer + NAME = "speedscope" + + class << self + def view(profile, params = {}) + new(profile).view(**params) + end + end + + def initialize(profile) + super() + @profile = profile + end + + def view(response: nil, autoredirect: nil, async: false) + id = Middleware.id(@profile.file) + + if response && response[0].to_i < 500 + response[1]["Location"] = "/app_profiler/#{NAME}/viewer/#{id}" + response[0] = 303 + else + AppProfiler.logger.info("[Profiler] Profile available at /app_profiler/#{id}\n") + end + end + end + end +end diff --git a/lib/app_profiler/viewer/speedscope_remote_viewer/base_middleware.rb b/lib/app_profiler/viewer/speedscope_remote_viewer/base_middleware.rb deleted file mode 100644 index a6fba235..00000000 --- a/lib/app_profiler/viewer/speedscope_remote_viewer/base_middleware.rb +++ /dev/null @@ -1,102 +0,0 @@ -# frozen_string_literal: true - -gem "rails-html-sanitizer", ">= 1.6.0" -require "rails-html-sanitizer" - -module AppProfiler - module Viewer - class SpeedscopeRemoteViewer < BaseViewer - class BaseMiddleware - class Sanitizer < Rails::HTML::Sanitizer.best_supported_vendor.safe_list_sanitizer - self.allowed_tags = Set.new([ - "strong", "em", "b", "i", "p", "code", "pre", "tt", "samp", "kbd", "var", "sub", - "sup", "dfn", "cite", "big", "small", "address", "hr", "br", "div", "span", "h1", - "h2", "h3", "h4", "h5", "h6", "ul", "ol", "li", "dl", "dt", "dd", "abbr", "acronym", - "a", "img", "blockquote", "del", "ins", "script", - ]) - end - - private_constant(:Sanitizer) - - def self.id(file) - file.basename.to_s.delete_suffix(".json") - end - - def initialize(app) - @app = app - end - - def call(env) - request = Rack::Request.new(env) - - return index(env) if request.path_info =~ %r(\A/app_profiler/?\z) - return viewer(env, Regexp.last_match(1)) if request.path_info =~ %r(\A/app_profiler/viewer/(.*)\z) - return show(env, Regexp.last_match(1)) if request.path_info =~ %r(\A/app_profiler/(.*)\z) - - @app.call(env) - end - - protected - - def id(file) - self.class.id(file) - end - - def profile_files - AppProfiler.profile_root.glob("**/*.json") - end - - def render(html) - [ - 200, - { "Content-Type" => "text/html" }, - [ - +<<~HTML, - - - - App Profiler - - - #{sanitizer.sanitize(html)} - - - HTML - ], - ] - end - - def sanitizer - @sanitizer ||= Sanitizer.new - end - - def viewer(_env, path) - raise NotImplementedError - end - - def index(_env) - render( - (+"").tap do |content| - content << "

Profiles

" - profile_files.each do |file| - content << <<~HTML -

- - #{id(file)} - -

- HTML - end - end - ) - end - - def show(env, id) - raise NotImplementedError - end - end - - private_constant(:BaseMiddleware) - end - end -end diff --git a/lib/app_profiler/viewer/speedscope_remote_viewer/middleware.rb b/lib/app_profiler/viewer/speedscope_remote_viewer/middleware.rb deleted file mode 100644 index 516f83b9..00000000 --- a/lib/app_profiler/viewer/speedscope_remote_viewer/middleware.rb +++ /dev/null @@ -1,58 +0,0 @@ -# frozen_string_literal: true - -require "app_profiler/yarn/command" -require "app_profiler/yarn/with_speedscope" - -module AppProfiler - module Viewer - class SpeedscopeRemoteViewer < BaseViewer - class Middleware < BaseMiddleware - include Yarn::WithSpeedscope - - def initialize(app) - super - @speedscope = Rack::File.new( - File.join(AppProfiler.root, "node_modules/speedscope/dist/release") - ) - end - - protected - - attr_reader(:speedscope) - - def viewer(env, path) - setup_yarn unless yarn_setup - env[Rack::PATH_INFO] = path.delete_prefix("/app_profiler") - - speedscope.call(env) - end - - def show(_env, name) - profile = profile_files.find do |file| - id(file) == name - end || raise(ArgumentError) - - render( - <<~HTML - - HTML - ) - end - end - end - end -end diff --git a/lib/app_profiler/viewer/speedscope_viewer.rb b/lib/app_profiler/viewer/speedscope_viewer.rb deleted file mode 100644 index 5d758cfb..00000000 --- a/lib/app_profiler/viewer/speedscope_viewer.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -require "app_profiler/yarn/command" -require "app_profiler/yarn/with_speedscope" - -module AppProfiler - module Viewer - class SpeedscopeViewer < BaseViewer - include Yarn::WithSpeedscope - - class << self - def view(profile, params = {}) - new(profile).view(**params) - end - end - - def initialize(profile) - super() - @profile = profile - end - - def view(_params = {}) - yarn("run", "speedscope", @profile.file.to_s) - end - end - end -end diff --git a/lib/app_profiler/yarn/command.rb b/lib/app_profiler/yarn/command.rb index 3866325a..42061da1 100644 --- a/lib/app_profiler/yarn/command.rb +++ b/lib/app_profiler/yarn/command.rb @@ -5,15 +5,20 @@ module Yarn module Command class YarnError < StandardError; end + GECKO_VIEWER_PACKAGE = AppProfiler.gecko_viewer_package + VALID_COMMANDS = [ ["which", "yarn"], ["yarn", "init", "--yes"], ["yarn", "add", "speedscope", "--dev", "--ignore-workspace-root-check"], ["yarn", "run", "speedscope", /.*\.json/], + ["yarn", "add", "--dev", %r{.*/firefox-profiler}], + ["yarn", "--cwd", %r{.*/firefox-profiler}], + ["yarn", "--cwd", %r{.*/firefox-profiler}, "build-prod"], ] private_constant(:VALID_COMMANDS) - mattr_accessor(:yarn_setup, default: false) + private_constant(:GECKO_VIEWER_PACKAGE) def yarn(command, *options) setup_yarn unless yarn_setup @@ -29,6 +34,10 @@ def setup_yarn yarn("init", "--yes") unless package_json_exists? end + def yarn_setup + @yarn_initialized || false + end + private def ensure_command_valid(command) @@ -39,6 +48,8 @@ def ensure_command_valid(command) def valid_command?(command) VALID_COMMANDS.any? do |valid_command| + next unless valid_command.size == command.size + valid_command.zip(command).all? do |valid_part, part| part.match?(valid_part) end @@ -55,7 +66,7 @@ def ensure_yarn_installed MSG ) end - self.yarn_setup = true + @yarn_initialized = true end def package_json_exists? diff --git a/lib/app_profiler/yarn/with_firefox_profiler.rb b/lib/app_profiler/yarn/with_firefox_profiler.rb new file mode 100644 index 00000000..38fa6e1a --- /dev/null +++ b/lib/app_profiler/yarn/with_firefox_profiler.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +module AppProfiler + module Yarn + module WithFirefoxProfile + include Command + + def setup_yarn + super + return if firefox_profiler_added? + + fetch_firefox_profiler + end + + private + + def firefox_profiler_added? + AppProfiler.root.join("node_modules/firefox-profiler/dist").exist? + end + + def fetch_firefox_profiler + raise ArgumentError unless AppProfiler.gecko_viewer_package.start_with?("https://github.com") + + repo, branch = AppProfiler.gecko_viewer_package.to_s.split("#") + + dir = "./tmp" + FileUtils.mkdir_p(dir) + Dir.chdir(dir) do + clone_args = ["git", "clone", repo, "firefox-profiler"] + clone_args.push("--branch=#{branch}") unless branch.nil? || branch&.empty? + system(*clone_args) + package_contents = File.read("firefox-profiler/package.json") + package_json = JSON.parse(package_contents) + package_json["name"] ||= "firefox-profiler" + package_json["version"] ||= "0.0.1" + File.write("firefox-profiler/package.json", package_json.to_json) + end + yarn("--cwd", "#{dir}/firefox-profiler") + + patch_firefox_profiler(dir) + yarn("--cwd", "#{dir}/firefox-profiler", "build-prod") + patch_file("#{dir}/firefox-profiler/dist/index.html", 'href="locales/en-US/app.ftl"', + 'href="/app_profiler/firefox/viewer/locales/en-US/app.ftl"') + + yarn("add", "--dev", "#{dir}/firefox-profiler") + end + + def patch_firefox_profiler(dir) + # Patch the publicPath so that the app can be "mounted" at the right location + patch_file("#{dir}/firefox-profiler/webpack.config.js", "publicPath: '/'", + "publicPath: '/app_profiler/firefox/viewer/'") + patch_file("#{dir}/firefox-profiler/src/app-logic/l10n.js", "fetch(`/locales/", + "fetch(`/app_profiler/firefox/viewer/locales/") + end + + def patch_file(file, find, replace) + contents = File.read(file) + new_contents = contents.gsub(find, replace) + File.write(file, new_contents) + end + end + end +end diff --git a/test/app_profiler/viewer/firefox_viewer/middleware_test.rb b/test/app_profiler/viewer/firefox_viewer/middleware_test.rb new file mode 100644 index 00000000..5fdac154 --- /dev/null +++ b/test/app_profiler/viewer/firefox_viewer/middleware_test.rb @@ -0,0 +1,115 @@ +# frozen_string_literal: true + +require "test_helper" + +module AppProfiler + module Viewer + class FirefoxViewer + class MiddlewareTest < TestCase + setup do + @app = Middleware.new( + proc { [200, { "Content-Type" => "text/plain" }, ["Hello world!"]] } + ) + end + + test ".id" do + profile = VernierProfile.new(vernier_profile) + profile_id = profile.file.basename.to_s + + assert_equal(profile_id, Middleware.id(profile.file)) + end + + test "#call index" do + profiles = Array.new(3) { VernierProfile.new(vernier_profile).tap(&:file) } + + code, content_type, html = @app.call({ "PATH_INFO" => "/app_profiler" }) + html = html.first + + assert_equal(200, code) + assert_equal({ "Content-Type" => "text/html" }, content_type) + assert_match(%r(App Profiler), html) + profiles.each do |profile| + id = Middleware.id(profile.file) + assert_match( + %r(), html + ) + end + end + + test "#call index with slash" do + profiles = Array.new(3) { VernierProfile.new(vernier_profile).tap(&:file) } + + code, content_type, html = @app.call({ "PATH_INFO" => "/app_profiler/" }) + html = html.first + + assert_equal(200, code) + assert_equal({ "Content-Type" => "text/html" }, content_type) + assert_match(%r(App Profiler), html) + profiles.each do |profile| + id = Middleware.id(profile.file) + assert_match( + %r(), html + ) + end + end + + test "#call show" do + profile = VernierProfile.new(vernier_profile) + id = Middleware.id(profile.file) + + code, content_type, body = @app.call({ "PATH_INFO" => "/app_profiler/firefox/#{id}" }) + + assert_equal(200, code) + assert_equal({ "Content-Type" => "application/json" }, content_type) + assert_equal(JSON.dump(profile.to_h), body.first) + end + + test "#call viewer sets up yarn" do + @app.expects(:system).with("which", "yarn", out: File::NULL).returns(true) + @app.expects(:system).with("yarn", "init", "--yes").returns(true) + + @app.expects(:system).with("git", "clone", AppProfiler.gecko_viewer_package, "firefox-profiler").returns(true) + + File.expects(:read).returns("{}") + File.expects(:write).returns(true) + + dir = "./tmp" + + @app.expects(:system).with("yarn", "--cwd", "#{dir}/firefox-profiler").returns(true) + + File.expects(:read).with("#{dir}/firefox-profiler/webpack.config.js").returns("") + File.expects(:write).with("#{dir}/firefox-profiler/webpack.config.js", "").returns(true) + + File.expects(:read).with("#{dir}/firefox-profiler/src/app-logic/l10n.js").returns("") + File.expects(:write).with("#{dir}/firefox-profiler/src/app-logic/l10n.js", "").returns(true) + + @app.expects(:system).with("yarn", "--cwd", "#{dir}/firefox-profiler", "build-prod").returns(true) + + File.expects(:read).with("#{dir}/firefox-profiler/dist/index.html").returns("") + File.expects(:write).with("#{dir}/firefox-profiler/dist/index.html", "").returns(true) + + @app.expects(:system).with("yarn", "add", "--dev", "#{dir}/firefox-profiler").returns(true) + @app.call({ "PATH_INFO" => "/app_profiler/firefox/viewer/index.html" }) + + assert_predicate(@app, :yarn_setup) + end + + test "#call viewer" do + with_yarn_setup(@app) do + @app.expects(:firefox_profiler).returns(proc { [200, { "Content-Type" => "text/plain" }, ["Firefox"]] }) + + response = @app.call({ "PATH_INFO" => "/app_profiler/firefox/viewer/index.html" }) + + assert_equal([200, { "Content-Type" => "text/plain" }, ["Firefox"]], response) + end + end + + test "#call" do + response = @app.call({ "PATH_INFO" => "/app_level_route" }) + + assert_equal([200, { "Content-Type" => "text/plain" }, ["Hello world!"]], response) + end + end + end + end +end diff --git a/test/app_profiler/viewer/firefox_viewer_test.rb b/test/app_profiler/viewer/firefox_viewer_test.rb new file mode 100644 index 00000000..46a3b25b --- /dev/null +++ b/test/app_profiler/viewer/firefox_viewer_test.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require "test_helper" + +module AppProfiler + module Viewer + class FirefoxViewerTest < TestCase + test ".view initializes and calls #view" do + FirefoxViewer.any_instance.expects(:view) + + profile = VernierProfile.new(vernier_profile) + FirefoxViewer.view(profile) + end + + test "#view logs middleware URL" do + profile = VernierProfile.new(vernier_profile) + + viewer = FirefoxViewer.new(profile) + id = FirefoxViewer::Middleware.id(profile.file) + + AppProfiler.logger.expects(:info).with( + "[Profiler] Profile available at /app_profiler/#{id}\n" + ) + + viewer.view + end + + test "#view with response redirects to URL" do + response = [200, {}, ["OK"]] + profile = VernierProfile.new(vernier_profile) + + viewer = FirefoxViewer.new(profile) + id = FirefoxViewer::Middleware.id(profile.file) + + viewer.view(response: response) + + assert_equal(303, response[0]) + assert_equal("/app_profiler/firefox/viewer/#{id}", response[1]["Location"]) + end + end + end +end diff --git a/test/app_profiler/viewer/speedscope_remote_viewer/middleware_test.rb b/test/app_profiler/viewer/speedscope_viewer/middleware_test.rb similarity index 80% rename from test/app_profiler/viewer/speedscope_remote_viewer/middleware_test.rb rename to test/app_profiler/viewer/speedscope_viewer/middleware_test.rb index 36a39c64..e3def965 100644 --- a/test/app_profiler/viewer/speedscope_remote_viewer/middleware_test.rb +++ b/test/app_profiler/viewer/speedscope_viewer/middleware_test.rb @@ -4,7 +4,7 @@ module AppProfiler module Viewer - class SpeedscopeRemoteViewer + class SpeedscopeViewer class MiddlewareTest < TestCase setup do @app = Middleware.new( @@ -15,7 +15,6 @@ class MiddlewareTest < TestCase test ".id" do profile = AbstractProfile.from_stackprof(stackprof_profile) profile_id = profile.file.basename.to_s.delete_suffix(".json") - assert_equal(profile_id, Middleware.id(profile.file)) end @@ -29,7 +28,10 @@ class MiddlewareTest < TestCase assert_equal({ "Content-Type" => "text/html" }, content_type) assert_match(%r(App Profiler), html) profiles.each do |profile| - assert_match(%r(), html) + id = Middleware.id(profile.file) + assert_match( + %r(), html + ) end end @@ -43,7 +45,10 @@ class MiddlewareTest < TestCase assert_equal({ "Content-Type" => "text/html" }, content_type) assert_match(%r(App Profiler), html) profiles.each do |profile| - assert_match(%r(), html) + id = Middleware.id(profile.file) + assert_match( + %r(), html + ) end end @@ -51,8 +56,7 @@ class MiddlewareTest < TestCase profile = AbstractProfile.from_stackprof(stackprof_profile) id = Middleware.id(profile.file) - code, content_type, html = @app.call({ "PATH_INFO" => "/app_profiler/#{id}" }) - html = html.first + code, content_type, body = @app.call({ "PATH_INFO" => "/app_profiler/speedscope/#{id}" }) assert_equal(200, code) assert_equal({ "Content-Type" => "text/html" }, content_type) @@ -73,6 +77,8 @@ class MiddlewareTest < TestCase html[-200..-1], message: "The generated HTML was incomplete" ) + assert_equal({ "Content-Type" => "application/json" }, content_type) + assert_equal(JSON.dump(profile.to_h), body.first) end test "#call viewer sets up yarn" do @@ -82,18 +88,16 @@ class MiddlewareTest < TestCase "yarn", "add", "speedscope", "--dev", "--ignore-workspace-root-check" ).returns(true) - @app.call({ "PATH_INFO" => "/app_profiler/viewer/index.html" }) + @app.call({ "PATH_INFO" => "/app_profiler/speedscope/viewer/index.html" }) - assert_predicate(Yarn::Command, :yarn_setup) - ensure - Yarn::Command.yarn_setup = false + assert_predicate(@app, :yarn_setup) end test "#call viewer" do - with_yarn_setup do + with_yarn_setup(@app) do @app.expects(:speedscope).returns(proc { [200, { "Content-Type" => "text/plain" }, ["Speedscope"]] }) - response = @app.call({ "PATH_INFO" => "/app_profiler/viewer/index.html" }) + response = @app.call({ "PATH_INFO" => "/app_profiler/speedscope/viewer/index.html" }) assert_equal([200, { "Content-Type" => "text/plain" }, ["Speedscope"]], response) end diff --git a/test/app_profiler/yarn/command_test.rb b/test/app_profiler/yarn/command_test.rb index 652dd224..11792536 100644 --- a/test/app_profiler/yarn/command_test.rb +++ b/test/app_profiler/yarn/command_test.rb @@ -8,11 +8,11 @@ class CommandTest < TestCase include(Command) setup do - Command.yarn_setup = true + @yarn_initialized = true end teardown do - Command.yarn_setup = false + @yarn_initialized = false end test "#yarn allows add speedscope" do diff --git a/test/test_helper.rb b/test/test_helper.rb index 55cdced6..9df26695 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -84,12 +84,12 @@ def vernier_params(params = {}) { mode: :wall, interval: 1000, metadata: { id: "foo" } }.merge(params) end - def with_yarn_setup - old_yarn_setup = Yarn::Command.yarn_setup - Yarn::Command.yarn_setup = true + def with_yarn_setup(app) + old_yarn_setup = app.yarn_setup + app.instance_variable_set(:@yarn_initialized, true) yield ensure - Yarn::Command.yarn_setup = old_yarn_setup + app.instance_variable_set(:@yarn_initialized, old_yarn_setup) end end end From 358806c63b8a044658b9984f4b985f1b57558590 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Wed, 24 Jan 2024 14:04:05 -0500 Subject: [PATCH 24/34] Make firefox-profiler the viewer for vernier profiles --- lib/app_profiler/profile/vernier.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/app_profiler/profile/vernier.rb b/lib/app_profiler/profile/vernier.rb index 02f267b8..f4373873 100644 --- a/lib/app_profiler/profile/vernier.rb +++ b/lib/app_profiler/profile/vernier.rb @@ -13,7 +13,7 @@ def format end def view(params = {}) - raise NotImplementedError + Viewer::FirefoxViewer.view(self, **params) end end end From 6cc32fbb260bcedb0dda71e8493de9fbd2be401b Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Wed, 24 Jan 2024 14:05:19 -0500 Subject: [PATCH 25/34] Update readme to specify viewer --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 2e6c079d..c739d75e 100644 --- a/README.md +++ b/README.md @@ -328,6 +328,8 @@ Rails.application.config.app_profiler.backend = AppProfiler::StackprofBackend # By default, the stackprof backend will be used. +In local development, changing the backend will change whether the profile is viewed in speedscope or firefox-profiler. + ## Running tests ``` From 12f45f24f74244587de8d1c76209cfee9bf76473 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Thu, 25 Jan 2024 13:24:58 -0500 Subject: [PATCH 26/34] Only insert viewer middlewares in dev or test --- lib/app_profiler/railtie.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/app_profiler/railtie.rb b/lib/app_profiler/railtie.rb index fb89fc00..48966bc1 100644 --- a/lib/app_profiler/railtie.rb +++ b/lib/app_profiler/railtie.rb @@ -45,8 +45,10 @@ class Railtie < Rails::Railtie initializer "app_profiler.add_middleware" do |app| unless AppProfiler.middleware.disabled - app.middleware.insert_before(0, Viewer::SpeedscopeViewer::Middleware) - app.middleware.insert_before(0, Viewer::FirefoxViewer::Middleware) + if Rails.env.development? || Rails.env.test? + app.middleware.insert_before(0, Viewer::SpeedscopeViewer::Middleware) + app.middleware.insert_before(0, Viewer::FirefoxViewer::Middleware) + end app.middleware.insert_before(0, AppProfiler.middleware) end end From e72518267a5171b02298a12124d0c4d55d2a8c42 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Tue, 30 Jan 2024 11:20:19 -0500 Subject: [PATCH 27/34] Revert "Fix default viewer" This reverts commit d7940d32bbb884f0e68a19502a3539549d1049a5. --- lib/app_profiler/profile/stackprof.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/app_profiler/profile/stackprof.rb b/lib/app_profiler/profile/stackprof.rb index daff1474..27c18747 100644 --- a/lib/app_profiler/profile/stackprof.rb +++ b/lib/app_profiler/profile/stackprof.rb @@ -13,7 +13,7 @@ def format end def view(params = {}) - AppProfiler.viewer.view(self, **params) + Viewer::SpeedscopeViewer.view(self, **params) end end end From 84b21466ea7669b94fa3947a324e8b1aa6307d92 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Fri, 9 Feb 2024 11:55:10 -0500 Subject: [PATCH 28/34] Bring back local speedscope viewer, rename middleware-based viewers to remote --- lib/app_profiler.rb | 5 +-- lib/app_profiler/viewer/base_viewer.rb | 4 +-- lib/app_profiler/viewer/middleware/firefox.rb | 2 +- .../viewer/middleware/speedscope.rb | 2 +- .../viewer/{ => remote}/firefox.rb | 2 +- lib/app_profiler/viewer/remote/speedscope.rb | 33 +++++++++++++++++++ lib/app_profiler/viewer/speedscope.rb | 16 +++------ lib/app_profiler/yarn/command.rb | 4 +++ .../{ => remote}/firefox_viewer_test.rb | 14 ++++---- .../middleware/firefox_middleware_test.rb} | 5 +-- .../middleware/speedscope_middleware_test.rb} | 23 ++----------- .../speedscope_viewer_test.rb} | 2 +- .../viewer/speedscope_viewer_test.rb | 19 ++++------- 13 files changed, 71 insertions(+), 60 deletions(-) rename lib/app_profiler/viewer/{ => remote}/firefox.rb (94%) create mode 100644 lib/app_profiler/viewer/remote/speedscope.rb rename test/app_profiler/viewer/{ => remote}/firefox_viewer_test.rb (68%) rename test/app_profiler/viewer/{firefox_viewer/middleware_test.rb => remote/middleware/firefox_middleware_test.rb} (95%) rename test/app_profiler/viewer/{speedscope_viewer/middleware_test.rb => remote/middleware/speedscope_middleware_test.rb} (80%) rename test/app_profiler/viewer/{speedscope_remote_viewer_test.rb => remote/speedscope_viewer_test.rb} (92%) diff --git a/lib/app_profiler.rb b/lib/app_profiler.rb index 180f670e..e393c819 100644 --- a/lib/app_profiler.rb +++ b/lib/app_profiler.rb @@ -29,7 +29,8 @@ module Storage module Viewer autoload :BaseViewer, "app_profiler/viewer/base_viewer" autoload :SpeedscopeViewer, "app_profiler/viewer/speedscope" - autoload :FirefoxViewer, "app_profiler/viewer/firefox" + autoload :SpeedscopeRemoteViewer, "app_profiler/viewer/remote/speedscope" + autoload :FirefoxRemoteViewer, "app_profiler/viewer/remote/firefox" end require "app_profiler/middleware" @@ -51,7 +52,7 @@ module Viewer mattr_reader :profile_url_formatter, default: DefaultProfileFormatter - mattr_accessor :gecko_viewer_package, default: "https://github.com/firefox-devtools/profiler" + mattr_accessor :gecko_viewer_package, default: "https://github.com/tenderlove/profiler#v0.0.2" mattr_accessor :storage, default: Storage::FileStorage mattr_accessor :middleware, default: Middleware mattr_accessor :server, default: Server diff --git a/lib/app_profiler/viewer/base_viewer.rb b/lib/app_profiler/viewer/base_viewer.rb index e1ddf99a..6008f925 100644 --- a/lib/app_profiler/viewer/base_viewer.rb +++ b/lib/app_profiler/viewer/base_viewer.rb @@ -92,9 +92,9 @@ def index(_env) content << "

Profiles

" profile_files.each do |file| viewer = if file.to_s.end_with?(AppProfiler::VernierProfile::FILE_EXTENSION) - AppProfiler::Viewer::FirefoxViewer::NAME + AppProfiler::Viewer::FirefoxRemoteViewer::NAME else - AppProfiler::Viewer::SpeedscopeViewer::NAME + AppProfiler::Viewer::SpeedscopeRemoteViewer::NAME end content << <<~HTML

diff --git a/lib/app_profiler/viewer/middleware/firefox.rb b/lib/app_profiler/viewer/middleware/firefox.rb index e11188bb..3b2ec9c1 100644 --- a/lib/app_profiler/viewer/middleware/firefox.rb +++ b/lib/app_profiler/viewer/middleware/firefox.rb @@ -5,7 +5,7 @@ module AppProfiler module Viewer - class FirefoxViewer < BaseViewer + class FirefoxRemoteViewer < BaseViewer class Middleware < BaseMiddleware include Yarn::WithFirefoxProfile diff --git a/lib/app_profiler/viewer/middleware/speedscope.rb b/lib/app_profiler/viewer/middleware/speedscope.rb index a1a243cc..cdc19429 100644 --- a/lib/app_profiler/viewer/middleware/speedscope.rb +++ b/lib/app_profiler/viewer/middleware/speedscope.rb @@ -5,7 +5,7 @@ module AppProfiler module Viewer - class SpeedscopeViewer < BaseViewer + class SpeedscopeRemoteViewer < BaseViewer class Middleware < BaseMiddleware include Yarn::WithSpeedscope diff --git a/lib/app_profiler/viewer/firefox.rb b/lib/app_profiler/viewer/remote/firefox.rb similarity index 94% rename from lib/app_profiler/viewer/firefox.rb rename to lib/app_profiler/viewer/remote/firefox.rb index 05b3ba41..2099dd55 100644 --- a/lib/app_profiler/viewer/firefox.rb +++ b/lib/app_profiler/viewer/remote/firefox.rb @@ -4,7 +4,7 @@ module AppProfiler module Viewer - class FirefoxViewer < BaseViewer + class FirefoxRemoteViewer < BaseViewer NAME = "firefox" class << self diff --git a/lib/app_profiler/viewer/remote/speedscope.rb b/lib/app_profiler/viewer/remote/speedscope.rb new file mode 100644 index 00000000..4b72fca5 --- /dev/null +++ b/lib/app_profiler/viewer/remote/speedscope.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require "app_profiler/viewer/middleware/speedscope" + +module AppProfiler + module Viewer + class SpeedscopeRemoteViewer < BaseViewer + NAME = "speedscope" + + class << self + def view(profile, params = {}) + new(profile).view(**params) + end + end + + def initialize(profile) + super() + @profile = profile + end + + def view(response: nil, autoredirect: nil, async: false) + id = Middleware.id(@profile.file) + + if response && response[0].to_i < 500 + response[1]["Location"] = "/app_profiler/#{NAME}/viewer/#{id}" + response[0] = 303 + else + AppProfiler.logger.info("[Profiler] Profile available at /app_profiler/#{id}\n") + end + end + end + end +end diff --git a/lib/app_profiler/viewer/speedscope.rb b/lib/app_profiler/viewer/speedscope.rb index fcfe7b31..5d758cfb 100644 --- a/lib/app_profiler/viewer/speedscope.rb +++ b/lib/app_profiler/viewer/speedscope.rb @@ -1,11 +1,12 @@ # frozen_string_literal: true -require "app_profiler/viewer/middleware/speedscope" +require "app_profiler/yarn/command" +require "app_profiler/yarn/with_speedscope" module AppProfiler module Viewer class SpeedscopeViewer < BaseViewer - NAME = "speedscope" + include Yarn::WithSpeedscope class << self def view(profile, params = {}) @@ -18,15 +19,8 @@ def initialize(profile) @profile = profile end - def view(response: nil, autoredirect: nil, async: false) - id = Middleware.id(@profile.file) - - if response && response[0].to_i < 500 - response[1]["Location"] = "/app_profiler/#{NAME}/viewer/#{id}" - response[0] = 303 - else - AppProfiler.logger.info("[Profiler] Profile available at /app_profiler/#{id}\n") - end + def view(_params = {}) + yarn("run", "speedscope", @profile.file.to_s) end end end diff --git a/lib/app_profiler/yarn/command.rb b/lib/app_profiler/yarn/command.rb index 42061da1..071ab78f 100644 --- a/lib/app_profiler/yarn/command.rb +++ b/lib/app_profiler/yarn/command.rb @@ -38,6 +38,10 @@ def yarn_setup @yarn_initialized || false end + def yarn_setup=(state) + @yarn_initialized = state + end + private def ensure_command_valid(command) diff --git a/test/app_profiler/viewer/firefox_viewer_test.rb b/test/app_profiler/viewer/remote/firefox_viewer_test.rb similarity index 68% rename from test/app_profiler/viewer/firefox_viewer_test.rb rename to test/app_profiler/viewer/remote/firefox_viewer_test.rb index 46a3b25b..baf8b1e7 100644 --- a/test/app_profiler/viewer/firefox_viewer_test.rb +++ b/test/app_profiler/viewer/remote/firefox_viewer_test.rb @@ -4,19 +4,19 @@ module AppProfiler module Viewer - class FirefoxViewerTest < TestCase + class FirefoxRemoteViewerTest < TestCase test ".view initializes and calls #view" do - FirefoxViewer.any_instance.expects(:view) + FirefoxRemoteViewer.any_instance.expects(:view) profile = VernierProfile.new(vernier_profile) - FirefoxViewer.view(profile) + FirefoxRemoteViewer.view(profile) end test "#view logs middleware URL" do profile = VernierProfile.new(vernier_profile) - viewer = FirefoxViewer.new(profile) - id = FirefoxViewer::Middleware.id(profile.file) + viewer = FirefoxRemoteViewer.new(profile) + id = FirefoxRemoteViewer::Middleware.id(profile.file) AppProfiler.logger.expects(:info).with( "[Profiler] Profile available at /app_profiler/#{id}\n" @@ -29,8 +29,8 @@ class FirefoxViewerTest < TestCase response = [200, {}, ["OK"]] profile = VernierProfile.new(vernier_profile) - viewer = FirefoxViewer.new(profile) - id = FirefoxViewer::Middleware.id(profile.file) + viewer = FirefoxRemoteViewer.new(profile) + id = FirefoxRemoteViewer::Middleware.id(profile.file) viewer.view(response: response) diff --git a/test/app_profiler/viewer/firefox_viewer/middleware_test.rb b/test/app_profiler/viewer/remote/middleware/firefox_middleware_test.rb similarity index 95% rename from test/app_profiler/viewer/firefox_viewer/middleware_test.rb rename to test/app_profiler/viewer/remote/middleware/firefox_middleware_test.rb index 5fdac154..d31a3c1c 100644 --- a/test/app_profiler/viewer/firefox_viewer/middleware_test.rb +++ b/test/app_profiler/viewer/remote/middleware/firefox_middleware_test.rb @@ -4,7 +4,7 @@ module AppProfiler module Viewer - class FirefoxViewer + class FirefoxRemoteViewer class MiddlewareTest < TestCase setup do @app = Middleware.new( @@ -68,7 +68,8 @@ class MiddlewareTest < TestCase @app.expects(:system).with("which", "yarn", out: File::NULL).returns(true) @app.expects(:system).with("yarn", "init", "--yes").returns(true) - @app.expects(:system).with("git", "clone", AppProfiler.gecko_viewer_package, "firefox-profiler").returns(true) + url, branch = AppProfiler.gecko_viewer_package.split("#") + @app.expects(:system).with("git", "clone", url, "firefox-profiler", "--branch=#{branch}").returns(true) File.expects(:read).returns("{}") File.expects(:write).returns(true) diff --git a/test/app_profiler/viewer/speedscope_viewer/middleware_test.rb b/test/app_profiler/viewer/remote/middleware/speedscope_middleware_test.rb similarity index 80% rename from test/app_profiler/viewer/speedscope_viewer/middleware_test.rb rename to test/app_profiler/viewer/remote/middleware/speedscope_middleware_test.rb index e3def965..32308977 100644 --- a/test/app_profiler/viewer/speedscope_viewer/middleware_test.rb +++ b/test/app_profiler/viewer/remote/middleware/speedscope_middleware_test.rb @@ -4,7 +4,7 @@ module AppProfiler module Viewer - class SpeedscopeViewer + class SpeedscopeRemoteViewer class MiddlewareTest < TestCase setup do @app = Middleware.new( @@ -14,7 +14,8 @@ class MiddlewareTest < TestCase test ".id" do profile = AbstractProfile.from_stackprof(stackprof_profile) - profile_id = profile.file.basename.to_s.delete_suffix(".json") + profile_id = profile.file.basename.to_s + assert_equal(profile_id, Middleware.id(profile.file)) end @@ -59,24 +60,6 @@ class MiddlewareTest < TestCase code, content_type, body = @app.call({ "PATH_INFO" => "/app_profiler/speedscope/#{id}" }) assert_equal(200, code) - assert_equal({ "Content-Type" => "text/html" }, content_type) - assert_match(%r(App Profiler), html) - assert_match(%r(}, - html[-200..-1], - message: "The generated HTML was incomplete" - ) assert_equal({ "Content-Type" => "application/json" }, content_type) assert_equal(JSON.dump(profile.to_h), body.first) end diff --git a/test/app_profiler/viewer/speedscope_remote_viewer_test.rb b/test/app_profiler/viewer/remote/speedscope_viewer_test.rb similarity index 92% rename from test/app_profiler/viewer/speedscope_remote_viewer_test.rb rename to test/app_profiler/viewer/remote/speedscope_viewer_test.rb index 4bb5b3e5..08f78306 100644 --- a/test/app_profiler/viewer/speedscope_remote_viewer_test.rb +++ b/test/app_profiler/viewer/remote/speedscope_viewer_test.rb @@ -35,7 +35,7 @@ class SpeedscopeRemoteViewerTest < TestCase viewer.view(response: response) assert_equal(303, response[0]) - assert_equal("/app_profiler/#{id}", response[1]["Location"]) + assert_equal("/app_profiler/speedscope/viewer/#{id}", response[1]["Location"]) end end end diff --git a/test/app_profiler/viewer/speedscope_viewer_test.rb b/test/app_profiler/viewer/speedscope_viewer_test.rb index 029bdfa0..2d1bd6b6 100644 --- a/test/app_profiler/viewer/speedscope_viewer_test.rb +++ b/test/app_profiler/viewer/speedscope_viewer_test.rb @@ -25,9 +25,7 @@ class SpeedscopeViewerTest < TestCase viewer.view - assert_predicate(Yarn::Command, :yarn_setup) - ensure - Yarn::Command.yarn_setup = false + assert_predicate(viewer, :yarn_setup) end test "#view doesn't init when package.json exists" do @@ -45,9 +43,8 @@ class SpeedscopeViewerTest < TestCase viewer.view - assert_predicate(Yarn::Command, :yarn_setup) + assert_predicate(viewer, :yarn_setup) ensure - Yarn::Command.yarn_setup = false AppProfiler.root.rmtree end @@ -64,21 +61,19 @@ class SpeedscopeViewerTest < TestCase viewer.view - assert_predicate(Yarn::Command, :yarn_setup) + assert_predicate(viewer, :yarn_setup) ensure - Yarn::Command.yarn_setup = false AppProfiler.root.rmtree end test "#view only opens profile in speedscope if yarn is already setup" do profile = AbstractProfile.from_stackprof(stackprof_profile) - with_yarn_setup do - viewer = SpeedscopeViewer.new(profile) - viewer.expects(:system).with("yarn", "run", "speedscope", profile.file.to_s).returns(true) + viewer = SpeedscopeViewer.new(profile) + viewer.yarn_setup = true + viewer.expects(:system).with("yarn", "run", "speedscope", profile.file.to_s).returns(true) - viewer.view - end + viewer.view end test "#view raises YarnError when yarn command fails" do From a067ddd32794b90f2ad648c2b94b213a4e9e31bb Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Fri, 9 Feb 2024 12:59:02 -0500 Subject: [PATCH 29/34] Use AppProfiler.speedscope_viewer to set viewer for stackprof, deprecated AppProfiler.viewer --- lib/app_profiler.rb | 13 +++++++++++++ lib/app_profiler/profile/stackprof.rb | 2 +- lib/app_profiler/railtie.rb | 2 ++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/lib/app_profiler.rb b/lib/app_profiler.rb index e393c819..a015306b 100644 --- a/lib/app_profiler.rb +++ b/lib/app_profiler.rb @@ -54,6 +54,8 @@ module Viewer mattr_accessor :gecko_viewer_package, default: "https://github.com/tenderlove/profiler#v0.0.2" mattr_accessor :storage, default: Storage::FileStorage + mattr_accessor :viewer, default: Viewer::SpeedscopeViewer # DEPRECATED + mattr_accessor :speedscope_viewer, default: Viewer::SpeedscopeViewer mattr_accessor :middleware, default: Middleware mattr_accessor :server, default: Server mattr_accessor :upload_queue_max_length, default: 10 @@ -180,6 +182,17 @@ def profile_url(upload) AppProfiler.profile_url_formatter.call(upload) end + + # DEPRECATIONS + def viewer + ActiveSupport::Deprecation.warn("viewer is deprecated, use speedscope_viewer instead") + @viewer + end + + def viewer=(viewer) + ActiveSupport::Deprecation.warn("viewer= is deprecated, use speedscope_viewer= instead") + @viewer = viewer + end end require "app_profiler/railtie" if defined?(Rails::Railtie) diff --git a/lib/app_profiler/profile/stackprof.rb b/lib/app_profiler/profile/stackprof.rb index 27c18747..b66338af 100644 --- a/lib/app_profiler/profile/stackprof.rb +++ b/lib/app_profiler/profile/stackprof.rb @@ -13,7 +13,7 @@ def format end def view(params = {}) - Viewer::SpeedscopeViewer.view(self, **params) + AppProfiler.speedscope_viewer.view(self, **params) end end end diff --git a/lib/app_profiler/railtie.rb b/lib/app_profiler/railtie.rb index 48966bc1..1b7d549a 100644 --- a/lib/app_profiler/railtie.rb +++ b/lib/app_profiler/railtie.rb @@ -11,6 +11,8 @@ class Railtie < Rails::Railtie AppProfiler.logger = app.config.app_profiler.logger || Rails.logger AppProfiler.root = app.config.app_profiler.root || Rails.root AppProfiler.storage = app.config.app_profiler.storage || Storage::FileStorage + AppProfiler.viewer = app.config.app_profiler.viewer || Viewer::SpeedscopeViewer + AppProfiler.speedscope_viewer = app.config.app_profiler.speedscope_viewer || AppProfiler.viewer AppProfiler.storage.bucket_name = app.config.app_profiler.storage_bucket_name || "profiles" AppProfiler.storage.credentials = app.config.app_profiler.storage_credentials || {} AppProfiler.middleware = app.config.app_profiler.middleware || Middleware From dd8fe9076124d16035755d1c99df53e682bb0356 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Fri, 9 Feb 2024 13:03:38 -0500 Subject: [PATCH 30/34] Readme update --- README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/README.md b/README.md index c739d75e..2477edc0 100644 --- a/README.md +++ b/README.md @@ -287,6 +287,11 @@ report.view # opens the profile locally in speedscope or firefox profiler, as ap Profile files can be found locally in your rails app at `tmp/app_profiler/*.json`. +**Note** In development, if using the SpeedscopeRemoteViewer for stackprof +or if using Vernier, a route for `/app_profiler` will be added to the application. +If using Vernier, a route for `/from-url` is also added. These will be handled +in middlewares, before any application routing logic. There is a small chance +that these could shadow existing routes in the application. ## Storage backends From c94424c42e8fea2c3f87cbb22784c223ab445044 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Fri, 9 Feb 2024 13:09:58 -0500 Subject: [PATCH 31/34] Bug fixes --- lib/app_profiler/profile/vernier.rb | 2 +- lib/app_profiler/railtie.rb | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/app_profiler/profile/vernier.rb b/lib/app_profiler/profile/vernier.rb index f4373873..d0008796 100644 --- a/lib/app_profiler/profile/vernier.rb +++ b/lib/app_profiler/profile/vernier.rb @@ -13,7 +13,7 @@ def format end def view(params = {}) - Viewer::FirefoxViewer.view(self, **params) + Viewer::FirefoxRemoteViewer.view(self, **params) end end end diff --git a/lib/app_profiler/railtie.rb b/lib/app_profiler/railtie.rb index 1b7d549a..75974cfd 100644 --- a/lib/app_profiler/railtie.rb +++ b/lib/app_profiler/railtie.rb @@ -11,7 +11,7 @@ class Railtie < Rails::Railtie AppProfiler.logger = app.config.app_profiler.logger || Rails.logger AppProfiler.root = app.config.app_profiler.root || Rails.root AppProfiler.storage = app.config.app_profiler.storage || Storage::FileStorage - AppProfiler.viewer = app.config.app_profiler.viewer || Viewer::SpeedscopeViewer + AppProfiler.viewer = app.config.app_profiler.viewer || Viewer::SpeedscopeRemoteViewer AppProfiler.speedscope_viewer = app.config.app_profiler.speedscope_viewer || AppProfiler.viewer AppProfiler.storage.bucket_name = app.config.app_profiler.storage_bucket_name || "profiles" AppProfiler.storage.credentials = app.config.app_profiler.storage_credentials || {} @@ -48,8 +48,10 @@ class Railtie < Rails::Railtie initializer "app_profiler.add_middleware" do |app| unless AppProfiler.middleware.disabled if Rails.env.development? || Rails.env.test? - app.middleware.insert_before(0, Viewer::SpeedscopeViewer::Middleware) - app.middleware.insert_before(0, Viewer::FirefoxViewer::Middleware) + if AppProfiler.speedscope_viewer == Viewer::SpeedscopeRemoteViewer + app.middleware.insert_before(0, Viewer::SpeedscopeRemoteViewer::Middleware) + end + app.middleware.insert_before(0, Viewer::FirefoxRemoteViewer::Middleware) end app.middleware.insert_before(0, AppProfiler.middleware) end From 9f8e8fa8a38511facf7257670bea0412a6e6acd0 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Fri, 9 Feb 2024 13:19:30 -0500 Subject: [PATCH 32/34] Remove dead code --- lib/app_profiler/yarn/command.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/app_profiler/yarn/command.rb b/lib/app_profiler/yarn/command.rb index 071ab78f..97fe17a7 100644 --- a/lib/app_profiler/yarn/command.rb +++ b/lib/app_profiler/yarn/command.rb @@ -5,8 +5,6 @@ module Yarn module Command class YarnError < StandardError; end - GECKO_VIEWER_PACKAGE = AppProfiler.gecko_viewer_package - VALID_COMMANDS = [ ["which", "yarn"], ["yarn", "init", "--yes"], @@ -18,7 +16,6 @@ class YarnError < StandardError; end ] private_constant(:VALID_COMMANDS) - private_constant(:GECKO_VIEWER_PACKAGE) def yarn(command, *options) setup_yarn unless yarn_setup From 832402e239d413e658ebac6c710c571a8f880070 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Tue, 30 Jan 2024 16:01:35 -0500 Subject: [PATCH 33/34] WIP precompile task for firefox --- Rakefile | 2 + lib/app_profiler/yarn/command.rb | 1 + .../yarn/with_firefox_profiler.rb | 69 +++++++++++++++++-- lib/tasks/firefox_profiler_compile.rake | 57 +++++++++++++++ 4 files changed, 125 insertions(+), 4 deletions(-) create mode 100644 lib/tasks/firefox_profiler_compile.rake diff --git a/Rakefile b/Rakefile index 8decebf5..5168d32c 100644 --- a/Rakefile +++ b/Rakefile @@ -3,6 +3,8 @@ require "bundler/gem_tasks" require "rake/testtask" +load "lib/tasks/firefox_profiler_compile.rake" + Rake::TestTask.new(:test) do |t| t.libs << "test" t.libs << "lib" diff --git a/lib/app_profiler/yarn/command.rb b/lib/app_profiler/yarn/command.rb index 97fe17a7..a1800302 100644 --- a/lib/app_profiler/yarn/command.rb +++ b/lib/app_profiler/yarn/command.rb @@ -7,6 +7,7 @@ class YarnError < StandardError; end VALID_COMMANDS = [ ["which", "yarn"], + ["which", "gcloud"], ["yarn", "init", "--yes"], ["yarn", "add", "speedscope", "--dev", "--ignore-workspace-root-check"], ["yarn", "run", "speedscope", /.*\.json/], diff --git a/lib/app_profiler/yarn/with_firefox_profiler.rb b/lib/app_profiler/yarn/with_firefox_profiler.rb index 38fa6e1a..f0d5d8ce 100644 --- a/lib/app_profiler/yarn/with_firefox_profiler.rb +++ b/lib/app_profiler/yarn/with_firefox_profiler.rb @@ -1,9 +1,16 @@ # frozen_string_literal: true +require "rubygems/package" +require "zlib" +require "open-uri" + module AppProfiler module Yarn module WithFirefoxProfile include Command + class CommandError < StandardError; end + + INSTALL_DIRECTORY = "./tmp" def setup_yarn super @@ -18,12 +25,68 @@ def firefox_profiler_added? AppProfiler.root.join("node_modules/firefox-profiler/dist").exist? end + def github_source? + AppProfiler.gecko_viewer_package.start_with?("https://github.com") + end + + def compiled_source? + AppProfiler.gecko_viewer_package.start_with?("https://") && + AppProfiler.gecko_viewer_package.end_with?("_compiled.tar.gz") + end + + def append_auth_header(opts) + if AppProfiler.gecko_viewer_package.start_with?("https://storage.googleapis.com/") + exec("which", "gcloud", silent: true) do + raise( + CommandError, + <<~MSG.squish + `gcloud` command not found, but gcloud auth required. + Please install `gcloud` or make it available in PATH. + MSG + ) + end + + opts["Authorization"] = "Bearer " + %x(gcloud auth print-access-token) + end + end + def fetch_firefox_profiler - raise ArgumentError unless AppProfiler.gecko_viewer_package.start_with?("https://github.com") + dir = INSTALL_DIRECTORY + + if github_source? + fetch_from_github(dir) + elsif compiled_source? + fetch_pre_compiled("#{dir}/firefox-profiler") + else + raise ArgumentError, "#{AppProfiler.gecko_viewer_package} is not a valid source for firefox profiler" + end + yarn("add", "--dev", "#{dir}/firefox-profiler") + end + + def fetch_pre_compiled(dir) + opts = {} + append_auth_header(opts) + tar_gz_file = URI.parse(AppProfiler.gecko_viewer_package).open(opts) + + Gem::Package::TarReader.new(Zlib::GzipReader.open(tar_gz_file)) do |tar| + tar.each do |entry| + next if entry.directory? + + target_file = File.join(dir, entry.full_name) + + FileUtils.mkdir_p(File.dirname(target_file)) + + File.open(target_file, "wb") do |f| + f.write(entry.read) + end + end + end + end + + def fetch_from_github(dir) repo, branch = AppProfiler.gecko_viewer_package.to_s.split("#") - dir = "./tmp" FileUtils.mkdir_p(dir) Dir.chdir(dir) do clone_args = ["git", "clone", repo, "firefox-profiler"] @@ -41,8 +104,6 @@ def fetch_firefox_profiler yarn("--cwd", "#{dir}/firefox-profiler", "build-prod") patch_file("#{dir}/firefox-profiler/dist/index.html", 'href="locales/en-US/app.ftl"', 'href="/app_profiler/firefox/viewer/locales/en-US/app.ftl"') - - yarn("add", "--dev", "#{dir}/firefox-profiler") end def patch_firefox_profiler(dir) diff --git a/lib/tasks/firefox_profiler_compile.rake b/lib/tasks/firefox_profiler_compile.rake new file mode 100644 index 00000000..05265d34 --- /dev/null +++ b/lib/tasks/firefox_profiler_compile.rake @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require "rubygems/package" +require "zlib" +require "fileutils" + +require "app_profiler" +require "app_profiler/yarn/command" +require "app_profiler/yarn/with_firefox_profiler" + +# The bundle is already compiled, so we can ignore most of the source contents +PACKAGE_INCLUDE = [ + %r{^\.circleci/config\.yml$}, + %r{^bin/pre-install\.js$}, + /^package\.json$/, + /^dist/, +].freeze + +class CompileShim + include AppProfiler::Yarn::WithFirefoxProfile +end + +namespace :firefox_profiler do + desc "Compile firefox profiler" + task :compile do + AppProfiler.root = Pathname.getwd + CompileShim.new.setup_yarn + end + desc "Package firefox profiler" + task package: :compile do + package_directory("node_modules/firefox-profiler", "out.tar.gz") + end +end + +def package_directory(source_dir, output) + File.open(output, "wb") do |tar_gz_file| + Zlib::GzipWriter.wrap(tar_gz_file) do |gzip_file| + Dir.chdir(source_dir) do + Gem::Package::TarWriter.new(gzip_file) do |tar| + Dir["**/*", ".*/**/*"].each do |file| + next unless PACKAGE_INCLUDE.any? { |pattern| file =~ pattern } + + mode = File.stat(file).mode + + if File.directory?(file) + tar.mkdir(file, mode) + else + tar.add_file_simple(file, mode, File.size(file)) do |tar_file| + IO.copy_stream(File.open(file, "rb"), tar_file) + end + end + end + end + end + end + end +end From 8b8dd4b581eeac16fed161ab1c35a8671e811e44 Mon Sep 17 00:00:00 2001 From: Dale Hamel Date: Fri, 16 Feb 2024 15:42:56 -0500 Subject: [PATCH 34/34] Add helper to prune package.json contents --- lib/tasks/firefox_profiler_compile.rake | 38 ++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/lib/tasks/firefox_profiler_compile.rake b/lib/tasks/firefox_profiler_compile.rake index 05265d34..6a9b20fb 100644 --- a/lib/tasks/firefox_profiler_compile.rake +++ b/lib/tasks/firefox_profiler_compile.rake @@ -10,12 +10,17 @@ require "app_profiler/yarn/with_firefox_profiler" # The bundle is already compiled, so we can ignore most of the source contents PACKAGE_INCLUDE = [ - %r{^\.circleci/config\.yml$}, - %r{^bin/pre-install\.js$}, /^package\.json$/, /^dist/, ].freeze +# Hack to make the package.json more portable by removing some constraints +# contains arrays of diggable hash keys +DELETE_KEYS = [ + ["engines"], + ["scripts", "preinstall"], +] + class CompileShim include AppProfiler::Yarn::WithFirefoxProfile end @@ -45,9 +50,12 @@ def package_directory(source_dir, output) if File.directory?(file) tar.mkdir(file, mode) else - tar.add_file_simple(file, mode, File.size(file)) do |tar_file| - IO.copy_stream(File.open(file, "rb"), tar_file) + fp = File.open(file, "rb") + fp = prune_keys(fp) if file == "package.json" + tar.add_file_simple(file, mode, fp.size) do |tar_file| + IO.copy_stream(fp, tar_file) end + fp.close end end end @@ -55,3 +63,25 @@ def package_directory(source_dir, output) end end end + +def prune_keys(orig_fp) + package_contents = JSON.parse(orig_fp.read) + orig_fp.close + DELETE_KEYS.each do |keys| + next unless package_contents.dig(*keys) + + to_delete = keys.pop + if keys.empty? + package_contents.delete(to_delete) + else + nested_hash = package_contents.dig(*keys) + nested_hash.delete(to_delete) + end + end + tmp = Tempfile.new("relaxed_package.json") + tmp.unlink + tmp.write(JSON.dump(package_contents)) + tmp.flush + tmp.rewind + tmp +end