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

Call native logging APIs when debugging in the extension host #104686

Closed
connor4312 opened this issue Aug 14, 2020 · 8 comments
Closed

Call native logging APIs when debugging in the extension host #104686

connor4312 opened this issue Aug 14, 2020 · 8 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@connor4312
Copy link
Member

Currently in extension host debugging, logged messages are placed directly in the Debug Console and bypass js-debug.

However, now (as of microsoft/vscode-js-debug#412) js-debug has the capability to replace paths stacktraces thrown from compiled code with their respective source locations, so there's a compelling benefit to change this behavior. Either to actually call the native APIs (which would be the easiest for me 😄) or implement some custom DAP message to pass the log through the debugger here

this.toDispose.push(this.extensionHostDebugService.onLogToSession(event => {

@connor4312 connor4312 added the debug Debug viewlet, configurations, breakpoints, adapter issues label Aug 14, 2020
@weinand weinand added this to the On Deck milestone Aug 14, 2020
@weinand weinand added the feature-request Request for new features or functionality label Aug 14, 2020
@weinand weinand assigned isidorn and unassigned weinand Nov 5, 2020
@weinand
Copy link
Contributor

weinand commented Nov 5, 2020

Since extension debugging is a private feature of VS Code, we can use any mechanism that works...

However, there is the DAPTracker API and we have LiveShare that uses the tracker to do their magic.
So we should always keep this in mind when bypassing anything.

@weinand weinand modified the milestones: On Deck, November 2020 Nov 5, 2020
@weinand weinand changed the title Call natiive logging APIs when debugging in the extension host Call native logging APIs when debugging in the extension host Nov 5, 2020
@isidorn
Copy link
Contributor

isidorn commented Nov 6, 2020

@connor4312 let me know you would like to tackle this with me? I am flexible.

@connor4312
Copy link
Member Author

Sure. I'm available next week, feel free to send an invite or grab me whenever we're both online.

@connor4312
Copy link
Member Author

Isi and I looked at this on a call this morning. The best way to deal with, among other alternatives, would be to have the extension host call the native methods, as in the title. This would also, as opposed to a custom dap message, allow us to expand and inspect logged objects link we normally would. It would also let us filter out messages that don't come from your extension, which I would personally love to have, since we get a stacktrace in CDP log messages.

The log messages are replaced in bootstrap-fork:

if (process.env.VERBOSE_LOGGING === 'true') {
console.log = function () { safeSend({ type: '__$console', severity: 'log', arguments: safeToArray(arguments) }); };
console.info = function () { safeSend({ type: '__$console', severity: 'log', arguments: safeToArray(arguments) }); };
console.warn = function () { safeSend({ type: '__$console', severity: 'warn', arguments: safeToArray(arguments) }); };
} else {
console.log = function () { /* ignore */ };
console.warn = function () { /* ignore */ };
console.info = function () { /* ignore */ };
}

I've made these call native APIs in the linked commit. However, there's also manual handling of output, which results in duplicate log messages being output.

It seems like @joaomoreno authored this code, a long time ago. I'm not sure what the best approach is. Ideally we could call the console log method and avoid writing to stdout, but that doesn't seem possible. You can make a new Console, but this does not result in the required CDP methods firing. Maybe we write a "marker" byte around the console messages so that the output handling skips them?

@weinand
Copy link
Contributor

weinand commented Nov 24, 2020

I like the approach to call the native methods (and the "marker byte" workaround sounds OK to me).

In general we should strive for minimising the amount of custom code for extension debugging.

@isidorn
Copy link
Contributor

isidorn commented Nov 24, 2020

I also like this approach! Nice work @connor4312
I can do a test pass once we wrap this up.

@isidorn isidorn added the verification-needed Verification of issue is requested label Dec 9, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2021
@connor4312
Copy link
Member Author

ping @isidorn if you wanted to verify this 🙂

@isidorn isidorn added the verified Verification succeeded label Jan 27, 2021
@isidorn
Copy link
Contributor

isidorn commented Jan 27, 2021

Thanks for the ping, I have verified that this works fine.

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 insiders-released Patch has been released in VS Code Insiders verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants
@weinand @isidorn @connor4312 and others