subscriber: fix missing register_callsite for Box<dyn Subscribe>
#2028
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
Turns out the `Subscribe` impl for `Box<dyn Subscribe<...> + ...>` never called the subscriber trait object's `register_callsite` method. Whoops! This commit adds the missing `register_callsite` hook.
davidbarsky
approved these changes
Mar 27, 2022
hawkw
added a commit
that referenced
this pull request
Mar 29, 2022
Depends on #2028 ## Motivation In many cases, it is desirable to have a variable number of subscribers at runtime (such as when reading a logging configuration from a config file). The current approach, where types implementing `Subscribe` are composed at the type level into `Layered` subscribers, doesn't work well when the number of subscribers varies at runtime. ## Solution To solve this, this branch adds a `Subscribe` implementation for `Vec<S> where S: Subscribe`. This allows variable-length lists of subscribers to be added to a collector. Although the impl for `Vec<S>` requires all the subscribers to be the same type, it can also be used in conjunction with `Box<dyn Subscribe<C> + ...>` trait objects to implement a variable-length list of subscribers of multiple types. I also wrote a bunch of docs examples. ## Notes Alternatively, we could have a separate type defined in `tracing-subscriber` for a variable-length list of type-erased subscribers. This would have one primary usability benefit, which is that we could have a `push` operation that takes an `impl Subscribe` and boxes it automatically. However, I thought the approach used here is nicer, since it's based on composing together existing primitives such as `Vec` and `Box`, rather than adding a whole new API. Additionally, it allows avoiding the additional `Box`ing in the case where the list consists of subscribers that are all the same type. Closes #1708 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Apr 1, 2022
Depends on #2028 ## Motivation In many cases, it is desirable to have a variable number of `Layer`s at runtime (such as when reading a logging configuration from a config file). The current approach, where types implementing `Layer` are composed at the type level into `Layered` layers, doesn't work well when the number of layers varies at runtime. ## Solution To solve this, this branch adds a `Layer` implementation for `Vec<L> where L: Layer`. This allows variable-length lists of layers to be added to a subscriber. Although the impl for `Vec<L>` requires all the layers to be the same type, it can also be used in onjunction with `Box<dyn Layer<S> + ...>` trait objects to implement a variable-length list of layers of multiple types. I also wrote a bunch of docs examples. ## Notes Alternatively, we could have a separate type defined in `tracing-subscriber` for a variable-length list of type-erased `Layer`s. This would have one primary usability benefit, which is that we could have a `push` operation that takes an `impl Layer` and boxes it automatically. However, I thought the approach used here is nicer, since it's based on composing together existing primitives such as `Vec` and `Box`, rather than adding a whole new API. Additionally, it allows avoiding the additional `Box`ing in the case where the list consists of layers that are all the same type. Closes #1708 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Apr 1, 2022
Depends on #2028 ## Motivation In many cases, it is desirable to have a variable number of `Layer`s at runtime (such as when reading a logging configuration from a config file). The current approach, where types implementing `Layer` are composed at the type level into `Layered` layers, doesn't work well when the number of layers varies at runtime. ## Solution To solve this, this branch adds a `Layer` implementation for `Vec<L> where L: Layer`. This allows variable-length lists of layers to be added to a subscriber. Although the impl for `Vec<L>` requires all the layers to be the same type, it can also be used in onjunction with `Box<dyn Layer<S> + ...>` trait objects to implement a variable-length list of layers of multiple types. I also wrote a bunch of docs examples. ## Notes Alternatively, we could have a separate type defined in `tracing-subscriber` for a variable-length list of type-erased `Layer`s. This would have one primary usability benefit, which is that we could have a `push` operation that takes an `impl Layer` and boxes it automatically. However, I thought the approach used here is nicer, since it's based on composing together existing primitives such as `Vec` and `Box`, rather than adding a whole new API. Additionally, it allows avoiding the additional `Box`ing in the case where the list consists of layers that are all the same type. Closes #1708 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Apr 1, 2022
Depends on #2028 ## Motivation In many cases, it is desirable to have a variable number of `Layer`s at runtime (such as when reading a logging configuration from a config file). The current approach, where types implementing `Layer` are composed at the type level into `Layered` layers, doesn't work well when the number of layers varies at runtime. ## Solution To solve this, this branch adds a `Layer` implementation for `Vec<L> where L: Layer`. This allows variable-length lists of layers to be added to a subscriber. Although the impl for `Vec<L>` requires all the layers to be the same type, it can also be used in onjunction with `Box<dyn Layer<S> + ...>` trait objects to implement a variable-length list of layers of multiple types. I also wrote a bunch of docs examples. ## Notes Alternatively, we could have a separate type defined in `tracing-subscriber` for a variable-length list of type-erased `Layer`s. This would have one primary usability benefit, which is that we could have a `push` operation that takes an `impl Layer` and boxes it automatically. However, I thought the approach used here is nicer, since it's based on composing together existing primitives such as `Vec` and `Box`, rather than adding a whole new API. Additionally, it allows avoiding the additional `Box`ing in the case where the list consists of layers that are all the same type. Closes #1708 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Apr 1, 2022
Depends on #2028 ## Motivation In many cases, it is desirable to have a variable number of `Layer`s at runtime (such as when reading a logging configuration from a config file). The current approach, where types implementing `Layer` are composed at the type level into `Layered` layers, doesn't work well when the number of layers varies at runtime. ## Solution To solve this, this branch adds a `Layer` implementation for `Vec<L> where L: Layer`. This allows variable-length lists of layers to be added to a subscriber. Although the impl for `Vec<L>` requires all the layers to be the same type, it can also be used in onjunction with `Box<dyn Layer<S> + ...>` trait objects to implement a variable-length list of layers of multiple types. I also wrote a bunch of docs examples. ## Notes Alternatively, we could have a separate type defined in `tracing-subscriber` for a variable-length list of type-erased `Layer`s. This would have one primary usability benefit, which is that we could have a `push` operation that takes an `impl Layer` and boxes it automatically. However, I thought the approach used here is nicer, since it's based on composing together existing primitives such as `Vec` and `Box`, rather than adding a whole new API. Additionally, it allows avoiding the additional `Box`ing in the case where the list consists of layers that are all the same type. Closes #1708 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
kaffarell
pushed a commit
to kaffarell/tracing
that referenced
this pull request
May 22, 2024
Depends on tokio-rs#2028 ## Motivation In many cases, it is desirable to have a variable number of `Layer`s at runtime (such as when reading a logging configuration from a config file). The current approach, where types implementing `Layer` are composed at the type level into `Layered` layers, doesn't work well when the number of layers varies at runtime. ## Solution To solve this, this branch adds a `Layer` implementation for `Vec<L> where L: Layer`. This allows variable-length lists of layers to be added to a subscriber. Although the impl for `Vec<L>` requires all the layers to be the same type, it can also be used in onjunction with `Box<dyn Layer<S> + ...>` trait objects to implement a variable-length list of layers of multiple types. I also wrote a bunch of docs examples. ## Notes Alternatively, we could have a separate type defined in `tracing-subscriber` for a variable-length list of type-erased `Layer`s. This would have one primary usability benefit, which is that we could have a `push` operation that takes an `impl Layer` and boxes it automatically. However, I thought the approach used here is nicer, since it's based on composing together existing primitives such as `Vec` and `Box`, rather than adding a whole new API. Additionally, it allows avoiding the additional `Box`ing in the case where the list consists of layers that are all the same type. Closes tokio-rs#1708 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
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.
Turns out the
Subscribeimpl forBox<dyn Subscribe<...> + ...>nevercalled the subscriber trait object's
register_callsitemethod. Whoops!This commit adds the missing
register_callsitehook.