Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
adambair opened this issue Nov 23, 2020 · 6 comments

Comments

@adambair
Copy link

adambair commented Nov 23, 2020

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 #459 (comment) 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
Copy link
Member

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

@OsamaSayegh
Copy link
Collaborator

OsamaSayegh commented Nov 24, 2020

@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
Copy link
Contributor

ivanyv commented Jan 9, 2021

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

@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
Copy link

LagTag commented Jan 13, 2021

@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 nateberkopec changed the title Snapshots page errors in default configuration fetch_snapshots_overview not implemented for FileStore - support or warn if not supported Apr 7, 2023
@nateberkopec
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants