microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.71k stars 29.08k forks source link

Remove LogLevel api #85992

Closed roblourens closed 3 years ago

roblourens commented 4 years ago

We have this API related to the LogLevel:

https://github.com/microsoft/vscode/blob/cd6c734313c0edf305de238e00a7cb99e913f2cc/src/vs/vscode.proposed.d.ts#L732-L757

I didn't even realize it was still proposed. I have used it in the SSH extension (sort of, now I ignore it and always log at "trace"). I think it hasn't changed in at least a year. It looks like git uses it.

Assuming that it's still useful, we should finalize this.

roblourens commented 4 years ago

In the API call we discussed whether this actually useful. On one hand, it seems that many extensions have reinvented the same thing driven by their own setting. This is a pretty simple API that could save some effort on the part of extension authors.

On the other hand, I am not aware of any request for this that has actually come from the community. It's not that hard to add a setting to set the log level per extension, and there are advantages to controlling logging in a more granular way anyway.

@eamodio, I see that gitlens has a setting for this, would you consider respecting vscode's logLevel instead or would you not use it?

@chrmarti, I see that the remote containers extension uses this, do you have any thoughts on this vs contributing a setting for it?

chrmarti commented 4 years ago

@roblourens It's certainly convenient, but it would become more useful if it also had a log object to pass messages to that would then show up in an Output channel and be stored in a log file at the extension's log location. Then we could also have generic UI to open a log file.

I agree the log level alone is easy to replicate.

eamodio commented 4 years ago

@roblourens I'm really not sure -- but I don't think so. On one hand it is nice for users to only need to toggle one setting, but at the same time, do we really want logging toggled on for everything because I need GitLens logs? Also, GitLens in debug/verbose levels logs A LOT and I would be worried about perf impact if 1 toggle causing everything to log like mad.

IMO, it would be really nice if there was a consistent UX for a user to turn on logging, collect logs, and unified log levels on a case-by-case basis, that extensions can use. Like @chrmarti is suggesting -- maybe a LogProvider (or the like) with unified levels, where we can wrap UX around the registration, etc.

roblourens commented 4 years ago

Discussed this during standup today. We did go down the route of providing a real structured logger object once, and I don't want to tackle that again. We discussed how to give a consistent per-extension logging enabling experience like @eamodio mentions but that is a big piece of work to tackle, and every extension will have edge cases that don't fit into the general model. Kai suggested that looking at a logging UX might make more sense when revisiting the extension pages.

Anyway for the question of what to do with this piece of API, I think we should just get rid of it.

roblourens commented 4 years ago

We will get rid of this API. I'm only aware of these consumers.

Please fix these, or I would be happy to fix them for you if you let me know what you want them to be replaced with, I assume a setting.

I will leave the actual implementation in place for several months to not break the extensions in the wild.

joaomoreno commented 4 years ago

@roblourens So the idea is to have a git specific log level, in git's case?

roblourens commented 4 years ago

Yeah

bd82 commented 4 years ago

@roblourens

In the API call we discussed whether this actually useful. On one hand, it seems that many extensions have reinvented the same thing driven by their own setting. This is a pretty simple API that could save some effort on the part of extension authors.

@chrmarti

It's certainly convenient, but it would become more useful if it also had a log object to pass messages to that would then show up in an Output channel and be stored in a log file at the extension's log location.


Hello, I've created a small (opinionated) logging library for VSCode.

After first attempting to contribute a Logging Example to VScode-Examples

While this may be less relevant to the logLevel API question, it may be relevant to general discussions on VSCode and Logging.

Feedback is always welcome 😄

Cheers. Shahar.

joaomoreno commented 4 years ago

Removed git's usage.

roblourens commented 4 years ago

Thanks @joaomoreno!

roblourens commented 4 years ago

Do you plan to look at this @chrmarti or should I?

chrmarti commented 4 years ago

I have planned doing this.

chrmarti commented 4 years ago

Done. Back to you @roblourens.

roblourens commented 4 years ago

Thank you! I will leave the implementation in place for a few months or until I think of it then delete it.

chrmarti commented 4 years ago

@roblourens Maybe remove it from the proposed API (while keeping the implementation).

roblourens commented 4 years ago

Yes, done

roblourens commented 4 years ago

Eh nvm, I don't want to add confusing code to exthost.api.impl to trick TS to include properties that aren't in the d.ts. I added deprecation tags.

jrieken commented 3 years ago

@roblourens This has now happened, the old proposal is gone. Added feature-request so that this get documented

alexr00 commented 3 years ago

Verified that the API is gone and I can't find lingering traces of it.