Custom timing columns #81

Merged
merged 5 commits into from Oct 8, 2012

2 participants

@rtomayko

This is an experiment in adding custom timing columns as discussed here:

I’d love to be able to show things like RPC calls + time, redis requests + time, and memcached requests + time. The existing SQL column feels like a perfect template for displaying this information. I imagine additional columns filling in to the right of the SQL one.

This is still really rough to look at but is basically working as imagined:

It's a lot harder to read than I pictured in my head but I believe that's mostly due to the distance of the extra columns from their containing timing. I'd like to play with the spacing and maybe some light zebra striping to see if that helps.

I haven't added specs for anything yet and am still mostly just experimenting but would love to get your feedback on the general direction.

Also, slightly off topic, but I'm curious if you've ever played with using the new ActiveSupport::Notifications APIs to get SQL timings instead of shimming into the various adapters. In the app I'm testing this with, I'm tapping into the AS::Notifications events for all the custom timings and reporting to the MiniProfiler#counter API added in this pull request. It feels very clean and organized. Might be worth looking into as I believe most of the timing data you're using here is available.

/cc @josh, @brianmario, @tmm1, @tnm, @kneath, @github/perf

@SamSaffron
Owner

I like this change, a lot.

My 2 main concerns are:

  1. I can not tell easily, but it seems this is not compatible with the .net fork (the js seems incompatible), can @Haacked or @xpaulbettsx have a look?
  2. The naming should be consistent ... if we are calling these things "counters" it should flow as counters all the way through, being called CustomTiming in one spot and counter in another is probably a tiny bit confusing.

This is an awesome use of AS::Notifications API, we should introduce this default on in the railstie.

The reason I opted for a non AS::Notification db hook was 2 fold. Firstly, pre rails 4 you don't get before and after hooks. Secondly, I wanted to keep this compatible with non AR and rails 2 apps. I am totally open to re-designing the hooking a bit so you can opt for AS::Notification db apis as opposed to the monkey patching (especially in the rails 4 context)

There is another advantage for the direct hooking, we are able to get both db time and time for materialization, see: https://github.com/SamSaffron/MiniProfiler/blob/master/Ruby/lib/patches/sql_patches.rb#L64

@SamSaffron
Owner

@rtomayko also ... slight apology in advance, I am out most of next week so please forgive me if I am a tad slow to respond here.

@rtomayko

Totally agree on consistency. I started with the existing JSON structure and cribbed from SqlTimerStruct, RequestTimerStruct, etc and so wanted to stay as consistent with that as possible. Then I remembered the suggestion of adding the MiniProfiler::counter API. Personally, I think changing "counter" to MiniProfiler::timing would be a good way to tighten everything up.

No worries on time. I'm fully expecting to work with this out of my fork for some time anyway.

@SamSaffron
Owner

I don't want this to diverge to much, so I am going to pull it in now. Will talk to @dixon hopefully he can confirm the .net bits still work as expected

@SamSaffron SamSaffron merged commit d4ab63f into SamSaffron:master Oct 8, 2012
@rtomayko

Oh wow. Awesome!

Do you want me to switch the API to MiniProfiler.timing from MiniProfiler.counter. That's one thing we should get right before letting too much time pass with this on master.

@SamSaffron
Owner

sure go for it, much prefer a consistent name

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