working on session panel, corollary fixes

[jvanasco]

I ran into some issues when working on an update to my package pyramid_session_multi, and decided to finally tackle #313 and backport some stuff into the standard debugtoolbar.

1. This introduces a new Session Panel to visualize the changes to a session. There is a Session section on the RequestVars panel; either that could be removed or this could go into that panel.

2. One of my tests inserted a float via random.random() into the session. This caused a fatal exception under Python3 due to sorting comparisons between a float and string.

• I updated several sorted methods in pyramid_debugtoolbar to handle this
• When the RequestVars and/or Headers panel(s) failed from the sort, the Headers panel never received a .response attribute and created another fatal Exception, which obfuscated/buried the stack trace about the original exception. I adjusted that panel with a default .response attribute and a conditional check.

Tests are provided, along with an example of the Py3 sorting exception; I figured I would include that with an AssertRaises, in case that behavior changes and the workarounds are no longer needed.

This works as intended, but I am considering it a WIP that needs feedback for two reasons:

• There is some duplication of data between two panels
• The RequestVars panel does a fancy trick to access the session via the wrap_load. I'm not sure if that is compatible or necessary with my attempt at this panel, as the new panel must always access the session.

Because of this, I think the right approach may to be keep RequestVars as-is, introduce the new panel, but have it controlled on a per-request/cookie basis like the profiling panel.

[mmerickel]

I'm hesitant to consider adding any panel that is off by default. Even the performance panel shows some basic information when disabled. I'd expect this to use the lazy load features to avoid loading / accessing the session unless it was loaded by the request itself.

[mmerickel]

That being said, the idea of a session panel being included is nice. I think we should add one. Could you include a screenshot of what this panel looks like? Obviously if we're merging this we'll want one anyway included in the docs, but you don't have to go that far yet.

[jvanasco]

Perhaps the "panel" could lazy-load by default; but would always show the information if a cookie/request instructed it to. (panel in quotes, because this could be wrapped into the RequestVars panel)

Here's a quick screencap from a test environment. The blue exclamation badges mark that value has changed. The keys are sorted, and we see the "in" and "out" values.

[mmerickel]

If you feel that strongly that you should be able to trigger session loading then fine, there can be some sort of toggle for it, but I'm not seeing it being useful. I want to see session data on requests that need it, and don't want to trigger all the side-effects that come with loading a session on requests that don't need it. It makes the behavior of the app fairly significantly different than when the toolbar is disabled in production.

[jvanasco]

I definitely understand and appreciate that, and don't want to change that functionality - which is why i brought up the "always on" setting to start with.

Also, it does seem like a major deficiency in this PR is not noting if the session was accessed.

For context:

1. Inherent to the functionality of this panel, there is only one opportunity to read the session data "in" - which is why it's "always on"*. The "smart" features you describe can only happen on the session data "out". (*The original values of cookie based sessions could be copied and analyzed, but that doesn't work for server-side sessions)

2. I've often used the "always show the session info" to debug situations where the necessary info to determine the problematic user-path was in the session.

[jvanasco]

I stripped tests because this is still WIP, but I wanted to see if this current approach seems okay.

• By default, the panel only displays session data that was loaded during the request.
• The checkbox can be used to enable an advanced mode, where it ALWAYS loads the session, both before and after the request processing.
• the request_vars wrapped load approach is used to track if the session was accessed during the main request or not. this is showcased to the user.

While doing this, I also found and corrected a small bug in the toolbar javascript. the string join and splits for active panels were on different characters, and duplication was possible.

[jvanasco]

converted this to draft; still working on tests.

[mmerickel]

I'm not clear on the disconnect here but the wrap_load function from the request_vars panel will allow you to capture the ingress values. There is no need for a setting. This function invokes a callback on your class when it is first loaded, where you can copy the values. Then later you can process the response, check if the values are still the same, etc. You should be using that function in the same way that it is working in request_vars. If it isn't crystal clear, the request_vars panel is capturing the INGRESS values of variables like the session already. And only when it's accessed inside the request.

Finally, the session should be removed from the request vars panel entirely if it's being moved into this new panel.

[mmerickel]

You should also use pyramid_debugtoolbar.utils.dictrepr, similarly to the request vars panel.

[jvanasco]

The current attempt - I converted to a draft while I iron some stuff out - is to implement the wrap_load from the request panel as default. That will offer the same exact functionality as the session panel, as you described above.

The setting will enable a new functionality, which will load the session data immediately. It uses a modified version of the wrap load, which will alert the panel if the session was accessed.

So, by default: the same functionality. The ingress values are only logged when the session is accessed by a user. But if the new setting is enabled, the ingress values are always logged on panel setup - so they are available when trying to debug problematic user flows.

[mmerickel]

I’m really having trouble buying what you’re selling. If the request doesn’t use the session then I really don’t think the toolbar should load it.

[jvanasco]

How does this version look? The docs and screenshot have been updated. (see https://github.com/jvanasco/pyramid_debugtoolbar/blob/feature-session_pre_post/docs/session.png )

The minimum supported version isn't an issue for me. I'm glad it works on earlier Pyramid.

I redid the wrapped loaders. By default the package only tracks ingress/egress on accessed sessions. Using a cookie, either per-request or via the toolbar settings, will enable the toolbar to peek inside the session for advanced debugging. In both circumstances, the toolbar informs developers if the Session has been accessed or not.

I updated the toolbar.js to use a variable as the delimiter. this was only done to aggregate the cookie name and panel delimiter at the top of the javascript file, so they could be documented alongside each other - and referenced back to toolbar.py in comments. the python file also references the javascript file now too. I added comments in a few places were things weren't too clear.

The "peek inside the session" functionality has saved us countless hours on debugging complex edge cases. Even if the session isn't accessed, it lets us look at the internal structure - such as user ids - so problematic user flows can be detected. If you are using browser tools to swap between cookie profiles, enabling this lets you quickly document the exact user flow.

[stevepiercy]

This is really good work. Thank you!

I've added some change requests that are mostly formatting, a few grammar fixes, and other items to maintain UI consistency within the toolbar. I'd appreciate your consideration of them.

[mmerickel]

What's the point of your variant of wrap_load versus using wrap_load itself? Is there a good reason this part of the code can't be more DRY?

[jvanasco]

Thanks for the feedback. I'll address everything on my next block; likely Friday or Monday.

IIRC, I think I just didn't want to migrate wrap_load into the utils file, and have both reference it in this PR; I wanted to avoid the panels referencing each other. I can certainly do that though.

[mmerickel]

It's fine with me if they reference each other atm. Moving it to utils would be nice too but not required.

[stevepiercy]

Thank you! The changes LGTM.

[jvanasco]

@stevepiercy when you get a chance to give a spin, please let me know if you find it useful. the internal version of this had really saved me a lot of frustration debugging some problems.

[mmerickel]

Nice work, I released 4.9 with this.

[stevepiercy]

@jvanasco we tried this out today during my Pyramid Training for PloneConf, and the step on sessions did a good job of demonstrating the ingress and egress, while reloading the page would increase each counter. I watched participants' eyes light up, "oooh purty!" Nice work!