Support dynamic updates of the log level #139
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DavisVaughan
commented
Jan 9, 2025
Comment on lines
+165
to
+211
| /// Log state managed by the LSP | ||
| /// | ||
| /// This state allows for dynamically updating the log level at runtime using [`reload`]. | ||
| /// | ||
| /// It works using an `RwLock` that's only ever locked by us inside `self.handle.reload()`, | ||
| /// this seems to be fast enough even though atomics are checked at every log call site. | ||
| /// Notably we only lock if the new log levels are actually different, which is important | ||
| /// since we call [`reload`] when ANY configuration changes. | ||
| pub(crate) struct LogState { | ||
| handle: LogReloadHandle, | ||
|
|
||
| /// The log level as provided by the client, before any extra processing is done. | ||
| /// Used to check if an update is required. | ||
| log_level: Option<LogLevel>, | ||
|
|
||
| /// The dependency log levels as provided by the client, before any extra processing is done | ||
| /// Used to check if an update is required. | ||
| dependency_log_levels: Option<String>, | ||
| } | ||
|
|
||
| impl LogState { | ||
| pub(crate) fn reload( | ||
| &mut self, | ||
| log_level: Option<LogLevel>, | ||
| dependency_log_levels: Option<String>, | ||
| ) { | ||
| if (self.log_level == log_level) && (self.dependency_log_levels == dependency_log_levels) { | ||
| // Nothing changed | ||
| return; | ||
| } | ||
|
|
||
| let (filter, message) = log_filter(log_level, dependency_log_levels.clone()); | ||
|
|
||
| match self.handle.reload(filter) { | ||
| Ok(()) => { | ||
| // Update to match the new filter | ||
| tracing::info!("{message}"); | ||
| self.log_level = log_level; | ||
| self.dependency_log_levels = dependency_log_levels; | ||
| } | ||
| Err(error) => { | ||
| // Log and return without updating our internal log level | ||
| tracing::error!("Failed to update log level: {error}"); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the core bit of it, pretty nice!
Comment on lines
+170
to
+171
| /// State used to dynamically update the log level | ||
| pub(crate) log_state: Option<logging::LogState>, |
There was a problem hiding this comment.
I'm excited to move to lsp-server because I think the startup procedure there will result in us creating GlobalState / LspState after we've received InitializeParams, meaning we will be able to get rid of Option<>s here and in GlobalState
Comment on lines
-235
to
+240
| let (log_state, log_tx) = LogState::new(client.clone()); | ||
| let (log_thread_state, log_tx) = LogThreadState::new(client.clone()); |
There was a problem hiding this comment.
Just a rename so we can use LogState for the bit that holds the reload handle
This thread state thing will also eventually go away in lsp-server world
lionel-
approved these changes
Jan 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #134
dynamic.mov