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

Request for clarification on interaction among condition, hitCondition, logMessage in breakpoints #363

Closed
fabioz opened this issue Dec 8, 2022 · 7 comments · Fixed by #366
Assignees
Labels
clarification Protocol clarification
Milestone

Comments

@fabioz
Copy link

fabioz commented Dec 8, 2022

Background:

Up until now what makes sense for me is that when logMessage is set breakpoints are skipped, but if condition or hitCondition is also set along with logMessage it'd log the message but it'd still break if the condition becomes true, even if logMessage is also set.

However, a user brought up (in microsoft/debugpy#1146) that the description of logMessage mentioned that in https://microsoft.github.io/debug-adapter-protocol/specification#Types_SourceBreakpoint just says in logMessage: "If this attribute exists and is non-empty, the debug adapter must not 'break' (stop) but log the message instead."

I think that this is an oversight in the spec (given that I expected breakpoints to stop in that case and the spec doesn't really say anything about the interaction with other properties -- and this is also how the javascript debugger in VSCode works), but I'd like to confirm if this is the case or not.

@roblourens
Copy link
Member

You could also argue that setting condition and logMessage turns it into a "conditional logpoint" that only logs when the condition is met.

I agree that the spec should clarify this. However, changing that line in logMessage is probably a breaking change. I'll have to find the original discussion.

@roblourens roblourens added the clarification Protocol clarification label Dec 8, 2022
@roblourens roblourens self-assigned this Dec 8, 2022
@puremourning
Copy link
Contributor

You could also argue that setting condition and logMessage turns it into a "conditional logpoint" that only logs when the condition is met.

This was my interpretation too. Any other interpretation makes the arguments conflicting. The spec is IMO correct today.

@msmans
Copy link

msmans commented Dec 9, 2022

I'm the user who reported this on debugpy.

You could also argue that setting condition and logMessage turns it into a "conditional logpoint" that only logs when the condition is met.

Yes. I actually expected this without having read the spec. Conditional logpoints are also a more flexible option. In other words, it's superset of the current behavior of debugpy. A hot code path can produce too much noise when logging. Being able to only log under certain conditions is useful. A real world example is my need to log certain requests among 10K+ which satisfy certain headers.

@connor4312
Copy link
Member

connor4312 commented Dec 9, 2022

I suggest the following wording on the logMessage property:

If either hitCondition or condition is specified, then the message should only be logged if those conditions are met.

I believe the existing requirement, "the debug adapter must not 'break'," is sufficient otherwise.

We can also clarify another similar ambiguity on hitCondition while we're there:

If both this property and condition are specified, hitCondition should be evaluated only if the condition is met, and the debug adapter should stop if both conditions are met.

@int19h
Copy link

int19h commented Dec 9, 2022

My understanding of logpoints and their use case is that it's basically an injectable conditional print. For that to work, it has to not break (and if the user desires to break, they can put another breakpoint on the same line, right?). It's also how they have worked in VS historically.

@puremourning
Copy link
Contributor

We can also clarify another similar ambiguity on condition and hitCondition while we're there:

Clients may specify at most one of condition or hitCondition for a given breakpoint

I think you can specify both, no? hitCondition counts the number of times the 'condition' is met.

@connor4312
Copy link
Member

That's reasonable. I've updated my proposal above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Protocol clarification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants