MiniProfiler / rack-mini-profiler

Profiler for your development and production Ruby rack apps.
MIT License
3.7k stars 402 forks source link

fetch_snapshots_overview not implemented for FileStore - support or warn if not supported #470

Open adambair opened 3 years ago

adambair commented 3 years ago

image

Clicking on 'snapshots' takes you to http://localhost:3000/mini-profiler-resources/snapshots which has this error:

Puma caught this error: fetch_snapshots is not implemented (NotImplementedError)
/gems/rack-mini-profiler-2.2.0/lib/mini_profiler/storage/abstract_store.rb:53:in `fetch_snapshots'
/gems/rack-mini-profiler-2.2.0/lib/mini_profiler/storage/abstract_store.rb:58:in `snapshot_groups_overview'
/gems/rack-mini-profiler-2.2.0/lib/mini_profiler/profiler.rb:754:in `handle_snapshots_request'
/gems/rack-mini-profiler-2.2.0/lib/mini_profiler/profiler.rb:182:in `serve_html'
/gems/rack-mini-profiler-2.2.0/lib/mini_profiler/profiler.rb:251:in `call'
/gems/newrelic_rpm-6.10.0.364/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/gems/railties-5.2.4.3/lib/rails/engine.rb:524:in `call'
/gems/newrelic_rpm-6.10.0.364/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/gems/rack-cors-1.1.1/lib/rack/cors.rb:100:in `call'
/gems/newrelic_rpm-6.10.0.364/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/gems/puma-4.3.5/lib/puma/configuration.rb:228:in `call'
/gems/puma-4.3.5/lib/puma/server.rb:713:in `handle_request'
/gems/puma-4.3.5/lib/puma/server.rb:472:in `process_client'
/gems/puma-4.3.5/lib/puma/server.rb:328:in `block in run'
/gems/puma-4.3.5/lib/puma/thread_pool.rb:134:in `block in spawn_thread'

I read through the docs and couldn't figure out why it wasn't working. I tried Memstore, Redis, Filestore... it wouldn't produce an error with the alternative stores but would instead say "no snapshots exist". I was pretty confused as I could see the snapshots on the filesystem and in Redis.

After poking around the repo and code for a bit I stumbled upon this comment https://github.com/MiniProfiler/rack-mini-profiler/pull/459#discussion_r477457529 which indicates that snapshots won't work for the user viewing the page if they have permissions to view the performance information...

I understand this for production situations but when testing locally it would be nice to be able to see previous snapshots in the UI. An alternative is to update the docs to call out this caveat to save folks some time with troubleshooting. Perhaps hide the 'snapshots' button if it's not available... or if there are no snapshots to show.

I understand this is a newer feature. I just didn't see anything about this situation so I figured I'd post something about it.

SamSaffron commented 3 years ago

Yeah, @OsamaSayegh can we hide the link if the user has no access to the feature?

OsamaSayegh commented 3 years ago

@SamSaffron if the user can see the speed badge, they should be able to access the snapshots page (snapshots page uses the same authorization logic as the speed badge). I think the problem here comes from the fact that requests made by admins/authorized users (i.e. users who get the speed badge) are excluded from snapshots sampling, but I think @adambair found this unexpected and was confused by it.

My reasoning behind this design decision was that admins already see profiling data via the speed badge on every request they make, so they can spot/notice slow stuff without needing their requests to be sampled. But I guess if people find this behavior confusing, I'm happy to change it. For now, I'll include requests from all users in sampling when in development env only so it becomes easier to debug/configure snapshots sampling.

@adambair which store were you using when you got the NotImplementedError error? And how did you fix it? MemoryStore and RedisStore implement this feature, so they shouldn't throw this error.

ivanyv commented 3 years ago

I also found this confusing, and it too took me a while to figure out the FileStore does not implement fetch_snapshots.

FWIW I wanted this so I could tag requests with custom fields and see those in the snapshots. As far as I understand, these fields won't show up in the speed badge (which would be helpful in order to differentiate GraphQL requests for example, which all go to the same URL).

adambair commented 3 years ago

@ivanyv Glad I'm not the only one, was worried there for a sec ;)

@OsamaSayegh I'm not sure as it was a while ago but I believe I was trying to follow the defaults in the docs so it was likely FileStore. Though I did try MemoryStore and RedisStore and didn't have much success (though it could have been for other reasons).

LagTag commented 3 years ago

@adambair @ivanyv

Yeah it looks like FileStore inherits from abstract store, which does not implement the fetch_snapshots method. I can see the stored files under tmp/miniprofiler - Do you know how I can retrieve one of the files and actually view it through the app?

nateberkopec commented 1 year ago

There are two possible resolutions, we will accept a PR for either or both:

  1. Warn or do not display links to the user if the backend does not implement snapshots.
  2. Implement snapshot storage for all backends.