profile-id improvements #159

bmansoob · 2025-02-06T16:48:25Z

Replaces CurrentAttributes with local thread storage which gets reset by the middleware at the end. The reason for this is if the profiling middleware runs before this, then the value would be inconsistent if it is read again on the way out (sending response back). I have tried to maintain the same API interface so that it does not break existing clients
We now persist profile_id in metadata, so that the clients do not have to do this.

bmansoob · 2025-02-06T17:05:55Z

once approved -- i would like to try this branch first in production before cutting a new release

bmansoob · 2025-02-06T17:07:22Z

lib/app_profiler/base_profile.rb

    # a vernier "result" object for vernier
    def initialize(data, id: nil, context: nil)
-      @id      = id.presence || ProfileId.current
+      ProfileId.set(id) if id.present?


We were not doing this before. If someone provides an Id we should set ProfileId too so that the values are consistent.

manuelfelipe

We now persist profile_id in metadata, so that the clients do not have to do this.

nice. Thanks for that!

manuelfelipe · 2025-02-06T17:31:48Z

We now persist profile_id in metadata, so that the clients do not have to do this.

Now that looked at the corresponding prod change, maybe we should also add the profiler key to the metadata from here similarly to how you are doing for profile_id instead of having all apps setting that value

lib/app_profiler/profile_id.rb

gmcgibbon · 2025-02-06T21:12:53Z

lib/app_profiler/base_profile.rb

    def initialize(data, id: nil, context: nil)
-      @id      = id.presence || ProfileId.current
+      ProfileId.set(id) if id.present?
+      @id      = ProfileId.current


IMO You don't need to track this as an ivar anymore. Any call should use the current value. You can only have 1 profile per request, right?

this is a good point 👍

changed this to:

def id metadata[PROFILE_ID_METADATA_KEY] end

gmcgibbon · 2025-02-06T21:14:08Z

lib/app_profiler.rb


 module AppProfiler
+  PROFILE_ID_METADATA_KEY = :profile_id
+  PROFILE_BACKEND_METADATA_KEY = :profiler


I would keep these in BaseProfile. Putting them in two places doesn't really make sense if they're solely used there.

Clients can use these ids when dealing with profile metadata. I think accessing it as AppProfiler:: PROFILE_ID_METADATA_KEY is better than nesting it under BaseProfile. I have removed it from BaseProfile.

Ok, as long as it isn't repeated I'm happy. 👍

gmcgibbon · 2025-02-06T21:16:30Z

lib/app_profiler/base_profile.rb

    private_constant :INTERNAL_METADATA_KEYS
    class UnsafeFilename < StandardError; end
+    PROFILE_ID_METADATA_KEY = :profile_id
+    PROFILE_BACKEND_METADATA_KEY = :profiler


Nit: I'd consider adding these to private_const above, but not strictly necessary if you need them in tests. You could just use the plain value there instead.

bmansoob · 2025-02-07T15:33:37Z

@gmcgibbon how bad would it be if I change this to

app_profiler/lib/app_profiler/base_profile.rb

Line 100 in dadf2e5

AppProfiler.profile_file_name.call(metadata) + format

to take the profile object, instead of metadata?

This is quite recent, like a week or so ago -- so not sure if there are usages out in the wild 😬

Will allow us to just use the profile object and its methods (id is now deeply integrated). I kind of dread i did not do this to start with.

gmcgibbon

LGTM after these changes

lib/app_profiler/profile_id.rb

lib/app_profiler/stackprof_profile.rb

lib/app_profiler/vernier_profile.rb

gmcgibbon · 2025-02-07T18:38:06Z

lib/app_profiler/vernier_profile.rb

+
+    class << self
+      def backend_name
+        NAME # cannot reference Backend::VernierBackend because of different ruby versions we have to support


Suggested change

NAME # cannot reference Backend::VernierBackend because of different ruby versions we have to support

BACKEND_NAME

Are you able to make Backend::VernierBackend.name and other classes reference this constant so we're not repeating ourselves? We need to eventually add loading tests to make sure we don't regress #155.

done - think I did all of them

> Rg "AppProfiler::Backend::VernierBackend.name" . # returns nothing

> Rg "AppProfiler::VernierProfile::BACKEND_NAME" . ./test/app_profiler/run_test.rb 35: AppProfiler.run(backend: AppProfiler::VernierProfile::BACKEND_NAME) do 36: assert_equal(AppProfiler::VernierProfile::BACKEND_NAME, AppProfiler.backend) ./lib/app_profiler.rb 167: backend_name&.to_sym == AppProfiler::VernierProfile::BACKEND_NAME 181: RUBY_VERSION >= "3.2.1" && defined?(AppProfiler::VernierProfile::BACKEND_NAME) ./lib/app_profiler/request_parameters.rb 36: if AppProfiler.vernier_supported? && backend == AppProfiler::VernierProfile::BACKEND_NAME && ./test/app_profiler/backend_test.rb 12: assert_raises(BackendError) { AppProfiler.backend = AppProfiler::VernierProfile::BACKEND_NAME } 24: assert(AppProfiler.backend = AppProfiler::VernierProfile::BACKEND_NAME) 25: assert_equal(AppProfiler.backend, AppProfiler::VernierProfile::BACKEND_NAME) 52: AppProfiler.backend_for(AppProfiler::VernierProfile::BACKEND_NAME),

test/app_profiler/middleware_test.rb

gmcgibbon · 2025-02-07T18:40:46Z

test/app_profiler/middleware_test.rb

+          middleware.call(mock_request_env(path: "/?profile=cpu"))
+        end
+      end
+      assert_nil(Thread.current[ProfileId::PROFILE_ID_KEY])


Suggested change

assert_nil(Thread.current[ProfileId::PROFILE_ID_KEY])

assert_nil(ProfileId.current)

If you want to test threads, you can do so in a ProfileId test. Now that we aren't using current attributes we should test this more thoroughly anyway.

I will think of adding more meaningful threads in ProfileId tests. But. given what we found for current attributes regarding order of middlewars, I specifically wanted to test this after middleware has run and finished. I ack this feels a bit out of of place - but is kind of of imp. Alternatively, I can expose a method nil? which is used only for tests and is without any side-effects.

added some basics tests for threads 1694438#diff-ad5a82381a58be0174af1d3fd8d65974e9aa806549a942ce058b482130edc42fR14

gmcgibbon · 2025-02-07T18:41:11Z

lib/app_profiler.rb


 module AppProfiler
+  PROFILE_ID_METADATA_KEY = :profile_id
+  PROFILE_BACKEND_METADATA_KEY = :profiler


Ok, as long as it isn't repeated I'm happy. 👍

gmcgibbon · 2025-02-07T18:50:22Z

@bmansoob You can make that change, but because it is a released feature you need to think about deprecation cycling the old behaviour. I would wrap a proc on assignment that checks if the type of the input. What you could do is:

Make a separate PR with the change to behaviour + docs + changelog
Make BaseProfile delegate [] to metadata so legacy procs would still work
Depending on how you feel about having the subscripting operator work that way, raise a deprecation warning, or keep the behaviour

bmansoob · 2025-02-07T18:52:06Z

@bmansoob You can make that change, but because it is a released feature you need to think about deprecation cycling the old behaviour. I would wrap a proc on assignment that checks if the type of the input. What you could do is:

thanks -- something to ponder and do in a separate PR 😅

bmansoob · 2025-02-07T19:25:21Z

I will squash the commits in a bit and test this branch in production before cutting a new release.

bmansoob · 2025-02-10T13:50:50Z

squashed

bmansoob · 2025-02-10T17:29:11Z

direction change - going to merge and release the gem.

bmansoob force-pushed the profile-id-improvements branch from 4fb0cf6 to 0392954 Compare February 6, 2025 16:58

bmansoob requested review from dalehamel, gmcgibbon and manuelfelipe February 6, 2025 17:06

bmansoob commented Feb 6, 2025

View reviewed changes

manuelfelipe approved these changes Feb 6, 2025

View reviewed changes

bmansoob force-pushed the profile-id-improvements branch from 96e054d to 785570c Compare February 6, 2025 17:53

bmansoob mentioned this pull request Feb 6, 2025

make sorbet / tapioca happy #158

Closed

bmansoob force-pushed the profile-id-improvements branch from 785570c to c8595fb Compare February 6, 2025 20:23

gmcgibbon reviewed Feb 6, 2025

View reviewed changes

gmcgibbon approved these changes Feb 7, 2025

View reviewed changes

profile-id improvements

2a49a66

bmansoob force-pushed the profile-id-improvements branch from 1694438 to 2a49a66 Compare February 10, 2025 13:50

bmansoob merged commit bf75b42 into main Feb 10, 2025
7 checks passed

bmansoob deleted the profile-id-improvements branch February 10, 2025 17:29

bmansoob mentioned this pull request Feb 10, 2025

release 0.2.9 #160

Merged

shopify-shipit bot temporarily deployed to production February 10, 2025 17:37 Inactive

	NAME # cannot reference Backend::VernierBackend because of different ruby versions we have to support
	BACKEND_NAME

	assert_nil(Thread.current[ProfileId::PROFILE_ID_KEY])
	assert_nil(ProfileId.current)

profile-id improvements #159

profile-id improvements #159

Uh oh!

Conversation

bmansoob commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmansoob commented Feb 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manuelfelipe left a comment

Choose a reason for hiding this comment

Uh oh!

manuelfelipe commented Feb 6, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmansoob commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmcgibbon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmcgibbon commented Feb 7, 2025

Uh oh!

bmansoob commented Feb 7, 2025

Uh oh!

bmansoob commented Feb 7, 2025

Uh oh!

bmansoob commented Feb 10, 2025

Uh oh!

bmansoob commented Feb 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bmansoob commented Feb 6, 2025 •

edited

Loading

bmansoob commented Feb 7, 2025 •

edited

Loading