Use Cow<'static, str> for Metadata strings #1020
Conversation
NOTE: requires changes in opentelemetry-rust as well
Subscriber is static
…e to just have `name` be a `Cow<'a, str>` instead for clarity.
* master: tracing: Instrument std::future::Future (tokio-rs#808) tracing: move `ValueSet` construction out of closures (tokio-rs#987)
Store Cows for name and target but return &str
Change test-only `CloseState` to use `String` Undo a few changes that are no longer necessary
* upstream/master: 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) chore: fix nightly clippy warnings (tokio-rs#991) chore: bump all crate versions (tokio-rs#998) macros: fix the `tracing-macros` crate not compiling (tokio-rs#1000) tracing: prepare to release 0.1.21 (tokio-rs#997) core: prepare to release 0.1.17 (tokio-rs#996) subscriber: make `PartialOrd` & `Ord` impls more correct (tokio-rs#995) core, tracing: don't inline dispatcher::get_default (tokio-rs#994) core: make `Level` and `LevelFilter` `Copy` (tokio-rs#992)
| @@ -122,7 +123,7 @@ impl<'a> Metadata<'a> { | |||
| /// Construct new metadata for a span or event, with a name, target, level, field | |||
| /// names, and optional source code location. | |||
| pub const fn new( | |||
There was a problem hiding this comment.
while with this we now have general support for dynamic creation, the current API, still won't allow for that. Probably want to add another fn dynamic_new where we can pass Cow<'a, str> into for dynamic dispatching.
|
As it stands this PR uses |
* 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)
tracing-core/src/metadata.rs
Outdated
| name: Cow<'a, str>, | ||
|
|
||
| /// The part of the system that the span that this metadata describes | ||
| /// occurred in. | ||
| target: &'a str, | ||
| target: Cow<'a, str>, | ||
|
|
||
| /// The level of verbosity of the described span. | ||
| level: Level, | ||
|
|
||
| /// The name of the Rust module where the span occurred, or `None` if this | ||
| /// could not be determined. | ||
| module_path: Option<&'a str>, | ||
| module_path: Option<Cow<'a, str>>, | ||
|
|
||
| /// The name of the source code file where the span occurred, or `None` if | ||
| /// this could not be determined. | ||
| file: Option<&'a str>, | ||
| file: Option<Cow<'a, str>>, | ||
|
|
There was a problem hiding this comment.
One fairly major issue with making this change: we've just changed tracing and tracing-core to support no-std platforms that don't have liballoc. Unfortunately, Cow is defined in liballoc. We'll need to change this to work gracefully on no_std platforms without alloc.
We now have an "alloc" feature flag for determining whether liballoc is supported, and APIs that depend on alloc can now require that feature. That should make it possible to conditionally support Cow only when alloc is availble, although it does make the API surface a little bit uglier.
Here's what I would recommend:
-
Change the fields to something like this:
/// ... #[cfg(feature = "alloc")] name: Cow<'a, str>, #[cfg(not(feature = "alloc"))] name: &'a str,
-
We can't change the return type of API functions based on whether or not a feature flag is enabled, since this would make enabling the feature break code in crates that don't enable it. Therefore, we can't have methods like
Metadata::file()andMetadata::line()returnOption<&str>when alloc is disabled, andOption<Cow<'a, str>>when it is enabled. Instead, we'll need two separate APIs: one which is always available but only returns borrowed strings, and another which returnsCows and is only available when the "alloc" feature is enabled.I would expect the implementation to look something like this:
impl<'a> Metadata<'a> { /// ... pub fn file(&'a self) -> Option<&'a str> { #[cfg(feature = "alloc")] self.file.as_ref().map(Cow::as_ref()) #[cfg(not(feature = "alloc"))] self.file.as_ref() } /// ... #[cfg(feature = "alloc")] #[cfg_attr(docsrs, doc(cfg(feature = "alloc")))] pub fn file_cow(&self) -> Option<&Cow<'a, String>> { self.file.as_ref() } }
(...and so on for the other fields).
Note that I don't particularly care for the name
file_cow(andtarget_cowetc), but I don't know if there are better options ---file_as_cowfeels a little wordy? If you can think of a better naming scheme for these, we can go with that. -
Finally, I think we'll need two constructors. The
const fn newwill continue to take '&str's only, and always be available, regardless of features. We'll add a new constructor which takesCows (orimpl Into<Cow<'a, str>>s?), and have that constructor only be exposed when the "alloc" feature is enabled.
Unfortunately, this makes the API a little less pleasant to work with, but it's the only way to feature flag the use of Cow without making a non-additive feature that breaks code when it's enabled...
There was a problem hiding this comment.
Thank you for the detailed feedback, much appreciated!
I made the change as suggested under 1) and while a bit ugly it works fine.
As for your second point I don't think it's necessarily a problem: as long as we can build a Metadata with dynamic data, it's fine to keep returning &str. So for example we keep Metadata::file returning an Option<&str> regardless of how it was built. FWIW I started this work thinking we'd need to duplicate the API and have methods return Cows, but @gnunicorn suggested we just keep things as they were. Do you think there's a need for/value in having the *_cow() methods?
I also added a from_cow constructor for your third point, featured gated. Struggling a bit to come up with a sensible test for it though.
There was a problem hiding this comment.
FWIW I started this work thinking we'd need to duplicate the API and have methods return Cows, but @gnunicorn suggested we just keep things as they were. Do you think there's a need for/value in having the *_cow() methods?
yeah, I really can't think of a good reason, why we'd need to expose the cow (in particular if non-mut) over just keeping the current expose functions.
Maybe re 2, I'd have the suggestion of just having two API-equivialent impl (and maybe even struct) blocks one for when the feature is activated and one without – and the earlier has the extra constructor for non-static-string. I think that would improve legibility quite a bit.
There was a problem hiding this comment.
I think that would improve legibility quite a bit.
I tried this and while the code does look better, I think I would need to duplicate the docs which increases the maintenance burden.
/// Lots of docs here to maintain
#[cfg(feature = "alloc")]
#[cfg_attr(docsrs, doc(cfg(any(feature = "std", feature = "alloc"))))]
pub struct Metadata<'a> {
}
/// Lots of docs here to maintain
#[cfg(not(feature = "alloc"))]
#[cfg_attr(docsrs, doc(cfg(not(any(feature = "std", feature = "alloc")))))]
pub struct Metadata<'a> {
}
#[cfg(not(feature = "alloc"))]
impl<'a> Metadata<'a> {
/// Construct new metadata for a span or event, with a name, target, level, field
/// names, and optional source code location.
pub const fn new(
…
}
}
#[cfg(feature = "alloc")]
impl<'a> Metadata<'a> {
/// Construct new metadata for a span or event, with a name, target, level, field
/// names, and optional source code location.
pub const fn new(…)
…
}
/// Construct new metadata for a span or event, with a name, target, level, field
/// names, and optional source code location using dynamically allocated data.
pub fn from_cow(…) {
…
}
}
impl<'a> Metadata<'a> {
// common impls here
}I think it's better to have an easier time maintaining the docs than more readable code in this case, but I'm easy and can do either.
@hawkw thoughts?
* upstream/master: 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)
* upstream/master: chore: fix tracing-macros::dbg (tokio-rs#1054)
* upstream/master: 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)
* upstream/master: subscriber: update sharded-slab to 0.1, pool hashmap allocations (tokio-rs#1062) subscriber: remove deprecated type, structs, and methods tokio-rs#1030 core: rename Subscriber to Collect (tokio-rs#1015) chore: fix rustdoc warning in tracing-subscriber (tokio-rs#1061)
|
Seems that there probably still needed to be an additional round of code review on this right? Since this PR is pretty old, I'll close it, but feel free to open a new one :) |
Allow Metadata to be dynamic (using
Cow<'a, str>)This experimental PR introduces
Cow<'a, str>to be used instead of&'static strinMetadataas outlined in the 0.2 tracking ticket to allow the dynamic generation of Metadata.We'd very much appreciate your feedback on this.
Rationale
When implementing tracing in Substrate (thanks and ❤️ for your great work here, it is an awesome project you are doing!), we ran into an issue with the current API.
In Substrate the innermost execution logic is compiled to Wasm and runs inside an executor, with its own memory. The Wasm runtime execution logic can change at any time which means we can't share or pre-emptively know the
&'static strof a span from within the runtime and communicate with the outer infrastructure that implements the actualtracing(the inner just provides host functions). Thus we can't create the properMetadata/Spanand so forth fortracing.We've resorted to pretty ugly hacks to at least provide this for levels, so the generic filters and alike work on that, but for
targetandnamefrom traces within Wasm the existing filters don't work. For that we have to generateMetadataat runtime.If we could provide a
CowtoMetadatainstead of&'static str, we could generate the entries on the fly and feed them intotracing. All tooling (likeEnvFilter) would work just as before.This PR is a first step to reach this goal.
Status:
tracingto storeCow<'a, str>MetadataBenchmarks comparing this branch and master as of October 20: