UI changes for profiling state in debuggers

[connor4312]

Per previous explorations, there are two changes we would like to make for profiling in VS Code. I've presented these as independent features, however we could also take the approach of supporting a generalized "profiling" state through DAP and drive the UI from that.

[ ] 1. The ability to contribute a hover action to targets in the call stack view.

This serves as a point of discovery for profiling. An additional contribution point like debug/callstack/actions would be the most straightforward approach for this.

[ ] 2. The ability to set a custom sub-state for debug sessions

This could be a VS Code-specific API like how we can change the DebugSession.name today, which would feel natural to me. Alternatively, it could be a new method on DAP. @weinand interested to hear your thoughts, I will open a proposal on the DAP repo if we want to go that route.

[x] 3. Disabling pausing the profile is running

For discussion: this is in the mockups and something I mentioned wanting before, but thinking on it further I would rather enable it and simply prompt the user when they click it with a modal like

Pausing your program will interrupt the profiling process. Do you want to stop profiling and pause now? [Always] [Yes] [[No]]

With this implementation we would not need UI changes.

[isidorn]

1. This makes sense, menu contribution point like debug/callstack/actions makes sense. However note that we show Sessions, Threads, StackFrames in the CALL STACK View. So we should either use when clauses to specify that this should only be shown on sessions or the menu contribution point should make it clear that this is only for sessions
2. This more feels to me like a description than a sub-state. Note that based on state various actions and things get enabled / disabled. Having it set via VS Code api feels like the way to go here.
3. As you mentioned this can simply be handled on the extension side - you show a modal dialog, I like that idea!

I like your mockups, feels to me like we are heading in the right direction.

[connor4312]

1. I think when's are good. We use that pattern for the context menu, so using it in the actions would make sense. Not sure if other debug extension want to do more e.g. call stack specific actions, but if they do then that would make it available to them.

[isidorn]

Looking into this and thinking how we could split up work.

@connor4312 How about the following suggestion:

• introduce 'inline' group to the existing menu contribution point debug/callstack/context. For start this contibution point will only be supported on the sessions. In the future we might add more @isidorn
• introduce debugSession.description to VS Code proposed API @connor4312 @weinand
  • Render description in the call stack view @isidorn
• Handle the pause action while profiling on the extension side @connor4312

This milestone we can do the first point - to have a menu contribution point. And the description vscode API would be best to tackle in may, since there is no need to rush this in. @connor4312 we should create a api-proposal and assign it to May so there is time to present it in the API sync.

Let me know what you think.

fyi @jrieken for the menu contribution point menu name

[jrieken]

Yeah, the name-generation-engines says debug/callstack/context (like scm/resourceGroup/context) because it is contextual actions. Define that the "first" group of that menu shows inline while the others go into the "..."-overfow-menu. Usually the name of that group is 'inline' (again, see git)

[isidorn]

@jrieken awesome. Yeah we should use the "inline" group. Editing my comment from above.

@connor4312 as the context when the contributed action is triggered on the session we would pass the session id to the extension. You can use the id to match the session, the only problem is that there is no API to get all sessions so you would have to store the session which you create, or to simply listen to our session creation events and store them.

[weinand]

@connor4312 you asked for input on

2. The ability to set a custom sub-state for debug sessions

From the DAP perspective the "stopped state names" are coming from the "description" attribute of DAP's "stopped" event. So in theory a debug adapter could just provide a formatted description that includes the "sub state". But this does not work for "Running (profiling)" because the string for the "Running" state is not received as part of a "Stopped" event but it is hardcoded in VS Code (@isidorn is this correct?)

So a clean solution would be to make the client (VS Code) receive the "Running" state string via DAP. Some ideas:

• we add a "description" attribute to the "continued" event. Problem: the "continued" event is optional. @isidorn would it work if js-debug would send a "continued" event with a "Running (profiling)" in a new "description" attribute?
• we introduce a new "state" request that VS Code calls whenever it updates the state of a thread to "Running".
• running out of ideas...

[isidorn]

@weinand Correct, "running" is hardcoded in the VS Code ui here

• Using the continued event will not work since it is not required by the adapter to send this, as specified in comments here. So it would be a bit strange that in the profiling case the debug adapter sends a not needed continued event only for the description
• Why not the original idea, that this can only be changed via vscode.d.ts api, we just add a new field description on the DebugSession?

[weinand]

@isidorn how would you explain the semantics of the debug session's description attribute?
It seems to be state related, but a debug session has no state attributes yet.

I think the only approach that makes sense is to get the "running" state description via DAP, because that's where the other descriptions are coming from. Having two sources for those strings is rather strange.

So we could provide it with the initial capabilities but that would make it more difficult to change the string dynamically, e.g. from "Running" to "Running (profiling)"

Or are you suggesting to show the "(profiling)" not as part of the session's state at all, but to move it to the name of the session?

[connor4312]

I like the idea of having the description be set purely in DAP. The displayed state is correlated and partially controlled by the debugger state already, and keeping it centralized in DAP is useful.

The continued event is optional, but we could say either that the continued event must also be sent if the adapter wants to update the description, or introduce a description in the responses that apply continuation.

One other slightly weird part is that it's possible to start profiling when we aren't paused, so would be emit an continued event even if we were already running? Introducing a separate method to set the description would decorrelate it from the running/paused state which is currently used to drive the description, so I don't think that would be a good alternative.

[weinand]

@connor4312 would it work for you if we decouple the "(Profiling)" from the state by appending it to the session name? That would solve the "weird part" you've mentioned above.

[weinand]

BTW, I created a DAP feature request for addressing the issue with the hard-coded "Running" state.

But please note that this feature would not automatically solve the issue of updating the "Running" state string at arbitrary times.

[connor4312]

Yea, appending (Profiling) to the name would be a good interim solution.

[isidorn]

We are now supporting contributing inline group to the debug/callstack/context menu.
I am not sure if we need to tackle more things here for this milestone (since we decided to append (Profiling) to the session name most of the work now seems to be on the extension side). So let me know if there is anything else I should look into. Thanks

[connor4312]

Yep, it's just work for me and the DAP issue that Andre opened.

[connor4312]

Okay, all of these are now done, pending possible DAP support for additional state descriptions. Thanks everyone 🙂