Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump this for local development of vernier, so that all tests will run.


gem("google-cloud-storage", "~> 1.21")
gem("rubocop", require: false)
Expand Down
4 changes: 2 additions & 2 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -233,7 +233,7 @@ DEPENDENCIES
rubocop
rubocop-performance
rubocop-shopify
vernier (~> 1.3.1)
vernier (~> 1.7.0)

BUNDLED WITH
2.5.9
20 changes: 17 additions & 3 deletions lib/app_profiler/backend/vernier_backend.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 1.7.0 or greater, we can just pass it directly as as it was added in https://github.com/jhawthorn/vernier/pull/145/files

@metadata = params.delete(:metadata)
end
clear

@collector ||= ::Vernier::Collector.new(@mode, **params)
Expand Down Expand Up @@ -74,11 +80,19 @@ def results

return unless vernier_profile

# Store all vernier metadata
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vernier stores some "internal" metadata that is actually interesting to us and we want to have in the output. We'll copy these to "user 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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The merge order here matters, so if a user supplied a key that is otherwise internal, the internal one takes precedence since we are jamming both into the same "namespace".


# 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
Copy link
Member Author

@dalehamel dalehamel Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for metadata, we now read directly from this output key. To be compatible with older versions of vernier, we'll store the metadata that was added by the user here if it doesn't exist.

@mode = nil
@metadata = nil

Expand Down
2 changes: 1 addition & 1 deletion lib/app_profiler/base_profile.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down
3 changes: 2 additions & 1 deletion lib/app_profiler/vernier_profile.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -22,7 +23,7 @@ def mode
end

def metadata
@data[:meta]
@data[:meta][:vernierUserMetadata]
end

def format
Expand Down
46 changes: 45 additions & 1 deletion test/app_profiler/backend/vernier_backend_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already a bit leaky, we should have been accessing it through the metadata helper method.

end

test ".run wall profile" do
Expand Down Expand Up @@ -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
13 changes: 9 additions & 4 deletions test/app_profiler/profile/vernier_profile_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 } }))
Copy link
Member Author

@dalehamel dalehamel Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are creating a "Mock" profile, these calls to vernier_profile take the vernierUserMedatata key to mimic the structure of a real profile


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
Expand All @@ -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
Expand Down