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

Streamline logging API #14861

Merged
merged 4 commits into from
Feb 11, 2025
Merged

Streamline logging API #14861

merged 4 commits into from
Feb 11, 2025

Conversation

msujew
Copy link
Member

@msujew msujew commented Feb 6, 2025

What it does

Fixes #14858

This change fixes a bunch of issues that have been bugging me about the logging API. Due to the previous way of how the binding was done for the @named decorator, creating a dedicated logger was quite a lot of work (at least compared to the new binding logic). Now, named loggers are simply created on the fly.

Also adds a new log-file argument to the CLI. This isn't strictly necessary, as operating systems offer ways of streaming the output of a executable into files, but I find this to be pretty useful anyways, even if just for discoverability.

How to test

  1. Ensure that named loggers still work as expected and they are correctly generated on the fly.
  2. Use the new log-file CLI argument to see whether this works as expected.
  3. Ensure that Log config file watching does not longer work #14858 has been fixed by following the repro instructions.

Review checklist

Reminder for reviewers

@msujew msujew added logging issues related to logging file-watchers issues related to filesystem watchers - nsfw labels Feb 6, 2025
@msujew msujew requested a review from sdirix February 6, 2025 12:28
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

I have tried it, and it worked very well for me. Thanks

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

Works for me!

I can confirm that the log config works again, the log-file and log-file renaming works too. Also logging works in general ;)

Note that this is a breaking change because if adopters followed the Theia conventions for creating their loggers, they will now face errors like this

Uncaught Exception: Error: Ambiguous match found for serviceIdentifier: Symbol(ILogger)
Error: Ambiguous match found for serviceIdentifier: Symbol(ILogger)

So we should mention that in the changelog I think

@msujew
Copy link
Member Author

msujew commented Feb 11, 2025

@sdirix Thank you for the hint. I didn't think about that. I added a breaking changelog entry for that :)

@msujew msujew merged commit 81b0404 into master Feb 11, 2025
9 of 11 checks passed
@msujew msujew deleted the msujew/logging branch February 11, 2025 11:25
@github-actions github-actions bot added this to the 1.59.0 milestone Feb 11, 2025
@yduuuuuuuu
Copy link

yduuuuuuuu commented Mar 12, 2025

Hello! Does it work for the package as well ? Is there any way for us to get the log file if we run the .exe ?

@msujew
Copy link
Member Author

msujew commented Mar 12, 2025

Hello! Does it work for the package as well ? Is there any way for us to get the log file if we run the .exe ?

@yduuuuuuuu Yes, you just need to start the .exe with the --log-file=./log.txt CLI argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
file-watchers issues related to filesystem watchers - nsfw logging issues related to logging
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Log config file watching does not longer work
5 participants