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

Unclutter CALL STACK view by eliminating parent debug session #99736

Closed
weinand opened this issue Jun 10, 2020 · 22 comments
Closed

Unclutter CALL STACK view by eliminating parent debug session #99736

weinand opened this issue Jun 10, 2020 · 22 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes on-testplan
Milestone

Comments

@weinand
Copy link
Contributor

weinand commented Jun 10, 2020

When debugging simple "one process" JS programs, there is always a top level debug session that clutters the CALL STACK view:

2020-06-10_10-07-57

We should try to get rid of this either:

  • by hiding it in the call stack view (e.g. by using tree's compressed feature), or
  • introducing some new abstraction in the debug architecture (e.g. "debug context"), that a debug extension can use, but is not necessarily visible in the call stack view.

In the general case (with multiple debug sessions) it is still needed to have a parent, but we should better indicate the purpose of the parent. Today (as seen in the screenshot) we show the parent as a regular debug session and duplicate the stopped reason ("Paused on Breakpoint"). This is really confusing. As a first step we should remove the reason and show a different icon or no better no icon (and probably offer different context menu actions).

@connor4312 @isidorn what's your opinion?

@weinand weinand added this to the June 2020 milestone Jun 10, 2020
@weinand weinand added debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality labels Jun 10, 2020
@isidorn
Copy link
Contributor

isidorn commented Jun 10, 2020

Love this suggestion, and I think we should really look into this to reduce the clutter and thus make debugging seem less complex.

Using the tree compressed feature might just work, however what would be the title in the example above. I think it should just be Launch Program?
Maybe I can try with the compressed tree approach and we will see what limitation we hit.

What do you think?

@connor4312
Copy link
Member

Using the debug context is more work, but I think it's also the best way to solve this. For js-debug, we don't really care about the top level container being a "session", we just need 'something' to put our sessions in. If there was the concept of contexts, that would also be a more generalized solution to the stopAll in compound launch configs (#95964).

However a compressed tree would be much less complex on the vscode and api side. 👍 on trying that first and seeing where that gets us.

@isidorn
Copy link
Contributor

isidorn commented Jun 10, 2020

Ok, so let's start with compact tree. Thus assigning to me, and once we have that we can figure out what to do next.

@isidorn
Copy link
Contributor

isidorn commented Jun 16, 2020

I have pushed a first iteration of this, now we use a compressed tree in the call stack view. Please note the following:

  • We only compress sessions like this (not other call stack elements)
  • The label which we show is simply labels of compressed session conacateneted
  • In the worst case this can be unexpected to some debuggers that use the .parentSession feature so we should double check with the python team since I think they use this feature
  • When user right clicks, or does any action on this compressed element the action is executed on the last session

Potential follow up work:

  • Fine tune the label, either by only using the parent label, or @connor4312 could make the child label more concise
  • Contact other extension authors, for example python
  • When there is only one compressed session maybe we should hide it as well?

Here's the current state of thigns. Try it out and let me know what you think

Screenshot 2020-06-16 at 11 58 33

isidorn added a commit that referenced this issue Jun 16, 2020
@isidorn
Copy link
Contributor

isidorn commented Jun 17, 2020

fyi @luabud do you know when Python uses child debug sessions? And if this behavior of compressing single child sessions makes sense for Python?

@int19h
Copy link

int19h commented Jun 18, 2020

There are two major scenarios for Python involving multiple sessions.

The first, and the most common, involves web frameworks that have auto-reloading functionality (e.g. Flask, Django) - those usually run the reloader in the parent process, and user code in the child process, and the former restarts the latter when files change on disk. In this scenario, the users really only care about the child process - but since we don't actually know that there's going to be one (it's configurable), the whole thing has to be arranged as a multiproc session.

Collapsing the child into the parent should therefore improve UX in this case, but I do have one concern about the behavior of Stop etc on the collapsed item. If I understand the above correctly, the actions it would show, and their behavior, would correspond to the child session (since it's always the one spawned last). This would be a problem in our case, because the child session is "attach", even if the parent is "launch" (which it usually is) - and so if the child session is driving UI for the collapsed representation, it would show "Disconnect" rather than "Stop".

(FWIW, we've already seen reports of confusion from the users with current behavior, where they see "Disconnect" in the floating toolbar when the child session is the one highlighted in Call Stack. But right now they can at least see the whole picture in Call Stack, and figure it out that way.)

The other scenario is the use of various frameworks for parallel computing that rely on multiprocessing - this is more common in Python compared to other languages, because multithreading is throttled by GIL. Even the standard library has some facilities for this (e.g. multiprocessing and concurrent.futures.ProcessPoolExecutor), and it's very common in advanced data science applications. But in this case, there's always more than one child worker - there would be no benefit from it otherwise - and so they wouldn't be collapsed under this proposal.

With respect to the labels, it would be nice if we had some control over it (without hacks, such as reporting an empty string for child sessions, so that concatenation is a no-op). Currently we do something like this:
image
so if they're concatenated, it would be rather confusing. Only using the parent label would also work fine.

Can you clarify the part about .parentSession? Would this proposal change its behavior as well, or is this just a different visual representation of the same object model? I don't think we ever actually read it - we only use the corresponding argument to startDebugging() - but it would be good to know for future reference.

@isidorn
Copy link
Contributor

isidorn commented Jun 18, 2020

@int19h thanks a lot for jumping into the discussion.
So you have 2 cases

  1. There are already issues with the presentation of your first scenario, as you say users as confused. So I would argue that there even might be slight improvements here
  2. This case is not a problem since there are always multiple children

Yes, you understood correctly the child session would drive the UX.

I have just changed the label computation to only use the parent label. I think that would also work for js-debug.

And yes this changes are only about the visual representation of the same object model. Nothing actually changed underneath. Also the .parentSession is not exposed on the DebugSession object to extensions.

So the only remaining issue is that the Compact object would show the disconnect, not the stop. I suggest we tackle this as a seperate issue.

For now closing this as we have done what we planned. For further improvements I suggest we open new issues.
@int19h can you be so kind to try this out with latest VS Code insiders? The current one has the label computated, but from Friday should just show the parent label.

@weinand are you aware of other debug extensions that might use the parent debug sessions?

@isidorn
Copy link
Contributor

isidorn commented Jun 18, 2020

We can have a "heuristic": never show disconnect on child sessions, always show stop.

Let me know if you think this makes sense in general. Since I believe there were other cases where disconnect is unexpected.

@int19h
Copy link

int19h commented Jun 19, 2020

For our scenarios, users generally expect the behavior of Disconnect/Stop to correspond to the type of the root session, or rather, to the debug configuration that they have used to start it - i.e. Stop for "launch", Disconnect for "attach". That makes it consistent with single-process debugging. Indeed, from users' perspective, it is logically a single process with "threads" in it, and we'd like to get as close to that experience as we can.

There are some very advanced corner cases where the user might want to detach from one specific child session, even though the root is "launch". But I think that's best handled via the Call Stack per-session controls that are already there (these scenarios all involve multiple child sessions, so they wouldn't be collapsed). All user confusion that I've seen so far was about the floating toolbar adjusting to the current selection in Call Stack.

With this change as is, while the UX is simpler and better overall, there's basically no way to Stop rather than Disconnect that I can see, since now both the floating toolbar and the Call Stack only show Disconnect - here's how this looks on Insiders:
image

@weinand
Copy link
Contributor Author

weinand commented Jun 19, 2020

@isidorn if we cannot find a good heuristic to make this "parent compression" work for all extensions, we could provide API to allow extensions to opt into this feature. E.g. a new option on the DebugSessionOptions.

@isidorn
Copy link
Contributor

isidorn commented Jun 22, 2020

@int19h thanks for trying it out with VS Code insiders.
First, the case that you mentioned that the use wants to detach from one specific child session, in that case there will be multiple child sessions and thus this UI will not be used.

As for your other concern that the Call Stack shows only Disconnect and not Stop. That seems to be the same concern that we should show Stop instead.

So my question is: when there is only one child session what is the difference in behavior if the user clicks on the disconnect on the child session vs clicking stop on the parent sessions? Do users differentiate this?

@weinand thanks for suggestion. We can go down that path if we can not find something which works for everyone.

@int19h
Copy link

int19h commented Jun 22, 2020

If they click "Disconnect" on the child session, that's exactly what's going to happen - they'll just disconnect from that session, but the corresponding process is still running; and they're still connected to the parent session. They'll have to stop that parent session next to fully stop debuggee. Which is very confusing in a launch scenario.

But there's also an attach scenario here, where they start the same app by other means. In that case, they'll still have two sessions, but both would be expected to have "Disconnect", and disconnecting from both is expected to keep the debuggee running.

So, the expectation here is that when the child session is collapsed into the parent, the resulting aggregate would behave in the same way as the parent session, because the parent session is where launch/attach is differentiated. If parent was "launch", then users expect to see Stop; if "attach", Disconnect.

In any scenario I can think of where collapse would be triggered (i.e. 1 parent + 1 child), the users don't actually care about the child session from the perspective of Stop/Disconnect. To be honest, they don't really care about it at all - the fact that there are two processes there is really just an implementation detail of the web framework. It just so happens that one of those processes runs user code, and the other one is responsible for the app lifecycle.

BTW, one thing that's not clear to me - what happens to Pause when sessions get collapsed? Does it pause all of them?

@isidorn
Copy link
Contributor

isidorn commented Jun 22, 2020

@int19h Ok, so it seems like we are on the same page. We could make this less complex to the users if we do it right.
Pause and all actions on the collapsed sessions only gets sent to the child session.
As for the disconnect / stop I can think of something to tackle this.

Do you have some users who are using vscode insiders and could provide feedback on this right now?

Bottom line: if pythong would like to opt out of this behavior we can provide some capability. But I think we should first try to make the current approach work.

isidorn added a commit that referenced this issue Jun 23, 2020
@isidorn
Copy link
Contributor

isidorn commented Jun 23, 2020

@int19h I have pushed a change what I already mentioned that the stop and disconnect are sent to the parent. All the other actions are sent to the child. It would be great if you can try this out in the next week and provide feedback so we introduce an opt out mechanism of this collapsing of sessions if you do not like this behavior for python.
I have tried this out for js-debug and it seems to work just fine.

fyi @connor4312

I have also created this test plan item so we cover this #100850

After discussing with @weinand we decided to be on the safe side and add an option to Extension to opt out of this behavior #100852

@connor4312
Copy link
Member

connor4312 commented Jun 24, 2020

One thing I've discovered with js-debug specifically is that a single page with a web worker will get collapsed, which is not desirable.

On stable:

On Insiders/OSS:

@isidorn
Copy link
Contributor

isidorn commented Jun 24, 2020

@connor4312 I can change our approach that we only compress two sessions, never 3. However that might be a bit hackish. Can you elaborate a bit why this is undesirable.

@isidorn
Copy link
Contributor

isidorn commented Jun 25, 2020

@connor4312 nicer solution: we are introducing a noCompact flag #100852 to the debugSessionOptions so for the workers you could just pass noCompact: true. Would that work for you?

@connor4312
Copy link
Member

noCompact/incompressible works. Special casing the second-level to not be compressed feels a bit too specific to js-debug for vscode to do in general; a flag there instead would be perfect.

@isidorn
Copy link
Contributor

isidorn commented Jun 25, 2020

@connor4312 awesome, then just try it out and while you are at it you can test this item :)
#101009

@isidorn
Copy link
Contributor

isidorn commented Jun 26, 2020

I have removed the "hack" that we treat the stoped / disconnect action special in the compressed element.
So all actions are now on the child session. Which means the disconnect will be rendered.
If we would prefer to show stopped in the compressed element and that it is sent to the parent session I think we should add an DebugSessionOptions flag for this such that this can be controlled by the edebug extension.

@int19h
Copy link

int19h commented Jun 26, 2020

@isidorn Is there an issue to track that, or would you like me to file one?

@isidorn
Copy link
Contributor

isidorn commented Jun 26, 2020

@int19h yes please do that, so we can consolidate with @connor4312 on the best approach to take here.
Thanks!

@isidorn isidorn added the on-release-notes Issue/pull request mentioned in release notes label Jul 2, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes on-testplan
Projects
None yet
Development

No branches or pull requests

4 participants