diff --git a/Gemfile b/Gemfile index 9a2e3a5..65ee825 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", "~> 1.3.1") if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new("3.2.1") +gem("vernier", "~> 1.7.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 6e20b53..76433d8 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -212,7 +212,7 @@ GEM thread_safe (~> 0.1) uber (0.1.0) unicode-display_width (2.5.0) - vernier (1.3.1) + vernier (1.7.0) webrick (1.7.0) PLATFORMS @@ -233,7 +233,7 @@ DEPENDENCIES rubocop rubocop-performance rubocop-shopify - vernier (~> 1.3.1) + vernier (~> 1.7.0) BUNDLED WITH 2.5.9 diff --git a/lib/app_profiler/backend/vernier_backend.rb b/lib/app_profiler/backend/vernier_backend.rb index 2dd50e8..3cc1956 100644 --- a/lib/app_profiler/backend/vernier_backend.rb +++ b/lib/app_profiler/backend/vernier_backend.rb @@ -15,6 +15,10 @@ class VernierBackend < BaseBackend :retained, ].freeze + PRIVATE_METADATA = [ + :started_at, # started_at uses a monotonic source, not realtime + ].freeze + class << self def name :vernier @@ -43,7 +47,9 @@ def start(params = {}) @mode = params.delete(:mode) || DEFAULTS[:mode] raise ArgumentError unless AVAILABLE_MODES.include?(@mode) - @metadata = params.delete(:metadata) + if Gem.loaded_specs["vernier"].version < Gem::Version.new("1.7.0") + @metadata = params.delete(:metadata) + end clear @collector ||= ::Vernier::Collector.new(@mode, **params) @@ -74,11 +80,19 @@ def results return unless vernier_profile + # Store all vernier metadata + meta = vernier_profile.meta.reject { |k, v| k == :user_metadata || v.nil? || PRIVATE_METADATA.include?(k) } + meta.merge!(@metadata) if @metadata + + # Internal metadata takes precedence over user metadata, but store + # everything in user metadata + vernier_profile.meta[:user_metadata]&.merge!(meta) + # 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 + data[:meta][:mode] = @mode + data[:meta][:vernierUserMetadata] ||= meta # for compatibility with < 1.7.0 @mode = nil @metadata = nil diff --git a/lib/app_profiler/base_profile.rb b/lib/app_profiler/base_profile.rb index c4515a8..a1f85c5 100644 --- a/lib/app_profiler/base_profile.rb +++ b/lib/app_profiler/base_profile.rb @@ -23,7 +23,7 @@ def from_stackprof(data) end def from_vernier(data) - options = INTERNAL_METADATA_KEYS.map { |key| [key, data[:meta]&.delete(key)] }.to_h + options = INTERNAL_METADATA_KEYS.map { |key| [key, data[:meta]&.[](:vernierUserMetadata)&.delete(key)] }.to_h VernierProfile.new(data, **options).tap do |profile| raise ArgumentError, "invalid profile data" unless profile.valid? diff --git a/lib/app_profiler/vernier_profile.rb b/lib/app_profiler/vernier_profile.rb index 0f26276..fc444e5 100644 --- a/lib/app_profiler/vernier_profile.rb +++ b/lib/app_profiler/vernier_profile.rb @@ -14,6 +14,7 @@ def backend_name def initialize(data, id: nil, context: nil) data[:meta] ||= {} + data[:meta][:vernierUserMetadata] ||= {} super(data, id: id, context: context) end @@ -22,7 +23,7 @@ def mode end def metadata - @data[:meta] + @data[:meta][:vernierUserMetadata] end def format diff --git a/test/app_profiler/backend/vernier_backend_test.rb b/test/app_profiler/backend/vernier_backend_test.rb index 0beb136..cfd6fae 100644 --- a/test/app_profiler/backend/vernier_backend_test.rb +++ b/test/app_profiler/backend/vernier_backend_test.rb @@ -77,7 +77,7 @@ def teardown assert_instance_of(AppProfiler::VernierProfile, profile) assert_equal("wowza", profile.id) assert_equal("bar", profile.context) - assert_equal("spam", profile[:meta][:extrameta]) + assert_equal("spam", profile.metadata[:extrameta]) end test ".run wall profile" do @@ -200,6 +200,50 @@ def teardown assert_nil(AppProfiler.profiler.results) end + + test ".results contain vernierUserMetadata, and extra meta" do + skip "metadata output added in >=1.7.0" if Gem.loaded_specs["vernier"].version < Gem::Version.new("1.7.0") + + profile = AppProfiler.profiler.run( + vernier_params( + interval: 2000, + metadata: { + user_data_1: "foo", + user_data_2: "bar", + interval: 1000, + }, + ), + ) do + sleep(0.1) + end + + assert_instance_of(AppProfiler::VernierProfile, profile) + + # Stores "internal" vernier metadata + assert_equal(:wall, profile.metadata[:mode]) + assert_equal(2000, profile.metadata[:interval]) # The internal value takes precedence over the user value + assert_equal(0, profile.metadata[:allocation_interval]) + assert_equal(false, profile.metadata[:gc]) + + # Don't include ignored/private metadata + AppProfiler::Backend::VernierBackend::PRIVATE_METADATA.each do |excluded| + refute_includes(profile.metadata, excluded) + end + + # Stores the user supplied data + assert_equal("foo", profile.metadata[:user_data_1]) + assert_equal("foo", profile.metadata[:user_data_1]) + + # Check that "extra" meta (for UI display) is also present + { + user_data_1: "foo", + user_data_2: "bar", + interval: 2000, + mode: :wall, + }.each do |k, v| + assert(profile[:meta][:extra].first[:entries].find { |e| e[:label] == k && e[:value] == v }) + end + end end end end diff --git a/test/app_profiler/profile/vernier_profile_test.rb b/test/app_profiler/profile/vernier_profile_test.rb index 0efbbc4..6734b2e 100644 --- a/test/app_profiler/profile/vernier_profile_test.rb +++ b/test/app_profiler/profile/vernier_profile_test.rb @@ -5,7 +5,7 @@ module AppProfiler class VernierProfileTest < TestCase test ".from_vernier assigns id and context metadata" do - profile = BaseProfile.from_vernier(vernier_profile(meta: { id: "foo", context: "bar" })) + profile = BaseProfile.from_vernier(vernier_profile(meta: { vernierUserMetadata: { id: "foo", context: "bar" } })) assert_equal("foo", profile.id) assert_equal("foo", ProfileId.current) @@ -97,14 +97,19 @@ class VernierProfileTest < TestCase end test "#[] forwards to profile metadata" do - profile = VernierProfile.new(vernier_profile(meta: { interval: 10_000 })) + profile = VernierProfile.new(vernier_profile(meta: { vernierUserMetadata: { interval: 10_000 } })) assert_equal(10_000, profile.metadata[:interval]) end test "#path raises an UnsafeFilename exception given chars not in allow list" do assert_raises(AppProfiler::BaseProfile::UnsafeFilename) do - profile = BaseProfile.from_vernier(vernier_profile(meta: { id: "|`@${}", context: "bar" })) + profile = BaseProfile.from_vernier(vernier_profile(meta: { + vernierUserMetadata: { + id: "|`@${}", + context: "bar", + }, + })) profile.file end end @@ -126,7 +131,7 @@ class VernierProfileTest < TestCase test "#file uses custom profile_file_name block when provided" do old_profile_file_name = AppProfiler.profile_file_name AppProfiler.profile_file_name = ->(metadata) { "file-name-#{metadata[:id]}" } - profile = VernierProfile.new(vernier_profile(meta: { id: "foo", context: "bar" })) + profile = VernierProfile.new(vernier_profile(meta: { vernierUserMetadata: { id: "foo", context: "bar" } })) assert_match("file-name-foo.vernier.json", File.basename(profile.file.to_s)) ensure AppProfiler.profile_file_name = old_profile_file_name