-
Notifications
You must be signed in to change notification settings - Fork 715
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
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: