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

rawLog option for OutputChannel to colourise the Ark log #1589

Merged
merged 2 commits into from
Oct 19, 2023

Conversation

lionel-
Copy link
Contributor

@lionel- lionel- commented Oct 16, 2023

I would like to have colourised output in our Ark log because it is currently hard to read. VS Code doesn't support ansi escapes in output channels (see wontfix issue microsoft/vscode#571) but it does colourise output channels that are created with log: true.

We can't do the latter option however because that automatically adds timestamps (which we already add elsewhere) and misleading [info] tags. I found a way to prevent VS Code from adding these with a new option. Using this, the Ark output becomes much more readable:

Screen.Recording.2023-10-16.at.15.18.50.mov

However this requires changing a bunch of VS Code signatures to thread the option in. Is this too much @jmcphers?

@lionel- lionel- marked this pull request as draft October 16, 2023 13:38
@@ -10763,7 +10763,7 @@ declare module 'vscode' {
* @param options Options for the log output channel.
* @returns A new log output channel.
*/
export function createOutputChannel(name: string, options: { /** literal-type defines return type */log: true }): LogOutputChannel;
export function createOutputChannel(name: string, options: { /** literal-type defines return type */log: true, rawLog?: boolean }): LogOutputChannel;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid modifying the VS Code API definitions. If it's Positron-specific, it should go in positron.d.ts. For this case I think we could make a scoped API, something like createLogChannel (i.e. rather than creating a superset of VS Code's API, we create an API whose use doesn't overlap with createOutputChannel).

It's okay to co-opt VS Code's internals to plumb additional flags although there's some tension between DRY vs. not wanting to create a lot of opportunities for merge conflicts.

Copy link
Contributor Author

@lionel- lionel- Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made these two changes:

  • A new createRawLogOutputChannel() function is created in vscode.window but not publicised to the API file. I created it in the vscode namespace rather than positron because we don't have access to extHostOutputService from the latter namespace. Since it's hidden, using it requires a @ts-ignore tag, which I thought is ok here.

  • I changed the approach to use more wrapping of classes and injected methods to avoid changing existing code. This should make merging easy.

@lionel- lionel- force-pushed the feature/colour-log branch from 0dd4b02 to 24613fd Compare October 18, 2023 16:12
@lionel- lionel- marked this pull request as ready for review October 18, 2023 16:12
@lionel- lionel- requested a review from jmcphers October 18, 2023 16:13
Copy link
Collaborator

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created it in the vscode namespace rather than positron because we don't have access to extHostOutputService from the latter namespace

We do! I punched a little hole for exactly this use case and we use it in a couple places already. Here's the relevant bit:

// Retrieve the raw `ExtHostWebViews` object from the rpcProtocol; this
// object is needed to create webviews, and was previously created in
// `createApiFactoryAndRegisterActors` when VS Code's API factory was
// created earlier.
//
// The `getRaw` method is a Positron extension to the `rpcProtocol` that
// allows us to retrieve the raw actor object so that the Positron API and
// VS Code API can share a single instance of `ExtHostWebViews`, which is
// necessary since the instance effectively needs to be a singleton.
const extHostWebviews: ExtHostWebviews = rpcProtocol.getRaw(ExtHostContext.ExtHostWebviews);
if (!extHostWebviews) {
throw new Error('Could not retrieve ExtHostWebviews from the RPC protocol. ' +
' The VS Code API must be created before the Positron API.');
}

So you can add a getRaw accessor, which will allow you to add this as an exposed Positron API instead of a hidden VS Code API (and should also eliminate the need for ts-ignore).

@lionel-
Copy link
Contributor Author

lionel- commented Oct 19, 2023

oh neat, I've now moved the function to the positron namespace

@@ -45,10 +46,7 @@ export function createPositronApiFactoryAndRegisterActors(accessor: ServicesAcce
// VS Code API can share a single instance of `ExtHostWebViews`, which is
// necessary since the instance effectively needs to be a singleton.
const extHostWebviews: ExtHostWebviews = rpcProtocol.getRaw(ExtHostContext.ExtHostWebviews);
if (!extHostWebviews) {
throw new Error('Could not retrieve ExtHostWebviews from the RPC protocol. ' +
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since getRaw() already throws, I've removed this error and merged the extra message info in the upstream error. This way we don't have too much redundant error handling in all the getRaw() calls.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's nicer, thanks!

@lionel- lionel- requested a review from jmcphers October 19, 2023 20:17
Copy link
Collaborator

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@lionel- lionel- merged commit 595818f into main Oct 19, 2023
1 check passed
@lionel- lionel- deleted the feature/colour-log branch October 19, 2023 21:12
juliasilge added a commit that referenced this pull request Sep 30, 2024
Addresses #4275 

This PR makes changes to all the output channels generated by the
`jupyter-adapter` extension.

The "main" Jupyter Adapter output channel is now a real log, with
timestamps and log level annotations:

![Screenshot 2024-09-28 at 4 51
07 PM](https://github.com/user-attachments/assets/28c48414-f7ee-4da5-9ebb-eeaa96d892ac)

The existing "Console: X.Y.Z" channel now only has messages generated
from Positron, not the kernel itself. This is now also a real log and is
the part that directly addresses #4275:

![Screenshot 2024-09-28 at 4 51
26 PM](https://github.com/user-attachments/assets/9e1b1345-4001-44d6-b43a-d2b9e092027c)

Notice the new "error" log level here; you can now set the log level in
these first two output channels and it will update moving forward.

These first two output channels are generated in the same way most of
the time (we write to the "Console" channel if that kernel is up but to
the "Jupyter Adapter" channel if not) so we have to make changes to them
together; this seems good rather than bad to me and overall both are now
improved.

There is a new "Kernel: X.Y.Z" channel which still uses the raw output
channel that we created back in #1589. It is not a real log and does not
have timestamps, unless the kernel log generated those. You cannot
change the log level via the normal method for this one:

![Screenshot 2024-09-28 at 4 51
38 PM](https://github.com/user-attachments/assets/2df12fb3-7cc5-4aab-b49b-933fc3ecdc7a)

It is kind of a bummer to now have _three_ output channels generated
from this one extension but IMO the tradeoffs are worth it.


### QA Notes

You'll now see timestamps and log level annotations in the "Jupyter
Adapter" and "Console: X.Y.Z" output channels, and there is a new
"Kernel: X.Y.Z" channel.
isabelizimm pushed a commit that referenced this pull request Oct 16, 2024
Addresses #4275 

This PR makes changes to all the output channels generated by the
`jupyter-adapter` extension.

The "main" Jupyter Adapter output channel is now a real log, with
timestamps and log level annotations:

![Screenshot 2024-09-28 at 4 51
07 PM](https://github.com/user-attachments/assets/28c48414-f7ee-4da5-9ebb-eeaa96d892ac)

The existing "Console: X.Y.Z" channel now only has messages generated
from Positron, not the kernel itself. This is now also a real log and is
the part that directly addresses #4275:

![Screenshot 2024-09-28 at 4 51
26 PM](https://github.com/user-attachments/assets/9e1b1345-4001-44d6-b43a-d2b9e092027c)

Notice the new "error" log level here; you can now set the log level in
these first two output channels and it will update moving forward.

These first two output channels are generated in the same way most of
the time (we write to the "Console" channel if that kernel is up but to
the "Jupyter Adapter" channel if not) so we have to make changes to them
together; this seems good rather than bad to me and overall both are now
improved.

There is a new "Kernel: X.Y.Z" channel which still uses the raw output
channel that we created back in #1589. It is not a real log and does not
have timestamps, unless the kernel log generated those. You cannot
change the log level via the normal method for this one:

![Screenshot 2024-09-28 at 4 51
38 PM](https://github.com/user-attachments/assets/2df12fb3-7cc5-4aab-b49b-933fc3ecdc7a)

It is kind of a bummer to now have _three_ output channels generated
from this one extension but IMO the tradeoffs are worth it.


### QA Notes

You'll now see timestamps and log level annotations in the "Jupyter
Adapter" and "Console: X.Y.Z" output channels, and there is a new
"Kernel: X.Y.Z" channel.
isabelizimm pushed a commit that referenced this pull request Oct 16, 2024
Addresses #4275 

This PR makes changes to all the output channels generated by the
`jupyter-adapter` extension.

The "main" Jupyter Adapter output channel is now a real log, with
timestamps and log level annotations:

![Screenshot 2024-09-28 at 4 51
07 PM](https://github.com/user-attachments/assets/28c48414-f7ee-4da5-9ebb-eeaa96d892ac)

The existing "Console: X.Y.Z" channel now only has messages generated
from Positron, not the kernel itself. This is now also a real log and is
the part that directly addresses #4275:

![Screenshot 2024-09-28 at 4 51
26 PM](https://github.com/user-attachments/assets/9e1b1345-4001-44d6-b43a-d2b9e092027c)

Notice the new "error" log level here; you can now set the log level in
these first two output channels and it will update moving forward.

These first two output channels are generated in the same way most of
the time (we write to the "Console" channel if that kernel is up but to
the "Jupyter Adapter" channel if not) so we have to make changes to them
together; this seems good rather than bad to me and overall both are now
improved.

There is a new "Kernel: X.Y.Z" channel which still uses the raw output
channel that we created back in #1589. It is not a real log and does not
have timestamps, unless the kernel log generated those. You cannot
change the log level via the normal method for this one:

![Screenshot 2024-09-28 at 4 51
38 PM](https://github.com/user-attachments/assets/2df12fb3-7cc5-4aab-b49b-933fc3ecdc7a)

It is kind of a bummer to now have _three_ output channels generated
from this one extension but IMO the tradeoffs are worth it.


### QA Notes

You'll now see timestamps and log level annotations in the "Jupyter
Adapter" and "Console: X.Y.Z" output channels, and there is a new
"Kernel: X.Y.Z" channel.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants