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

Add support for breakpoint dependencies (trigger points) #152428

Closed
gayanper opened this issue Jun 17, 2022 · 25 comments
Closed

Add support for breakpoint dependencies (trigger points) #152428

gayanper opened this issue Jun 17, 2022 · 25 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-testplan under-discussion Issue is under discussion for relevance, priority, approach

Comments

@gayanper
Copy link
Contributor

When debugging a production level application like an angular app, java microservice we often need to add breakpoints into methods which are called frequently. For an example rendering method in the frontend or event subscriber on the consumer. But most of the time we are only interested to suspend on these methods when it was triggered from a another operation which we are interested of.

How this is done in Eclipse is using trigger point, We can add a trigger point (different kind of a breakpoint) on the operation which is called before our debugging method, so once only this trigger point is hit, our rendering or event subscription breakpoint will get enabled. In IntelliJ this is done using breakpoint dependencies.

having this support for vscode would be really beneficial, if we can build this on the vscode debug extension layer, we could even support polygot scenarios, where the trigger point is in one app we are debugging and actual breakpoint which get enabled might be on another app.

@weinand weinand added debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality labels Jun 17, 2022
@weinand weinand modified the milestones: Backlog Candidates, On Deck Sep 8, 2022
@gayanper
Copy link
Contributor Author

gayanper commented Sep 8, 2022

@weinand what if we go with the approach of making the dependent breakpoints hit state a different type of condition ?

So that in breakpoint widget, in condition editor we can add a swicter to switch between expression and break point dependency.

its similar to what intellij offers. WDYT?

@gayanper
Copy link
Contributor Author

gayanper commented Sep 8, 2022

To answer @roblourens about conditions, yes you can have a condition on the trigger point. Because the conditions are evaluated by DA and only if it satisfied the DA will tell the client that the breakpoint is hit. Then only we will act on it to enable non trigger breakpoints as of current implementation

@connor4312 connor4312 reopened this Sep 8, 2022
@weinand
Copy link
Contributor

weinand commented Sep 8, 2022

@gayanper above you said:

what if we go with the approach of making the dependent breakpoints hit state a different type of condition

Yes, something along the lines of your suggestion was my first thought as well....

But I wonder whether we can (or even should) map "dependent breakpoints" to "conditional breakpoints".

  • For a conditional breakpoint the breakpoint is always hit but the condition is evaluated to determine whether the "hit" is actually shown to the user. Evaluating the condition on every hit can have a big performance impact.

  • Dependent breakpoints on the other hand only need to become active when the trigger point is hit. So there is no need to have dependent breakpoints always enabled because they don't need to evaluate a "trigger condition" repeatedly. Instead a hit of a trigger point can just enable them. This significantly reduces the performance impact of "dependent breakpoints" (which is good).

Given this conceptual difference, I don't think that we should try to fold "dependent breakpoints" into "conditional breakpoints".

But I see an opportunity for introducing the concept of a "breakpoint action": the user can specify what should happen when a breakpoint is hit. One option would be to "enable other breakpoints". With this the breakpoint would become a "trigger point".

I remember feature requests for "breakpoint actions" but I cannot find them right now in the VS Code repo.
However here is a question on Stackoverflow: https://stackoverflow.com/questions/67919379/run-code-whenever-a-breakpoint-is-hit-in-vscode-python

@weinand
Copy link
Contributor

weinand commented Sep 8, 2022

BTW, found some older duplicates for trigger points:

@weinand weinand added the under-discussion Issue is under discussion for relevance, priority, approach label Sep 8, 2022
@gayanper
Copy link
Contributor Author

gayanper commented Sep 8, 2022

@weinand I think i was not clear on my last comment. What I meant was to use the condition UI widget to add breakpoint relation. But I think you explain it very well in your comment. So yes we could implement support for breakpoint action. How do you think its better from the UX

  • having a new menu option called "Set breakpoint action" and show a quick pick with list of existing breakpoints and and place holder text which says "Enable selected breakpoint when hit"

  • Introduce a new widget similar to condition and log message editor.

I think the first option might be better since such UX flows are already there in vscode. But let me know what you think. The we can store the depdency breakpoint information like (uri, line, column) as a identifier with the trigger point and disable it. We can also make sure then starting the debug session also to disable such dependent breakpoints.

WDYT ?

@gayanper
Copy link
Contributor Author

@weinand any feedback ?

@gayanper
Copy link
Contributor Author

Please provide feedback so I can continue this work on a agreed path to avoid rework :).

@weinand
Copy link
Contributor

weinand commented Sep 19, 2022

@gayanper Sorry, but I'm on vacation for the next three weeks...

@gayanper
Copy link
Contributor Author

@gayanper Sorry, but I'm on vacation for the next three weeks...

Sorry @weinand, You can pick this up after vacation then, We will park this issue for now then. Hope it will not get closed.

@gayanper
Copy link
Contributor Author

gayanper commented Oct 15, 2022

@weinand hope you are back from vacation and had good time. I got some time to change the design and start with the UI part.

WDYT about the below

image

Here i added a new option for condition breakpoint to select the breakpoint which the current editing one should wait for. If the UI part looks good, my idea is to build this as another type of condition input in addition to expression where we can use the same icon as well.

Let me know what you think.

@gayanper
Copy link
Contributor Author

ping to see if anyone can provide me feedback to finish this feature.

@gayanper
Copy link
Contributor Author

screencast.mp4

New change in action

@gayanper
Copy link
Contributor Author

screencast-typescript.mp4

typescript in action

@weinand weinand removed their assignment Nov 22, 2022
@roblourens
Copy link
Member

I will take a look at this @gayanper, I'm still a little unsure about committing to the feature as a whole, and it needs some UX work, but I will take a closer look when I have time. Due to the holiday, that may not be until early December. Thanks for your work on this so far.

@gayanper
Copy link
Contributor Author

gayanper commented Dec 4, 2022

I will take a look at this @gayanper, I'm still a little unsure about committing to the feature as a whole, and it needs some UX work, but I will take a closer look when I have time. Due to the holiday, that may not be until early December. Thanks for your work on this so far.

Let me know if we need to do some UI changes, i don't mind spending some time on changes that requires to make this change available in vscode.

@gayanper
Copy link
Contributor Author

Any update on this issue and the connected PR ? Could we have a timeline where this can be discussed and see how we can get this functionality merged. Also if vscode team thinks this is not something that would fit the product please let me know as well, so I can look into how i can implement same functionality as an extension using the extension API

@gayanper
Copy link
Contributor Author

gayanper commented Jan 8, 2023

@roblourens @weinand I also see that Jetbrains Fleet also trying to keep breakpoint properties simpler without adding other than conditions. Are you considering the same ? If so do you see implementing the same feature with a different configuration panel as an extension might be good choice and may be suggest extensions-points/APIs for missing pieces ?

@roblourens
Copy link
Member

Yeah, this should be possible with our extension APIs, e.g. debug.addBreakpoints:

export function addBreakpoints(breakpoints: readonly Breakpoint[]): void;
and at the moment I am leaning towards saying that this sort of functionality would be better in an extension. Of course then there would be a compromise of UX and functionality. I'm wondering whether you'd be interested in doing some investigation here, like a quick prototype, to see whether the API we have could give a decent experience for this feature.

@gayanper
Copy link
Contributor Author

@roblourens Yes thats sounds like a good idea. I can try that and share here.

@mblout
Copy link
Contributor

mblout commented Apr 21, 2023

I'm finding that I need (source) breakpoint enhancements for some functionality we are trying to duplicate from an existing debugger, and I think it overlaps with a lot of what has been mentioned here.

At a minimum (MVF - minimum viable feature)-

  • an additional property in a (Source)Breakpoint (just a string?), that only has meaning to the extension
  • ability to set this data programatically on creation (onWillChangeBreakpoints? or anything?)

Bonus:

  • ability to change the visualization of the breakpoint; minimally, some color/image/anything simple. and being able to change it at any time; for my case, being able to make it 'not there' would be ideal; meaning, i could hide it, and the user could add another source breakpoint (same place, or elsewhere).
  • ability to represent the additional property somehow in the breakpoint list view

My use case is, while debugging, i need the be able to allow the user to set a breakpoint for the selected thread in the UI (I know it sounds weird, but threads are really 'tasks' in our use case). So, if debugging, and they select 'thread' X, only the breakpoints for 'thread' X show, they can add remove as normal, and those breakpoints are created with some property indicating they are meant for thread "x". And when not debugging, all breakpoints show.

I'm currently trying to find some alternate compromise with some combination of 'tricks' below that could allow us to move forward for now, but its all very strange for the user;

  • keeping track of 'additional breakpoint' information ourselves in workspace storage (and showing in the bp view tooltip)
  • listening for breakpoint changes, and immediately deleting and (re)adding breakpoints (trying to leverage 'column' somehow)
  • using the (new) editor/lineNumber/context inline menu stuff.
  • using 'unverified' state as the the closest thing to 'hiding', and trying to explain it in the tooltip message in the breakpoint view.
  • maybe leveraging two-lane gutters to help with the "is valid for selected thread" visualization
  • etc.

It might make some sense to separate out to a separate feature-request, but wanted to mention it here first, if there's ideas how how these might be related. I also understand that separating the "UI-related" parts might make the non-UI parts easier to provide.

@gayanper
Copy link
Contributor Author

Yeah, this should be possible with our extension APIs, e.g. debug.addBreakpoints:

export function addBreakpoints(breakpoints: readonly Breakpoint[]): void;

and at the moment I am leaning towards saying that this sort of functionality would be better in an extension. Of course then there would be a compromise of UX and functionality. I'm wondering whether you'd be interested in doing some investigation here, like a quick prototype, to see whether the API we have could give a decent experience for this feature.

I look into implementing this feature as an extension, But unfortunately I don't see a way of listening breakpoints to get notified when they are hit. Is there a a possibility to do this ?

@connor4312
Copy link
Member

Done in #166202

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-testplan under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

7 participants
@roblourens @weinand @connor4312 @gayanper @mblout @aiday-mar and others