-
Notifications
You must be signed in to change notification settings - Fork 755
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
Panic after reloading layer, combined with .with_filter
#1629
Comments
Thanks for the report, I'll take a look at this! |
This issue occurs because, when a Unfortunately, we cannot easily change |
An alternative approach is to add APIs to the This approach isn't a breaking change. It would mean, though, that we should document clearly that |
I had the same issue, and eventually used a different approach without per-layer filters: I added the level filter as separate reloadable layer, in order to control the global log level dynamically, and made a separate reloadable layer which chains a few different let (level, level_handle) = reload::Layer::new(LevelFilter::from_level(default_level));
let (target, target_handle) = reload::Layer::new(make_target_layer(default_target).unwrap());
let subscriber = Registry::default()
.with(env_filter)
.with(level)
.with(target);
tracing::subscriber::set_global_default(subscriber).unwrap(); |
The workaround from the OP (using I have a custom type I've tried several permutations and it seems this is currently not possible because of this bug, but maybe I've missed something and there are possible workarounds available on the currently released version? I can confirm that a Enumerating the different ways I combined the two layers: When using
( When using Are there other workarounds? |
## Motivation Changing the filter of a Filtered at runtime is currently only possible by replacing it with a new Filtered via the reload::Handle's reload method. This currently doesn't work (see tokio-rs#1629). While it would be desirable to just make that work, it would only be possible via a breaking change (according to Eliza). Making it possible to change the filter via the handle's modify method and mutating the inner filter is an easy workaround.
## Motivation Changing the filter of a Filtered at runtime is currently only possible by replacing it with a new Filtered via the reload::Handle's reload method. This currently doesn't work (see tokio-rs#1629). While it would be desirable to just make that work, it would only be possible via a breaking change (according to Eliza). Making it possible to change the filter via the handle's modify method and mutating the inner filter is an easy workaround.
## Motivation Changing the filter of a Filtered at runtime is currently only possible by replacing it with a new Filtered via the reload::Handle's reload method. This currently doesn't work (see tokio-rs#1629). While it would be desirable to just make that work, it would only be possible via a breaking change (according to Eliza). Making it possible to change the filter via the handle's modify method and mutating the inner filter is an easy workaround.
## Motivation Changing the filter of a Filtered at runtime is currently only possible by replacing it with a new Filtered via the reload::Handle's reload method. This currently doesn't work (see tokio-rs#1629). While it would be desirable to just make that work, it would only be possible via a breaking change (according to Eliza). Making it possible to change the filter via the handle's modify method and mutating the inner filter is an easy workaround.
## Motivation Changing the filter of a Filtered at runtime is currently only possible by replacing it with a new Filtered via the reload::Handle's reload method. This currently doesn't work (see tokio-rs#1629). While it would be desirable to just make that work, it would only be possible via a breaking change (according to Eliza). Making it possible to change the filter via the handle's modify method and mutating the inner filter is an easy workaround.
Partially fixes #1629 (I think making `reload::Handle::reload` work with `Filtered` would be cleaner, but this approach seemed easier to me) I assumed opening the PR against v0.1.x is correct as I couldn't find the `Filtered` type in master. I think it'd be sensible to note the fact that `reload::Handle::reload` doesn't work with `Filtered` in the docs somewhere, should I add that? ## Motivation Changing the filter of a `Filtered` at runtime is currently only possible by replacing it with a new `Filtered` via the `reload::Handle::reload` method. This currently doesn't work as the new `Filtered` won't receive a `FilterId` (see #1629). While it would be desirable to just make that work, it would only be possible via a breaking change (according to Eliza) so this seems like a reasonable (and easy) workaround for now. (I can't judge whether this method is only useful as a workaround for the bug or if it suits the public API independently) ## Solution Offer mutable access to the `Filtered::filter` field in the public API. This can be used via the `reload::Handle::modify` method to change the filter inside the existing `Filtered`. Fixes #1629
Partially fixes #1629 (I think making `reload::Handle::reload` work with `Filtered` would be cleaner, but this approach seemed easier to me) I assumed opening the PR against v0.1.x is correct as I couldn't find the `Filtered` type in master. I think it'd be sensible to note the fact that `reload::Handle::reload` doesn't work with `Filtered` in the docs somewhere, should I add that? ## Motivation Changing the filter of a `Filtered` at runtime is currently only possible by replacing it with a new `Filtered` via the `reload::Handle::reload` method. This currently doesn't work as the new `Filtered` won't receive a `FilterId` (see #1629). While it would be desirable to just make that work, it would only be possible via a breaking change (according to Eliza) so this seems like a reasonable (and easy) workaround for now. (I can't judge whether this method is only useful as a workaround for the bug or if it suits the public API independently) ## Solution Offer mutable access to the `Filtered::filter` field in the public API. This can be used via the `reload::Handle::modify` method to change the filter inside the existing `Filtered`. Fixes #1629
Partially fixes #1629 (I think making `reload::Handle::reload` work with `Filtered` would be cleaner, but this approach seemed easier to me) I assumed opening the PR against v0.1.x is correct as I couldn't find the `Filtered` type in master. I think it'd be sensible to note the fact that `reload::Handle::reload` doesn't work with `Filtered` in the docs somewhere, should I add that? ## Motivation Changing the filter of a `Filtered` at runtime is currently only possible by replacing it with a new `Filtered` via the `reload::Handle::reload` method. This currently doesn't work as the new `Filtered` won't receive a `FilterId` (see #1629). While it would be desirable to just make that work, it would only be possible via a breaking change (according to Eliza) so this seems like a reasonable (and easy) workaround for now. (I can't judge whether this method is only useful as a workaround for the bug or if it suits the public API independently) ## Solution Offer mutable access to the `Filtered::filter` field in the public API. This can be used via the `reload::Handle::modify` method to change the filter inside the existing `Filtered`. Fixes #1629
I have a feeling what I want to do doesn't presently have a workaround but since there are proposed changes to support this (#2101) I thought I'd throw my hat in the ring. I am trying to permit a client to dynamically trace (with runtime-created filtering) an in-progress service over a local socket. This works perfectly and I can receive log messages over my socket however I encounter panics when I to introduce server-side filtering where I encounter the same problems. I currently have a Is there something I have missed or am I stuck waiting until the proposed atomic |
Partially fixes tokio-rs#1629 (I think making `reload::Handle::reload` work with `Filtered` would be cleaner, but this approach seemed easier to me) I assumed opening the PR against v0.1.x is correct as I couldn't find the `Filtered` type in master. I think it'd be sensible to note the fact that `reload::Handle::reload` doesn't work with `Filtered` in the docs somewhere, should I add that? ## Motivation Changing the filter of a `Filtered` at runtime is currently only possible by replacing it with a new `Filtered` via the `reload::Handle::reload` method. This currently doesn't work as the new `Filtered` won't receive a `FilterId` (see tokio-rs#1629). While it would be desirable to just make that work, it would only be possible via a breaking change (according to Eliza) so this seems like a reasonable (and easy) workaround for now. (I can't judge whether this method is only useful as a workaround for the bug or if it suits the public API independently) ## Solution Offer mutable access to the `Filtered::filter` field in the public API. This can be used via the `reload::Handle::modify` method to change the filter inside the existing `Filtered`. Fixes tokio-rs#1629
- Removed `reload_config` b/c it's been deprecated. There are code patterns like `writer.lock().unwrap_or_else(...)` which ignore `PoisonError`, assuming that the log writer should not cause a panic even if another thread has poisoned the lock. If log_dir changes by updating config, logs should be saved in that directory. This commit implements this by wrapping the file writer. Alternatively, `tracing_subscriber::reload` could be used, but it cannot handle cases where the directory is added or removed (as it requires reassigning the writer itself, which is not possible due to internal limitations). Related issue: [tokio-rs/tracing #1629](tokio-rs/tracing#1629)
- Removed `reload_config` b/c it's been deprecated. There are code patterns like `writer.lock().unwrap_or_else(...)` which ignore `PoisonError`, assuming that the log writer should not cause a panic even if another thread has poisoned the lock. If log_dir changes by updating config, logs should be saved in that directory. This commit implements this by wrapping the file writer. Alternatively, `tracing_subscriber::reload` could be used, but it cannot handle cases where the directory is added or removed (as it requires reassigning the writer itself, which is not possible due to internal limitations). Related issue: tokio-rs/tracing#1629
- Removed `reload_config` b/c it's been deprecated. There are code patterns like `writer.lock().unwrap_or_else(...)` which ignore `PoisonError`, assuming that the log writer should not cause a panic even if another thread has poisoned the lock. If `log_dir` changes by updating config, logs should be saved in that directory. This commit implements this by wrapping the file writer. Alternatively, `tracing_subscriber::reload` could be used, but it cannot handle cases where the directory is added or removed (as it requires reassigning the writer itself, which is not possible due to internal limitations). Related issue: tokio-rs/tracing#1629
- Removed `reload_config` b/c it's been deprecated. There are code patterns like `writer.lock().unwrap_or_else(...)` which ignore `PoisonError`, assuming that the log writer should not cause a panic even if another thread has poisoned the lock. If `log_dir` changes by updating config, logs should be saved in that directory. This commit implements this by wrapping the file writer. Alternatively, `tracing_subscriber::reload` could be used, but it cannot handle cases where the directory is added or removed (as it requires reassigning the writer itself, which is not possible due to internal limitations). Related issue: tokio-rs/tracing#1629 Close: #112
- Removed `reload_config` b/c it's been deprecated. There are code patterns like `writer.lock().unwrap_or_else(...)` which ignore `PoisonError`, assuming that the log writer should not cause a panic even if another thread has poisoned the lock. If `log_dir` changes by updating config, logs should be saved in that directory. This commit implements this by wrapping the file writer. Alternatively, `tracing_subscriber::reload` could be used, but it cannot handle cases where the directory is added or removed (as it requires reassigning the writer itself, which is not possible due to internal limitations). Related issue: tokio-rs/tracing#1629 Close: #112
- Removed `reload_config` b/c it's been deprecated. There are code patterns like `writer.lock().unwrap_or_else(...)` which ignore `PoisonError`, assuming that the log writer should not cause a panic even if another thread has poisoned the lock. If `log_dir` changes by updating config, logs should be saved in that directory. This commit implements this by wrapping the file writer. Alternatively, `tracing_subscriber::reload` could be used, but it cannot handle cases where the directory is added or removed (as it requires reassigning the writer itself, which is not possible due to internal limitations). Related issue: tokio-rs/tracing#1629 Close: #112
- Removed `reload_config` b/c it's been deprecated. There are code patterns like `writer.lock().unwrap_or_else(...)` which ignore `PoisonError`, assuming that the log writer should not cause a panic even if another thread has poisoned the lock. If `log_dir` changes by updating config, logs should be saved in that directory. This commit implements this by wrapping the file writer. Alternatively, `tracing_subscriber::reload` could be used, but it cannot handle cases where the directory is added or removed (as it requires reassigning the writer itself, which is not possible due to internal limitations). Related issue: tokio-rs/tracing#1629 Close: #112
- Removed `reload_config` b/c it's been deprecated. There are code patterns like `writer.lock().unwrap_or_else(...)` which ignore `PoisonError`, assuming that the log writer should not cause a panic even if another thread has poisoned the lock. If `log_dir` changes by updating config, logs should be saved in that directory. This commit implements this by wrapping the file writer. Alternatively, `tracing_subscriber::reload` could be used, but it cannot handle cases where the directory is added or removed (as it requires reassigning the writer itself, which is not possible due to internal limitations). Related issue: tokio-rs/tracing#1629 Close: #112
- Removed `reload_config` b/c it's been deprecated. There are code patterns like `writer.lock().unwrap_or_else(...)` which ignore `PoisonError`, assuming that the log writer should not cause a panic even if another thread has poisoned the lock. If `log_dir` changes by updating config, logs should be saved in that directory. This commit implements this by wrapping the file writer. Alternatively, `tracing_subscriber::reload` could be used, but it cannot handle cases where the directory is added or removed (as it requires reassigning the writer itself, which is not possible due to internal limitations). Related issue: tokio-rs/tracing#1629 Close: #112
As of 2025, this issue is still unresolved. Has there been any progress on resolving this architectural limitation? |
Bug Report
Version
Platform
Description
When layer, wrapped in
reload::Layer
, have a filter, set with.with_filter
(my_layer.with_filter(my_filter)
), it will panic on next call toevent!
(info!
in my case). But if layer built asmy_filter.and_then(my_layer)
, everything works:I am not sure, whether both implementations of
reloadable_layer_fn
equal or not, but they both seems work in this case.I expected:
After reload
is printedInstead, this happened: panic on printing
After reload
The text was updated successfully, but these errors were encountered: