-
Notifications
You must be signed in to change notification settings - Fork 699
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
fmt: Add support for runtime filter reloading #45
Conversation
This is a draft currently as I have yet to factor out the portion of this change that requires the subscriber downcasting API. I figured I'd ptu it up now as a preview. |
398bb41
to
adad266
Compare
@LucioFranco I've edited this branch to remove the portion of the change that requires downcasting support to land, so this can now be merged into the current master. Thus this branch is now ready for a proper review. I've separated the changes that require downcasting and will propose them in a follow-up branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!!! Thank you so much!!!
} | ||
} | ||
|
||
impl<F: 'static> ReloadFilter<F> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im not totally following why this needs 'static
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LucioFranco it's because of the 'static
bound on the filter type on the impl of Subscriber
for FmtSubscriber
:
https://github.com/tokio-rs/tokio-trace-nursery/blob/462a08fb6ca64db3944689ecafdae0107c6982c7/tokio-trace-fmt/src/lib.rs#L83
this is because implementors of Subscriber
have to be 'static
. So a non-'static
filter wouldn't actually be usable.
In retrospect, the 'static
bound should probably just be on the Filter
trait...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that makes sense, it also makes sense to move it lower into the Filter trait.
167f284
to
8c7fd7b
Compare
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
f166267
to
11158c6
Compare
Build failure is due to an unrelated upstream change --- c993cca should fix that. |
This branch adds support for runtime filter reloading to `tokio-trace-fmt`. Filters may be reloaded through a `Handle` type that can be constructed when the subscriber or filter are constructed. In the future, when PR #44 merges, it will also be possible to add a free function `record_current` that downcasts and reloads the current subscriber. Currently, when using a reloadable filter, `Interest` is never cached. When tokio-rs/tokio#1039 is merged, we can invalidate the callsite cache when reloading, allowing us to cache interests. Closes #42 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
This branch adds support for runtime filter reloading to
tokio-trace-fmt
. Filters may be reloaded through aHandle
type that can be constructed when the subscriber or filter are
constructed.
In the future, when PR #44 merges, it will also be possible to add a
free function
record_current
that downcasts and reloads the currentsubscriber.
Currently, when using a reloadable filter,
Interest
is never cached.When tokio-rs/tokio#1039 is merged, we can invalidate the callsite cache
when reloading, allowing us to cache interests.
Closes #42
Signed-off-by: Eliza Weisman eliza@buoyant.io