Switch FmtSpan To a Combinable Bitflag #1277

zicklag · 2021-03-03T20:22:30Z

Allows arbitrarily combining different FmtSpan events to listen to.

Motivation

The idea is to allow any combination of FmtSpan options, such as the currently unachievable combination of FmtSpan::NEW | FmtSpan::CLOSE.

Solution

Make FmtSpan behave like a bitflag that can be combined using the bitwise or operator ( | ) while maintaining backward compatibility by keeping the same names for all existing presets and keeping the implementation details hidden.

hawkw · 2021-03-03T21:59:01Z

Great, thanks for working on this!

I'm going to give a more complete review shortly, but before I do, I just wanted to let you know that the CI failure is due to an issue with a dependency and is not your fault or related to this change at all. Fortunately it looks like that'll be fixed upstream soon.

tracing-subscriber/src/fmt/fmt_subscriber.rs

tracing-subscriber/src/fmt/mod.rs

davidbarsky · 2021-03-04T17:10:54Z

tracing-subscriber/src/fmt/mod.rs

+    /// - `FmtSpan::ACTIVE`: Events will be synthesized when spans are entered
+    ///    or exited.


Suggested change

/// - `FmtSpan::ACTIVE`: Events will be synthesized when spans are entered

/// or exited.

/// - `FmtSpan::ACTIVE`: An event will be synthesized when spans are entered

/// or exited.

Also, question for @hawkw: should we deprecate this variant and pushing folks to use FmtSpan::Enter | FmtSpan::Exit instead?

(These changes have been merged, but unresolving to make this discussion more visible.)

IMHO ACTIVE is not the most intuitive name and it would be more natural if you just combined them explicitly. That would make things more "self-documenting" with the code being more obvious and longer descriptions of the meaning of flags less necessary.

tracing-subscriber/src/fmt/mod.rs

davidbarsky · 2021-03-04T17:12:26Z

Thanks for opening this PR! Just a few comments.

zicklag · 2021-03-04T17:20:42Z

No problem, glad I could help!

I committed all of your wording suggestions.

hawkw

Overall this looks good to me!

I noticed that the fmt::Debug implementation for FmtSpan will produce somewhat ugly output, so I suggested a potential improvement there. Once that's fixed, I'll be happy to merge this!

tracing-subscriber/src/fmt/format/mod.rs

hawkw · 2021-03-10T22:05:49Z

tracing-subscriber/src/fmt/format/mod.rs

+impl_fmt_span_bit_op!(BitAnd, bitand, &);
+impl_fmt_span_bit_op!(BitOr, bitor, |);
+impl_fmt_span_bit_op!(BitXor, bitxor, ^);


i'm not totally convinced we actually need an & operation, as i'm not sure how often user code will actually need to test for the presence of flags. i don't think providing it is a problem, though, so this is fine

Yeah, I wasn't positive either, but I figured if we were going to act like it was a bitflag, a user might expect that they could do it so I thought we might as well add it.

Fixes tokio-rs#1136. Allows arbitrarily combining different FmtSpan events to listen to.

hawkw · 2021-03-10T22:36:20Z

tracing-subscriber/src/fmt/format/mod.rs

+        if FmtSpan::NONE | self.clone() == FmtSpan::NONE {
+            f.write_str("FmtSpan::NONE")?;
+        } else {
+            write_flags(FmtSpan::NEW, "FmtSpan::NEW")?;
+            write_flags(FmtSpan::ENTER, "FmtSpan::ENTER")?;
+            write_flags(FmtSpan::EXIT, "FmtSpan::EXIT")?;
+            write_flags(FmtSpan::CLOSE, "FmtSpan::CLOSE")?;
        }


<3 looks great

hawkw

I'm going to go ahead and merge this now; we can add more tests etc in a follow-up.

Thanks again!

hawkw · 2021-03-11T18:40:30Z

tracing-subscriber/src/fmt/format/mod.rs

@@ -1284,4 +1336,31 @@ pub(super) mod test {
        assert_eq!(fmt(123456789012), "123s");
        assert_eq!(fmt(1234567890123), "1235s");
    }
+
+    #[test]
+    fn fmt_span_combinations() {


TIOLI: it could be nice to have a test that we actually do reasonable behavior with various bitflag combinations, as well, but I'm not going to block merging this on that, the code is pretty straightforward.

zicklag · 2021-03-11T18:51:46Z

Glad I could help! This was one of my pet annoyances with tracing so it's cool to be able to fix it. 😄

This backports #1277 from `master`. Fixes #1136. Allows arbitrarily combining different FmtSpan events to listen to. Motivation ---------- The idea is to allow any combination of `FmtSpan` options, such as the currently unachievable combination of `FmtSpan::NEW | FmtSpan::CLOSE`. Solution -------- Make `FmtSpan` behave like a bitflag that can be combined using the bitwise or operator ( `|` ) while maintaining backward compatibility by keeping the same names for all existing presets and keeping the implementation details hidden.

@cratelyn

# 0.2.17 (March 12, 2020) ### Fixed - **fmt**: `Pretty` formatter now honors `with_ansi(false)` to disable ANSI terminal formatting ([#1240]) - **fmt**: Fixed extra padding when using `Pretty` formatter ([#1275]) - **chrono**: Removed extra trailing space with `ChronoLocal` time formatter ([#1103]) ### Added - **fmt**: Added `FmtContext::current_span()` method, returning the current span ([#1290]) - **fmt**: `FmtSpan` variants may now be combined using the `|` operator for more granular control over what span events are generated ([#1277]) Thanks to new contributors @cratelyn, @dignati, and @zicklag, as well as @Folyd, @matklad, and @najamelan, for contributing to this release!

@cratelyn

# 0.2.17 (March 12, 2020) ### Fixed - **fmt**: `Pretty` formatter now honors `with_ansi(false)` to disable ANSI terminal formatting ([#1240]) - **fmt**: Fixed extra padding when using `Pretty` formatter ([#1275]) - **chrono**: Removed extra trailing space with `ChronoLocal` time formatter ([#1103]) ### Added - **fmt**: Added `FmtContext::current_span()` method, returning the current span ([#1290]) - **fmt**: `FmtSpan` variants may now be combined using the `|` operator for more granular control over what span events are generated ([#1277]) Thanks to new contributors @cratelyn, @dignati, and @zicklag, as well as @Folyd, @matklad, and @najamelan, for contributing to this release! [#1240]: #1240 [#1275]: #1275 [#1103]: #1103 [#1290]: #1290 [#1277]: #1277

3532: Bump tracing-subscriber from 0.2.15 to 0.2.17 r=mergify[bot] a=dependabot[bot] Bumps [tracing-subscriber](https://github.com/tokio-rs/tracing) from 0.2.15 to 0.2.17. <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/tokio-rs/tracing/commit/88685417f66ae7b74df57f56ebe28256dc9fd7ff"><code>8868541</code></a> subscriber: prepare to release 0.2.17 (<a href="https://github.com/tokio-rs/tracing/issues/1302">#1302</a>)</li> <li><a href="https://github.com/tokio-rs/tracing/commit/742b21461bffc59c60ba4d5726e752144b3fffc3"><code>742b214</code></a> attributes: prepare to release v0.1.15 (<a href="https://github.com/tokio-rs/tracing/issues/1301">#1301</a>)</li> <li><a href="https://github.com/tokio-rs/tracing/commit/ec7313466dbe615ed8397422bc0aa34280dec0a6"><code>ec73134</code></a> subscriber: update pretty formatter for no ansi (<a href="https://github.com/tokio-rs/tracing/issues/1240">#1240</a>)</li> <li><a href="https://github.com/tokio-rs/tracing/commit/a947c5a63f73b840d100ee85638e90a3488f9789"><code>a947c5a</code></a> subscriber: change <code>FmtSpan</code> to a combinable bitflag (<a href="https://github.com/tokio-rs/tracing/issues/1277">#1277</a>)</li> <li><a href="https://github.com/tokio-rs/tracing/commit/f3eb5ccf2dc128ef171a2320bff488c144903376"><code>f3eb5cc</code></a> subscriber: fix extra padding in pretty format (<a href="https://github.com/tokio-rs/tracing/issues/1275">#1275</a>)</li> <li><a href="https://github.com/tokio-rs/tracing/commit/ac0aa9763b434885bba27db7334683c0e5665e97"><code>ac0aa97</code></a> attributes: fix <code>#[instrument]</code> skipping code when returning pinned futures (...</li> <li><a href="https://github.com/tokio-rs/tracing/commit/544da6b5d8b489cee084974100514e38fe25b6ec"><code>544da6b</code></a> subscriber: Add a public <code>current_span()</code> method for <code>FmtContext</code> (<a href="https://github.com/tokio-rs/tracing/issues/1290">#1290</a>)</li> <li><a href="https://github.com/tokio-rs/tracing/commit/14d11ebdd60db0955806d10d31345a54a79a38f7"><code>14d11eb</code></a> subscriber: remove unnecessary transparent attribute (<a href="https://github.com/tokio-rs/tracing/issues/1282">#1282</a>)</li> <li><a href="https://github.com/tokio-rs/tracing/commit/51812267b8af46244c412cf0490391d024e60902"><code>5181226</code></a> subscriber: use struct update syntax when constructing from <code>self</code> (<a href="https://github.com/tokio-rs/tracing/issues/1289">#1289</a>)</li> <li><a href="https://github.com/tokio-rs/tracing/commit/6f9b537c2ebbfb73c57b5c40dc25e011eb1b4d7c"><code>6f9b537</code></a> subscriber: Remove trailing space from ChronoLocal time formatter. (<a href="https://github.com/tokio-rs/tracing/issues/1103">#1103</a>)</li> <li>Additional commits viewable in <a href="https://github.com/tokio-rs/tracing/compare/tracing-subscriber-0.2.15...tracing-subscriber-0.2.17">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=tracing-subscriber&package-manager=cargo&previous-version=0.2.15&new-version=0.2.17)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually </details> 3533: Bump socket2 from 0.3.19 to 0.4.0 r=mergify[bot] a=dependabot[bot] Bumps [socket2](https://github.com/rust-lang/socket2) from 0.3.19 to 0.4.0. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/rust-lang/socket2/blob/master/CHANGELOG.md">socket2's changelog</a>.</em></p> <blockquote> <h1>0.4.0</h1> <h2>Added</h2> <ul> <li>New <code>all</code> feature: enables API that is not available on all tier 1 platforms.</li> <li><code>SockRef</code> type: used to create a reference to an existing socket, e.g. <code>std::net::TcpStream</code>, making all methods of <code>Socket</code> available on it.</li> <li>Support for vectored I/O: <ul> <li><code>Socket::recv_vectored</code>, <code>Socket::recv_with_flags</code>.</li> <li><code>Socket::recv_from_vectored</code>, <code>Socket::recv_from_vectored_with_flags</code>.</li> <li><code>Socket::send_vectored</code>, <code>Socket::send_vectored_with_flags</code>.</li> <li><code>Socket::send_to_vectored</code>, <code>Socket::send_to_vectored_with_flags</code>.</li> <li>In the <code>Read</code> and <code>Write</code> implementations.</li> </ul> </li> <li><code>Socket::new_raw</code>, <code>Socket::pair_raw</code> and <code>Socket::accept_raw</code> methods that don't set common flags, such as the close-on-exec flag.</li> <li><code>Socket::accept4</code>: <code>accept4(2)</code> system call.</li> <li><code>Socket::sendfile</code>: the <code>sendfile(2)</code> system call.</li> <li><code>Socket::set_cloexec</code>: set the close-on-exec flag on Unix.</li> <li><code>Socket::set_no_inherit</code>: set inherit handle flag on Windows.</li> <li><code>Socket::set_nosigpipe</code>: set <code>SO_NOSIGPIPE</code> on Apple targets.</li> <li><code>Socket::set_mark</code> and <code>Socket::mark</code>, setting/getting the <code>SO_MARK</code> socket option.</li> <li><code>Socket::set_cpu_affinity</code> and <code>Socket::cpu_affinity</code>, setting/getting the <code>SO_INCOMING_CPU</code> socket option.</li> <li><code>Socket::set_mss</code> and <code>Socket::mss</code>, setting/getting the <code>TCP_MAXSEG</code> socket option.</li> <li><code>Socket::set_freebind</code> and <code>Socket::freebind</code>, setting/getting the <code>IP_FREEBIND</code> socket option.</li> <li><code>Socket::bind_device</code> and <code>Socket::device</code>, setting/getting the <code>SO_BINDTODEVICE</code> socket option.</li> <li>Adopted Mio's TCP keepalive API: <ul> <li><code>Socket::keepalive_time</code>,</li> <li><code>Socket::keepalive_interval</code>,</li> <li><code>Socket::keepalive_retries</code>,</li> <li><code>Socket::set_tcp_keepalive</code>.</li> </ul> </li> <li><code>Socket::is_listener</code> getting the <code>SO_ACCEPTCONN</code> socket option.</li> <li><code>Socket::domain</code> getting the <code>SO_DOMAIN</code> socket option.</li> <li><code>Socket::protocol</code> getting the <code>SO_PROTOCOL</code> socket option.</li> <li><code>Socket::type</code> getting the <code>SO_TYPE</code> socket option.</li> <li><code>Domain::for_address</code>: the correct <code>Domain</code> for a <code>std::net::SocketAddr</code>.</li> <li><code>Type::nonblocking</code>: set <code>SOCK_NONBLOCK</code>.</li> <li><code>Type::cloexec</code>: set <code>SOCK_CLOEXEC</code>.</li> <li><code>Type::no_inherit</code>: set <code>HANDLE_FLAG_INHERIT</code>.</li> <li><code>SockAddr::init</code>: initialises a <code>SockAddr</code>.</li> <li><code>MaybeUninitSlice</code> type: a version of <code>IoSliceMut</code> that allows the buffer to be uninitialised, used in <code>Socket::recv_vectored</code> and related functions.</li> <li><code>RecvFlags</code> type: provides additional information about incoming messages, returned by <code>Socket::recv_vectored</code> and related functions.</li> <li><code>TcpKeepalive</code> type: configuration type for a socket's TCP keepalive parameters.</li> </ul>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/rust-lang/socket2/commit/b1479fffa0749147eabd15ad9038d0f9a0cc7825"><code>b1479ff</code></a> Release v0.4.0</li> <li><a href="https://github.com/rust-lang/socket2/commit/adf32dabfc5c152af8488161b1d99512c8b8b054"><code>adf32da</code></a> Use SO_LINGER_SEC for Socket::get/set_linger on macOS</li> <li><a href="https://github.com/rust-lang/socket2/commit/559ba4290899002406bb4c89caef7905a186ba0a"><code>559ba42</code></a> Fuchsia doesn't support <code>sendfile</code></li> <li><a href="https://github.com/rust-lang/socket2/commit/94c32b95921c13740abfa8a109acc821a4f2ed05"><code>94c32b9</code></a> Release v0.4.0-alpha.5</li> <li><a href="https://github.com/rust-lang/socket2/commit/ca694de9a75f52607693704dc0fa6fa7573b089d"><code>ca694de</code></a> Change Socket::(set_)cpu_affinity to take an immutable reference</li> <li><a href="https://github.com/rust-lang/socket2/commit/4f643fc66fd6ab52d86477a6d93dd04dd209115a"><code>4f643fc</code></a> Release v0.4.0-alpha.4</li> <li><a href="https://github.com/rust-lang/socket2/commit/c3b447e1597886fc81c12ba108201944235569b1"><code>c3b447e</code></a> Add Socket::(set_)cpu_affinity</li> <li><a href="https://github.com/rust-lang/socket2/commit/33da3507f8e5205f7a66654b8349aec112447ef6"><code>33da350</code></a> Release v0.4.0-alpha.3</li> <li><a href="https://github.com/rust-lang/socket2/commit/4f22376fc1fba8062f756e2c4f4bdf7d05ede5a9"><code>4f22376</code></a> tests: add vsock test</li> <li><a href="https://github.com/rust-lang/socket2/commit/07c7b373615e1011f51e79269960d72fe006f089"><code>07c7b37</code></a> unix: add SockAddr::vsock_address()</li> <li>Additional commits viewable in <a href="https://github.com/rust-lang/socket2/compare/v0.3.19...v0.4.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=socket2&package-manager=cargo&previous-version=0.3.19&new-version=0.4.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually </details> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

This backports tokio-rs#1277 from `master`. Fixes tokio-rs#1136. Allows arbitrarily combining different FmtSpan events to listen to. Motivation ---------- The idea is to allow any combination of `FmtSpan` options, such as the currently unachievable combination of `FmtSpan::NEW | FmtSpan::CLOSE`. Solution -------- Make `FmtSpan` behave like a bitflag that can be combined using the bitwise or operator ( `|` ) while maintaining backward compatibility by keeping the same names for all existing presets and keeping the implementation details hidden.

@cratelyn

# 0.2.17 (March 12, 2020) ### Fixed - **fmt**: `Pretty` formatter now honors `with_ansi(false)` to disable ANSI terminal formatting ([tokio-rs#1240]) - **fmt**: Fixed extra padding when using `Pretty` formatter ([tokio-rs#1275]) - **chrono**: Removed extra trailing space with `ChronoLocal` time formatter ([tokio-rs#1103]) ### Added - **fmt**: Added `FmtContext::current_span()` method, returning the current span ([tokio-rs#1290]) - **fmt**: `FmtSpan` variants may now be combined using the `|` operator for more granular control over what span events are generated ([tokio-rs#1277]) Thanks to new contributors @cratelyn, @dignati, and @zicklag, as well as @Folyd, @matklad, and @najamelan, for contributing to this release! [tokio-rs#1240]: tokio-rs#1240 [tokio-rs#1275]: tokio-rs#1275 [tokio-rs#1103]: tokio-rs#1103 [tokio-rs#1290]: tokio-rs#1290 [tokio-rs#1277]: tokio-rs#1277

zicklag requested review from davidbarsky, hawkw and a team as code owners March 3, 2021 20:22

zicklag force-pushed the feature/flexible-fmtspan branch 2 times, most recently from 408ac70 to ab21a76 Compare March 3, 2021 20:42

zicklag mentioned this pull request Mar 3, 2021

Allow for synthesized events on created/closed #1136

Closed

davidbarsky requested changes Mar 4, 2021

View reviewed changes

zicklag force-pushed the feature/flexible-fmtspan branch from b082a9e to f5f9213 Compare March 4, 2021 17:19

hawkw requested changes Mar 10, 2021

View reviewed changes

Switch FmtSpan To a Combinable Bitflag

2304525

Fixes tokio-rs#1136. Allows arbitrarily combining different FmtSpan events to listen to.

zicklag force-pushed the feature/flexible-fmtspan branch from f5f9213 to 2304525 Compare March 10, 2021 22:30

hawkw approved these changes Mar 10, 2021

View reviewed changes

hawkw requested a review from davidbarsky March 10, 2021 22:36

hawkw approved these changes Mar 11, 2021

View reviewed changes

hawkw merged commit ba57971 into tokio-rs:master Mar 11, 2021

zicklag deleted the feature/flexible-fmtspan branch March 11, 2021 18:51

hawkw mentioned this pull request Mar 12, 2021

backport recent subscriber and attributes changes #1300

Merged

hawkw mentioned this pull request Mar 12, 2021

subscriber: prepare to release 0.2.17 #1302

Merged

This was referenced Mar 17, 2021

build(deps): bump tracing-subscriber from 0.2.16 to 0.2.17 wangandi/materialize#23

Closed

build(deps): bump tracing-subscriber from 0.2.16 to 0.2.17 ruchirK/materialize#41

Closed

davidbarsky mentioned this pull request Sep 26, 2023

chore: backport roughly a year's worth of changes #2728

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch FmtSpan To a Combinable Bitflag #1277

Switch FmtSpan To a Combinable Bitflag #1277

zicklag commented Mar 3, 2021 •

edited

Loading

hawkw commented Mar 3, 2021

davidbarsky Mar 4, 2021

davidbarsky Mar 4, 2021

zicklag Mar 4, 2021

davidbarsky commented Mar 4, 2021

zicklag commented Mar 4, 2021

hawkw left a comment

hawkw Mar 10, 2021

zicklag Mar 10, 2021

hawkw Mar 10, 2021

hawkw left a comment

hawkw Mar 11, 2021

zicklag commented Mar 11, 2021

		/// - `FmtSpan::ACTIVE`: Events will be synthesized when spans are entered
		/// or exited.

Switch FmtSpan To a Combinable Bitflag #1277

Switch FmtSpan To a Combinable Bitflag #1277

Conversation

zicklag commented Mar 3, 2021 • edited Loading

Motivation

Solution

hawkw commented Mar 3, 2021

davidbarsky Mar 4, 2021

Choose a reason for hiding this comment

davidbarsky Mar 4, 2021

Choose a reason for hiding this comment

zicklag Mar 4, 2021

Choose a reason for hiding this comment

davidbarsky commented Mar 4, 2021

zicklag commented Mar 4, 2021

hawkw left a comment

Choose a reason for hiding this comment

hawkw Mar 10, 2021

Choose a reason for hiding this comment

zicklag Mar 10, 2021

Choose a reason for hiding this comment

hawkw Mar 10, 2021

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw Mar 11, 2021

Choose a reason for hiding this comment

zicklag commented Mar 11, 2021

zicklag commented Mar 3, 2021 •

edited

Loading