log link to speedscope during gcs upload #47

kimbilida · 2022-05-16T19:36:54Z

Why?

Make it easier to link to the flamegraph in speedscope. When trying to get to the profile I needed to read through the docs and then eventually disturb @dalehamel to get the right link.

How?

Always output the link to AppProfiler.speedscope_host instead of just the gcs location to make it easier to get to the flame graph and to avoid having to read through the docs
pulled the link logic that was in the middleware/upload_action into the app_profiler and accessed the method from profile

Next: can remove the logging from the background queue in core

For reviewers:

other suggested approaches?

dalehamel

I think that this functionality already will basically depend on the url formatter being set to provide useful output, as the default case generates (and apparently, always has) a nonsense URL. For this reason, I'm suggesting simplifying the PR a bit and just checking if the formatter is defined, and if so, call it, leaving the current profile_url method where it is.

Overall this does look like it will achieve what it sets out to, and make it easier to get the profile URL by relying on the custom formatter being set by the app.

I'd like to defer to @gmcgibbon though as he's a better rubyist than myself and is familiar with this gem. Before rewriting anything, let's hear what he has to say.

dalehamel · 2022-05-17T18:33:22Z

lib/app_profiler.rb

+
+    def profile_url(upload)
+      if AppProfiler.profile_url_formatter.nil?
+        "#{AppProfiler.speedscope_host}#profileURL=#{upload.url}"


As I mentioned on slack, I think this branch path can be misleading, it is likely it will generate URLs that do not work (but this code was already here since the initial import and I guess that was just missed). We more or less require that the profile formatter be given to generate the correct URL, as I am pretty sure this default one will fail in almost every situation.

I think I'd opt for leaving this code where it is, tucked in the middleware module, and then just directly call the formatter if it is defined, since I'd argue the default case here has essentially no value, perhaps even negative value as it will generate wrong URLs.

This is for the default case of using a local speedscope instance and filesystem storage. I would prefer if we just made this the default profile_url_formatter proc eg. mattr_accessor :profile_url_formatter, default: proc { |upload| "#{AppProfiler.speedscope_host}#profileURL=#{upload.url}" }. This way there are no conditionals.

@gmcgibbon ah! that makes sense, i hadn't considered that it would work with a local file and speedscope instance! Thanks for clarifying 👍

dalehamel · 2022-05-17T18:51:20Z

lib/app_profiler/profile.rb

@@ -41,6 +41,7 @@ def upload
      AppProfiler.storage.upload(self).tap do |upload|
        if upload && defined?(upload.url)
          AppProfiler.logger.info("[Profiler] uploaded profile profile_url=#{upload.url}")
+          AppProfiler.logger.info("[Profiler] Profile available at profile_url=#{AppProfiler.profile_url(upload)}")


I wonder if a better approach would be to do the following:

unless AppProfiler.profile_url_formatter.nil? AppProfiler.logger.info("[Profiler] Profile available at profile_url=#{AppProfiler.profile_url_formatter.call(upload)}") end

Then add a new test like assert_custom_url_printed_on_upload that sets the url formatter and checks the output as you have in your existing test.

I think we're talking about 2 different things in this log message. The profile_url (the location of the profile on the storage service) and the speedscope_url (the address to actually view the profile). I think this would make more sense:

AppProfiler.logger.info("[Profiler] uploaded profile profile_url=#{upload.url} speedscope_url=#{AppProfiler.profile_url_formatter.call(upload)}")

It would be nice if we could define #speedscope_url on the upload object, but this would require a larger refactor of the storage services.

kimbilida · 2022-05-24T14:59:25Z

lib/app_profiler/profile.rb

+          AppProfiler.logger.info(
+            "[Profiler] data uploaded: " \
+              "profile_url=#{upload.url} " \
+              "profile_viewer_url=#{AppProfiler.profile_url_formatter.call(upload)}"
+          )


I updated the wording of the log here. Please review and I'll need to update the docs as well.

Suggested change

AppProfiler.logger.info(

"[Profiler] data uploaded: " \

"profile_url=#{upload.url} " \

"profile_viewer_url=#{AppProfiler.profile_url_formatter.call(upload)}"

)

AppProfiler.logger.info(

<<~INFO.squish

[Profiler] data uploaded:

profile_url=#{upload.url}

profile_viewer_url=#{AppProfiler.profile_viewer_url(upload)}

INFO

)

Nit: We could use a heredoc for this multiline string instead

gmcgibbon · 2022-05-24T22:12:33Z

lib/app_profiler.rb

@@ -72,5 +75,9 @@ def profile_data_header
    def profile_url_formatter=(block)
      @@profile_url_formatter = block # rubocop:disable Style/ClassVars
    end
+
+    def profile_url(upload)


Suggested change

def profile_url(upload)

def profile_viewer_url(upload)

If we're going to keep this method, we should name it to distinguish the difference between a profile url and a profile viewer url and use it in lib/app_profiler/profile.rb too. profile_url_formatter should also be renamed to profile_viewer_url_formatter, but we would need to do a deprecation cycle for that (so maybe we could consider doing this in another change).

gmcgibbon · 2022-05-24T22:37:22Z

lib/app_profiler/profile.rb

+          AppProfiler.logger.info(
+            "[Profiler] data uploaded: " \
+              "profile_url=#{upload.url} " \
+              "profile_viewer_url=#{AppProfiler.profile_url_formatter.call(upload)}"
+          )


Suggested change

AppProfiler.logger.info(

"[Profiler] data uploaded: " \

"profile_url=#{upload.url} " \

"profile_viewer_url=#{AppProfiler.profile_url_formatter.call(upload)}"

)

AppProfiler.logger.info(

<<~INFO.squish

[Profiler] data uploaded:

profile_url=#{upload.url}

profile_viewer_url=#{AppProfiler.profile_viewer_url(upload)}

INFO

)

Nit: We could use a heredoc for this multiline string instead

gmcgibbon · 2022-05-24T22:38:10Z

lib/app_profiler.rb

@@ -35,7 +35,10 @@ module Viewer
  mattr_accessor :autoredirect, default: false
  mattr_reader   :profile_header, default: "X-Profile"
  mattr_accessor :context, default: nil
-  mattr_reader   :profile_url_formatter, default: nil
+  mattr_reader   :profile_url_formatter,


Suggested change

mattr_reader :profile_url_formatter,

mattr_reader :profile_url_formatter, default: proc do |upload|

"#{AppProfiler.speedscope_host}#profileURL=#{upload.url}"

end

@gmcgibbon when I use do instead of braces I'm getting an error

`proc': tried to create Proc object without a block (ArgumentError)

Ah, yeah sorry. Let's assign the proc to a constant and then assign the constant to the default.

gmcgibbon · 2022-05-24T22:38:31Z

CHANGELOG.md

@@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]

 [Unreleased]: https://github.com/Shopify/app_profiler
+- Log info with link to speedscope during file upload (#47)


Suggested change

- Log info with link to speedscope during file upload (#47)

- Log info with link to speedscope during file upload (#47)

gmcgibbon

LGTM after the proc fix. Please squash your commits before merging. Thanks!

PR review pr review make proc a const

This reverts commit ca2b44c, reversing changes made to 638d663.

kimbilida requested a review from dalehamel May 16, 2022 19:37

kimbilida force-pushed the kimb-log-speedscope-link branch from 5711b5c to 87bd1ff Compare May 17, 2022 13:34

dalehamel reviewed May 17, 2022

View reviewed changes

kimbilida requested review from dalehamel and gmcgibbon May 24, 2022 14:57

kimbilida commented May 24, 2022

View reviewed changes

gmcgibbon reviewed May 24, 2022

View reviewed changes

kimbilida force-pushed the kimb-log-speedscope-link branch from 08b3ade to 7ca3f59 Compare May 25, 2022 13:19

gmcgibbon approved these changes May 25, 2022

View reviewed changes

log link to speedscope during gcs upload

9dbfd8a

PR review pr review make proc a const

kimbilida force-pushed the kimb-log-speedscope-link branch from a21dc9e to 9dbfd8a Compare May 25, 2022 18:44

kimbilida merged commit ca2b44c into main May 25, 2022

kimbilida deleted the kimb-log-speedscope-link branch May 25, 2022 18:46

shopify-shipit bot temporarily deployed to production June 16, 2022 13:21 Inactive

dalehamel added a commit that referenced this pull request Jun 24, 2022

Revert "Merge pull request #47 from Shopify/kimb-log-speedscope-link"

3276d32

This reverts commit ca2b44c, reversing changes made to 638d663.

dalehamel mentioned this pull request Jun 27, 2022

[bug] - Profile formatter NPE #52

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

log link to speedscope during gcs upload #47

log link to speedscope during gcs upload #47

kimbilida commented May 16, 2022

dalehamel left a comment

dalehamel May 17, 2022

gmcgibbon May 19, 2022 •

edited

Loading

dalehamel May 19, 2022 •

edited

Loading

dalehamel May 17, 2022

gmcgibbon May 19, 2022

kimbilida May 24, 2022

gmcgibbon May 24, 2022

gmcgibbon May 24, 2022

gmcgibbon May 24, 2022

gmcgibbon May 24, 2022

kimbilida May 25, 2022

gmcgibbon May 25, 2022

gmcgibbon May 24, 2022

gmcgibbon left a comment

	- Log info with link to speedscope during file upload (#47)

	- Log info with link to speedscope during file upload (#47)

log link to speedscope during gcs upload #47

log link to speedscope during gcs upload #47

Conversation

kimbilida commented May 16, 2022

dalehamel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcgibbon May 19, 2022 • edited Loading

Choose a reason for hiding this comment

dalehamel May 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcgibbon left a comment

Choose a reason for hiding this comment

gmcgibbon May 19, 2022 •

edited

Loading

dalehamel May 19, 2022 •

edited

Loading