-
Notifications
You must be signed in to change notification settings - Fork 734
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
subscriber: fix reload
ergonomics
#1035
Merged
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
Currently, the `reload` layer is generic over both the type of the layer being reloaded, *and* the type of the subscriber that the layer is layered onto. This means that the `reload::Handle` type that's used to reload the value of the layer *also* is parameterized over the subscriber's type. The subscriber type parameter makes the `reload` API significantly harder to use. Any time a `reload::Handle` is returned by a function, taken as an argument, or stored in a struct, the full type of the subscriber under the layer must be written out --- and often, it is quite long. What makes this worse is that sometimes the type of the subscriber may vary at runtime based on some configuration, while the type of the layer that's reloaded remains the same. For example, in Linkerd, we've had to do [this][1], which is really not ideal. This branch removes the `Subscriber` type parameter from `reload::Layer` and `reload::Handle`. Now, the `Handle` type is only generic over the type of the inner layer that's being reloaded. It turns out that the `Subscriber` type parameter was only necessary to add a `L: Layer<S>` bound to `reload::Layer`'s constructor, which isn't really necessary --- if the layer does not implement `Layer<S>`, the type error will occur when `Subscriber::with` is actually used to layer it, which is fine. I also changed the `with_filter_reloading` option on the `FmtSubscriber` builder to also work with `LevelFilter`s, since there's no reason for it not to, and added an example. Since this breaks existing code, this change has to be made as part of 0.3. [1]: https://github.com/linkerd/linkerd2-proxy/blob/6c484f6dcdeebda18b68c800b4494263bf98fcdc/linkerd/app/core/src/trace.rs#L19-L36
LucioFranco
approved these changes
Oct 13, 2020
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
dvdplm
added a commit
to dvdplm/tracing
that referenced
this pull request
Oct 16, 2020
…spatch-as-ref-tokio-rs#455 * upstream/master: (28 commits) opentelemetry: prepare for 0.8.0 release (tokio-rs#1036) docs: add favicon for extra pretty docs (tokio-rs#1033) subscriber: fix `reload` ergonomics (tokio-rs#1035) chore(deps): update crossbeam-channel requirement from 0.4.2 to 0.5.0 (tokio-rs#1031) opentelemetry: Assign default ids if missing (tokio-rs#1027) chore: remove deprecated add-path from CI (tokio-rs#1026) attributes: fix `#[instrument(err)]` in case of early returns (tokio-rs#1006) core: remove mandatory liballoc dependency with no-std (tokio-rs#1017) chore(deps): update cfg-if requirement from 0.1.10 to 1.0.0 (tokio-rs#1023) subscriber: warn if trying to enable a statically disabled level (tokio-rs#990) subscriber: use macros for module declarations (tokio-rs#1009) chore: remove `stdlib.rs` (tokio-rs#1008) core: fix linked list tests reusing `Registration`s (tokio-rs#1016) subscriber: support dash in target names (tokio-rs#1012) docs: switch to intra-doc links in tracing-core (tokio-rs#1010) tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007) core: add intrusive linked list for callsite registry (tokio-rs#988) serde: allow tracing-serde to work on no_std. (tokio-rs#960) tracing: remove `Into<Option<Id>>` impl for `Span` (tokio-rs#1003) tracing: make `Entered` `!Send` (tokio-rs#1001) ...
dvdplm
added a commit
to dvdplm/tracing
that referenced
this pull request
Oct 16, 2020
* upstream/master: opentelemetry: prepare for 0.8.0 release (tokio-rs#1036) docs: add favicon for extra pretty docs (tokio-rs#1033) subscriber: fix `reload` ergonomics (tokio-rs#1035) chore(deps): update crossbeam-channel requirement from 0.4.2 to 0.5.0 (tokio-rs#1031) opentelemetry: Assign default ids if missing (tokio-rs#1027) chore: remove deprecated add-path from CI (tokio-rs#1026) attributes: fix `#[instrument(err)]` in case of early returns (tokio-rs#1006) core: remove mandatory liballoc dependency with no-std (tokio-rs#1017) chore(deps): update cfg-if requirement from 0.1.10 to 1.0.0 (tokio-rs#1023)
dvdplm
added a commit
to dvdplm/tracing
that referenced
this pull request
Oct 19, 2020
…cros * upstream/master: (30 commits) chore(deps): update pin-project requirement from 0.4 to 1.0 (tokio-rs#1038) chore: remove duplicated section from tracing/README.md (tokio-rs#1046) opentelemetry: prepare for 0.8.0 release (tokio-rs#1036) docs: add favicon for extra pretty docs (tokio-rs#1033) subscriber: fix `reload` ergonomics (tokio-rs#1035) chore(deps): update crossbeam-channel requirement from 0.4.2 to 0.5.0 (tokio-rs#1031) opentelemetry: Assign default ids if missing (tokio-rs#1027) chore: remove deprecated add-path from CI (tokio-rs#1026) attributes: fix `#[instrument(err)]` in case of early returns (tokio-rs#1006) core: remove mandatory liballoc dependency with no-std (tokio-rs#1017) chore(deps): update cfg-if requirement from 0.1.10 to 1.0.0 (tokio-rs#1023) subscriber: warn if trying to enable a statically disabled level (tokio-rs#990) subscriber: use macros for module declarations (tokio-rs#1009) chore: remove `stdlib.rs` (tokio-rs#1008) core: fix linked list tests reusing `Registration`s (tokio-rs#1016) subscriber: support dash in target names (tokio-rs#1012) docs: switch to intra-doc links in tracing-core (tokio-rs#1010) tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007) core: add intrusive linked list for callsite registry (tokio-rs#988) serde: allow tracing-serde to work on no_std. (tokio-rs#960) ...
dvdplm
added a commit
to dvdplm/tracing
that referenced
this pull request
Oct 19, 2020
…gger * upstream/master: (31 commits) chore: fix tracing-macros::dbg (tokio-rs#1054) chore(deps): update pin-project requirement from 0.4 to 1.0 (tokio-rs#1038) chore: remove duplicated section from tracing/README.md (tokio-rs#1046) opentelemetry: prepare for 0.8.0 release (tokio-rs#1036) docs: add favicon for extra pretty docs (tokio-rs#1033) subscriber: fix `reload` ergonomics (tokio-rs#1035) chore(deps): update crossbeam-channel requirement from 0.4.2 to 0.5.0 (tokio-rs#1031) opentelemetry: Assign default ids if missing (tokio-rs#1027) chore: remove deprecated add-path from CI (tokio-rs#1026) attributes: fix `#[instrument(err)]` in case of early returns (tokio-rs#1006) core: remove mandatory liballoc dependency with no-std (tokio-rs#1017) chore(deps): update cfg-if requirement from 0.1.10 to 1.0.0 (tokio-rs#1023) subscriber: warn if trying to enable a statically disabled level (tokio-rs#990) subscriber: use macros for module declarations (tokio-rs#1009) chore: remove `stdlib.rs` (tokio-rs#1008) core: fix linked list tests reusing `Registration`s (tokio-rs#1016) subscriber: support dash in target names (tokio-rs#1012) docs: switch to intra-doc links in tracing-core (tokio-rs#1010) tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007) core: add intrusive linked list for callsite registry (tokio-rs#988) ...
dvdplm
added a commit
to dvdplm/tracing
that referenced
this pull request
Oct 22, 2020
…spatch-as-owned-tokio-rs#455 * upstream/master: (34 commits) subscriber: remove TraceLogger (tokio-rs#1052) subscriber: make Registry::enter/exit much faster (tokio-rs#1058) chore(deps): update env_logger requirement from 0.7 to 0.8 (tokio-rs#1050) chore: fix tracing-macros::dbg (tokio-rs#1054) chore(deps): update pin-project requirement from 0.4 to 1.0 (tokio-rs#1038) chore: remove duplicated section from tracing/README.md (tokio-rs#1046) opentelemetry: prepare for 0.8.0 release (tokio-rs#1036) docs: add favicon for extra pretty docs (tokio-rs#1033) subscriber: fix `reload` ergonomics (tokio-rs#1035) chore(deps): update crossbeam-channel requirement from 0.4.2 to 0.5.0 (tokio-rs#1031) opentelemetry: Assign default ids if missing (tokio-rs#1027) chore: remove deprecated add-path from CI (tokio-rs#1026) attributes: fix `#[instrument(err)]` in case of early returns (tokio-rs#1006) core: remove mandatory liballoc dependency with no-std (tokio-rs#1017) chore(deps): update cfg-if requirement from 0.1.10 to 1.0.0 (tokio-rs#1023) subscriber: warn if trying to enable a statically disabled level (tokio-rs#990) subscriber: use macros for module declarations (tokio-rs#1009) chore: remove `stdlib.rs` (tokio-rs#1008) core: fix linked list tests reusing `Registration`s (tokio-rs#1016) subscriber: support dash in target names (tokio-rs#1012) ...
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.
Motivation
Currently, the
reload
layer is generic over both the type of the layerbeing reloaded, and the type of the subscriber that the layer is
layered onto. This means that the
reload::Handle
type that's used toreload the value of the layer also is parameterized over the
subscriber's type.
The subscriber type parameter makes the
reload
API significantlyharder to use. Any time a
reload::Handle
is returned by a function,taken as an argument, or stored in a struct, the full type of the
subscriber under the layer must be written out --- and often, it is
quite long. What makes this worse is that sometimes the type of the
subscriber may vary at runtime based on some configuration, while the
type of the layer that's reloaded remains the same. For example, in
Linkerd, we've had to do this, which is really not ideal.
Solution
This branch removes the
Subscriber
type parameter fromreload::Layer
and
reload::Handle
. Now, theHandle
type is only generic over thetype of the inner layer that's being reloaded. It turns out that the
Subscriber
type parameter was only necessary to add aL: Layer<S>
bound to
reload::Layer
's constructor, which isn't really necessary ---if the layer does not implement
Layer<S>
, the type error will occurwhen
Subscriber::with
is actually used to layer it, which is fine.I also changed the
with_filter_reloading
option on theFmtSubscriber
builder to also work with
LevelFilter
s, since there's no reason for itnot to, and added an example.
Since this breaks existing code, this change has to be made as part of
0.3.