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

Improve console.group in Debug Console #34981

Closed
roblourens opened this issue Sep 25, 2017 · 32 comments
Closed

Improve console.group in Debug Console #34981

roblourens opened this issue Sep 25, 2017 · 32 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-testplan
Milestone

Comments

@roblourens
Copy link
Member

image

vs

image

Can we introduce a concept like this into the debug protocol, and improve the display to support collapsible sections of logs? Do other runtimes have a concept like this?

It's now more visible when debugging vscode.

@roblourens roblourens added the under-discussion Issue is under discussion for relevance, priority, approach label Sep 25, 2017
@auchenberg
Copy link
Contributor

Our console for sure would gain much with simple object formatting / tree structure. Is the debug console rendering virtualized today?

@weinand
Copy link
Contributor

weinand commented Sep 25, 2017

The debug console is based on a tree, so this should be simple to implement.

@weinand weinand added debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality and removed under-discussion Issue is under discussion for relevance, priority, approach labels Sep 25, 2017
@weinand weinand added this to the Backlog milestone Sep 25, 2017
@weinand
Copy link
Contributor

weinand commented Nov 8, 2017

@roblourens is the <Start group>/<End group> markup your adhoc invention and the DA is adding this to the stack trace or does this come from CDP?

I was able to achieve the following with the existing DAP:

2017-11-08_15-08-31

So, the only issue is the array index that appears in front of the stack frames (but we could call this a feature).

The missing link detection is an orthogonal issue that we will fix anyway.

@roblourens
Copy link
Member Author

The markup is just something I added so the group/groupEnd calls aren't totally invisible.

My original example isn't the best example, it's logging a single item as a one-off group. In your screenshot it looks like you're logging an array of strings, which is possible, but I don't think it's the right model for logging a group.

  • The group should be updated live as log messages are produced, where an array could only be logged once the group is closed. At least I don't think I can log an object, then change the object.
  • Groups should be expanded by default
  • They need a log location for each line
  • Nested groups should look good
  • Multiline group titles and grouped messages should look good

@weinand
Copy link
Contributor

weinand commented Nov 8, 2017

What are "good" examples for a dynamic group?
What do you mean by "They need a log location for each line"?

@roblourens
Copy link
Member Author

Code like

console.log('foo 1');
console.group('group 1');
console.log('foo 2');
console.log('foo 3');
console.group('group 2\nmultiline group\ntitle');

let i = 0;
const interval = setInterval(() => {
    console.log('bar ' + i++);
}, 1000);

setTimeout(() => {
    console.groupEnd();
    console.log('ended group 1');
    console.groupEnd();
    console.log('ended other group');
}, 4000)

Looks like this in chrome devtools

image

By "log location" I mean the location that we added for each log last month.

@weinand weinand modified the milestones: Backlog, On Deck Nov 14, 2017
@ktownsend-cbre
Copy link

ktownsend-cbre commented Jun 15, 2018

I just discovered console.group and console.groupCollapsed for using in my react native project and was surprised they didn't actually group with the collapsible arrow like logged objects do. PLEASE implement good visual collapsible grouping sooner rather than later. Although not "critical", it's such a simple thing and will improve my quality of dev-life :)

@rdegelo
Copy link

rdegelo commented Dec 5, 2018

This is open for over one year... Any updates on this? Are we waiting for vscode to support Grouping?

@fortinmike
Copy link

@weinand I didn't want to just add a +1 (we would really like this to happen) so here's another great example of grouping (and the illegible results in Code): redux actions logged by redux-logger

Safari:

screen shot 2018-12-07 at 12 46 33 pm

Chrome:

screen shot 2018-12-07 at 12 44 43 pm

VS Code:

This is so verbose and unclear (especially with the additional whitespace after <Start group>) that it makes the output in Debug Console hard to read and much less useful than the same output in Chrome / Safari.

screen shot 2018-12-07 at 12 48 47 pm

@weinand
Copy link
Contributor

weinand commented Dec 7, 2018

If you "would really like this to happen", you could submit a PR... ;-)

@fortinmike
Copy link

Haha, of course, but it's not an option with our current work pipeline. Plus, I wouldn't know where to start!

Am I mistaken in thinking you had something of a partial implementation, mentioned in #34981 (comment)?

@koutsenko
Copy link

Same issue here. Debugger console is unusable with redux-logger.

@DanTup
Copy link
Contributor

DanTup commented Jul 10, 2019

@weinand how did you get the expand/collapse output in the debug console shown in #34981 (comment)?

As far as I can tell, OutputEvent only deals with strings? I have some tree-like errors I'd like to show here and that looks ideal.

@weinand
Copy link
Contributor

weinand commented Aug 15, 2019

@DanTup this is just an OutputEvent with a variablesReference for the exception object. VS Code will render the variable as an expandable object.

@weinand
Copy link
Contributor

weinand commented Aug 15, 2019

@isidorn An additional aspect of groups is that after a group has been created with a specific name, later console output can be appended to the group if the same name is used.

@pavelfeldman
Copy link
Member

@weinand, @isidorn: I might have confused you wrt names. Console groups are not named, they are synchronous grouping and there is no way to add to the group post-groupEnd. A better example would be a console group that was opened for a very long time (forever). Now all the console output should be getting into that group, so we need to dynamically populate a second level of the object tree.

Unrelated and FYI: console.context was introduced to address these synchronous shortcomings and be closer to the traditional logging systems. console.context('scope') returns an object that has the same API as a regular console. Application can create console.context for any component of theirs and have independent 'consoles' for those. We never told the world about it, so it was never adopted, but I guess we should have. In this case, everything within the context could be logged with the [scope] prefix or within the group.

[server] Starting server on port 8000
[server] Port listening
[client] Connecting to port 8000

@weinand weinand removed this from the On Deck milestone Oct 14, 2019
@weinand weinand added this to the November 2019 milestone Oct 14, 2019
@isidorn
Copy link
Contributor

isidorn commented Dec 2, 2019

Not going to happen in november due to lack of time. Removing the Milestone and I leave it up to @weinand to assign a new milestone and to drive the changes in the DAP needed for this.

@isidorn isidorn removed this from the November 2019 milestone Dec 2, 2019
@isidorn
Copy link
Contributor

isidorn commented Jan 21, 2020

I have experimented with what Chrome can do and came to the same conclusions as @roblourens already pointed out.

We would need to introduce a optional new field in the OutputEvent called groupof type string.
A debug extension can always send an OutputEvent which has a group set and the UX might have a limitation that it only shows groups continously.

Some more details:
VSCode would treat the first OutputEvent with a group set as the parent in the tree that can be expanded / collapsed to show all the other content that has the same group set.
While a group is open any user input / output would belong to the group.
When a group ends the adapter would simply send OutputEvents that no longer has the group property set.
The downside of this approach is that it is not possible to end the group without a new OutputEvent coming in. However I think this will work in practice.

@roblourens @weinand @connor4312 let me know what you think. Thanks!

@connor4312
Copy link
Member

connor4312 commented Jan 21, 2020

The group idea would generally work. I'd prefer a groupId: number--given that the group name would be in the existing output property, having it as a numeric ID makes it a little more clear that it's just a tag and not a display string.


One alternative would be to treat these as variables and use the existing variablesReference. We would need two things for that:

  1. A way to tell the UI, over the OutputEvent, that we want to autoExpand the event's variableReference
  2. A way to update existing variables given through output

With these, I think groups could be implemented without needing explicit knowledge of grouping. This might be more generalized, I can't recall encountering log 'grouping' in other languages. The ability to update existing output variables might(?) be interesting to have elsewhere.

This would let us implement console.groupCollapsed without additional steps, we would just omit the autoExpand flag.

@roblourens
Copy link
Member Author

This is proposed above too, one thing missing is that the log messages also have source locations associated with them, which a variable in an array wouldn't.

Also the group can change after it is first displayed. An array can change after it is logged too, but usually users don't expect objects that they previously logged to change on them.

@weinand
Copy link
Contributor

weinand commented Jan 22, 2020

Since the node.js API has already a console.group(groupName: string), new debug adapter protocol should be able to support that functionality without loosing information (e.g. the 'groupName'). So I think @isidorn's proposal for a new group attribute on the OutputEvent is the way to go.

A client implementation can decide whether it wants to show the group name (which I prefer), or whether it just uses it for implementing the grouping.

What is not clear to me is whether the console.groupEnd() needs to be reflected in the protocol too. Currently in Isi's proposal the group ends implicitly if an output event has no group property.

@connor4312
Copy link
Member

connor4312 commented Jan 22, 2020

Since the node.js API has already a console.group(groupName: string), new debug adapter protocol should be able to support that functionality without loosing information (e.g. the 'groupName').

The group name can be represented as the existing output/variables reference. In console.group, you can also use non-strings there, I think it would make sense to treat it as a 'normal' output event with a group identifier (rather than using the group as a display string to a user).

image

Making the group/Id numeric in DAP makes it more obvious to me as an implementor that it's only an identifying tag, like a numeric variable or source reference. (maybe groupRef would be more appropriate, in that avenue)

What is not clear to me is whether the console.groupEnd() needs to be reflected in the protocol too. Currently in Isi's proposal the group ends implicitly if an output event has no group property.

If I'm reading Isi's proposal correctly with "the first OutputEvent with a group set as the parent in the tree that can be expanded / collapsed to show all the other content that has the same group set", I think we need the explicit end, otherwise in

console.group('a'); // -> { group: '1', output: 'a' } (starts the group)
console.log('1');   // -> { group: '1', output: '1' }
console.groupEnd();
console.group('b'); // -> { group: '2', output: 'b' }
console.log('2');   // -> { group: '2', output: '2' }
console.groupEnd();

...we would not be able to tell that that b is a separate top-level group instead of a child. This could alternatively be solved by having a separate parentGroup property, which would be more explicit.


Regardless of what direction we go I would like to see if we can figure out a way to implement groupCollapsed as well, as that's used in several real-world tools like redux-logger above.

@isidorn
Copy link
Contributor

isidorn commented Jan 22, 2020

@connor4312 in your case the adapter would have to send the following event before b arrives.
{group: undefined, output: ''}
So this is my implicit proposal of the groupEnd.

My proposal does not cover the groupCollapsed and if that is a big part of the feature we should design for it from the beggining. I am not sure if that is just another boolean in the OutputEvent.
It would probably be neater to group it together into an object with the group string as following

group: {
  name: string;
  collapsed: boolean;
}

@connor4312
Copy link
Member

connor4312 commented Jan 22, 2020

I think that if we want to get a solution for console grouping, we should try to find a design that can support it fully to avoid hacking things on later 🙂

I think the group object like you have makes sense, but that will become verbose if added to every output event, and the collapsed flag only has meaning on the first event of the group. Maybe startGroup starts a group and group indicates what group the console item is in? That would reduce verbosity and also avoid needing the extra group: undefined event.

console.group('a');       // -> { startGroup: { name: '1', collapased: false }, output: 'a' }
console.log('1');         // -> { group: '1', output: '1' }
console.groupEnd();
console.group('b');       // -> { startGroup: { name: '2', collapased: false }, output: 'b' }
console.log('2');         // -> { group: '2', output: '2' }
console.groupCollapsed(); // -> { group: '2', startGroup: { name: '3', collapased: true }, output: 'b' }
console.log('3');         // -> { group: '3', output: '3' }
console.groupEnd();

@isidorn
Copy link
Contributor

isidorn commented Feb 11, 2020

Adding this to the February milestone since it is on the plan.
Once we have this in the debug adapter protocol I can look into the implementation.

@weinand
Copy link
Contributor

weinand commented Feb 18, 2020

My summary of the node.js console.group documentation:

  • console.group(one or more labels):
    Increases indentation of subsequent lines by two spaces.
    If one or more labels are provided, those are printed first without the additional indentation.
    If group is shown with a collapsable/expandable UI, it should be initially expanded.
  • console.groupCollapsed():
    Increases indentation of subsequent lines by two spaces.
    If group is shown with a collapsable/expandable UI, it should be initially collapsed.
  • console.groupEnd():
    Decreases indentation of subsequent lines by two spaces.

My DAP proposal:

We add an optional enum property group to the Output event with these values:

  • start: start a new group in expanded mode. Subsequent Output events are members of the group and should be shown indented. An optional output attribute becomes the name of the group and is not indented.
  • startCollapsed: start a new group in collapsed mode. Subsequent Output events are members of the group and should be shown indented (as soon as the group is expanded). An optional output attribute becomes the name of the group and is not indented.
  • end: end the current group and decreases the indentation of subsequent Output events.

/cc @isidorn @roblourens @connor4312

@weinand
Copy link
Contributor

weinand commented Feb 18, 2020

For further discussion of the group feature in the DAP, I've create this issue.

@weinand weinand removed their assignment Feb 18, 2020
@weinand
Copy link
Contributor

weinand commented Feb 18, 2020

@isidorn I've added a group attribute to DAP and updated debugProtocol.d.ts in VS Code.

@isidorn
Copy link
Contributor

isidorn commented Feb 20, 2020

I have pushed support for this.

@weinand thanks for adding the protocol!

@connor4312 @roblourens it would be very cool if the node2 or the javascript debugger could support console groups this milestone, then we could add a test plan item which uses node and not mock debug. Anyways let me know how it goes, so I prepare a test plan item in time. Thanks!

@connor4312
Copy link
Member

It would be a 💪 to do in the remaining days this iteration, but the test plan could literally for Rob or I to implement and verify grouping in js-debug. Andre has written a few items like that for me in the past 🙂

@isidorn
Copy link
Contributor

isidorn commented Feb 20, 2020

@connor4312 writing a test plan like that would work for me. Let's see the status on this tomorrow evening and I will update the test plan accordingly. Thanks!

@weinand
Copy link
Contributor

weinand commented Feb 20, 2020

Great idea to implement the protocol for js-debug as part of the test plan item ;-)

@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 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-testplan
Projects
None yet
Development

No branches or pull requests