Merge pull request #165 from Shopify/support-vernier-metadata

Support vernier metadata

Author: dalehamel

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)

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

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
@@ -29,7 +33,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)
@@ -60,11 +66,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
 

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?

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

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

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