Skip to content
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

appender: replace chrono with time #1652

Merged
merged 14 commits into from
Oct 22, 2021

Conversation

davidbarsky
Copy link
Member

Motivation

This PR continues the work started in #1646 to replace chrono with time. I'll refer to @hawkw's motivation:

Currently, tracing-subscriber supports the chrono crate for timestamp formatting, via a default-on feature flag. When this code was initially added to tracing-subscriber, the time crate did not have support for the timestamp formatting options we needed.

Unfortunately, the chrono crate's maintenance status is now in question (see #1598). Furthermore, chrono depends on version 0.1 of the time crate, which contains a security vulnerability (https://rustsec.org/advisories/RUSTSEC-2020-0071.html). This vulnerability is fixed in more recent releases of time, but chrono still uses v0.1.

Solution

I've replaced chrono with time 0.3. Unfortunately, some of chrono's builders for DateTimes are not present in time, which required the usage of macro_rules! macros to construct some of the hard-coded times. I also took the opportunity to tidy some of the tests and change the internal representation of Rotation::NEVER from year 9,999 to an Option::None.

@davidbarsky davidbarsky requested a review from a team as a code owner October 19, 2021 19:21
@davidbarsky davidbarsky self-assigned this Oct 19, 2021
tracing-appender/Cargo.toml Outdated Show resolved Hide resolved
tracing-appender/src/inner.rs Show resolved Hide resolved
tracing-appender/src/inner.rs Show resolved Hide resolved
tracing-appender/src/rolling.rs Outdated Show resolved Hide resolved
tracing-appender/src/rolling.rs Show resolved Hide resolved
tracing-appender/src/rolling.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Oct 19, 2021

hmm looks like including the macros as a dev dependency still breaks MSRV. can we drop them from the tests as well?

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall, LGTM; can we update the tracing-appender MSRV docs before merging this?

@@ -21,7 +21,7 @@ jobs:
uses: actions-rs/cargo@v1
with:
command: check
args: --all
args: --all --exclude=tracing-appender
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also update the MSRV in tracing-appender's documentation & readme?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup!

tracing-appender/src/rolling.rs Show resolved Hide resolved
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSRV docs need to be updated in a couple additional places; modulo that, this LGTM

.github/workflows/CI.yml Outdated Show resolved Hide resolved
tracing-appender/README.md Show resolved Hide resolved
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, this looks great to me!

up to you, but i don't think it's really necessary to mention the project MSRV for other crates in the MSRV docs for tracing-appender. it could just be simplified to discuss that crate.

not a blocker though.

tracing-appender/src/lib.rs Outdated Show resolved Hide resolved
tracing-appender/README.md Outdated Show resolved Hide resolved
@hawkw hawkw merged commit 336f8ca into master Oct 22, 2021
@hawkw hawkw deleted the davidbarsky/once-upon-a-time-in-tracing branch October 22, 2021 17:46
hawkw pushed a commit that referenced this pull request Oct 22, 2021
## Motivation

This PR continues the work started in
#1646 to replace `chrono` with
`time`. I'll refer to @hawkw's motivation:

> Currently, `tracing-subscriber` supports the `chrono` crate for
> timestamp formatting, via a default-on feature flag. When this code
> was initially added to `tracing-subscriber`, the `time` crate did not
> have support for the timestamp formatting options we needed.
>
> Unfortunately, the `chrono` crate's maintenance status is now in
> question (see #1598). Furthermore, `chrono` depends on version 0.1 of
> the `time` crate, which contains a security vulnerability
> (https://rustsec.org/advisories/RUSTSEC-2020-0071.html). This
> vulnerability is fixed in more recent releases of `time`, but `chrono`
> still uses v0.1.

## Solution

I've replaced chrono with time 0.3. Unfortunately, some of chrono's
builders for DateTimes are not present in `time`, which required the
usage of `macro_rules!` macros to construct some of the hard-coded
times. I also took the opportunity to tidy some of the tests and change
the internal representation of `Rotation::NEVER` from year 9,999 to an
`Option::None`.

This branch changes `tracing-appender`'s MSRV from Rust 1.42 to Rust
1.51, the MSRV for the `time` crate when certain required feature flags
are enabled. This does *not* effect the MSRV for other crates in this
repository.
hawkw pushed a commit that referenced this pull request Oct 22, 2021
## Motivation

This PR continues the work started in
#1646 to replace `chrono` with
`time`. I'll refer to @hawkw's motivation:

> Currently, `tracing-subscriber` supports the `chrono` crate for
> timestamp formatting, via a default-on feature flag. When this code
> was initially added to `tracing-subscriber`, the `time` crate did not
> have support for the timestamp formatting options we needed.
>
> Unfortunately, the `chrono` crate's maintenance status is now in
> question (see #1598). Furthermore, `chrono` depends on version 0.1 of
> the `time` crate, which contains a security vulnerability
> (https://rustsec.org/advisories/RUSTSEC-2020-0071.html). This
> vulnerability is fixed in more recent releases of `time`, but `chrono`
> still uses v0.1.

## Solution

I've replaced chrono with time 0.3. Unfortunately, some of chrono's
builders for DateTimes are not present in `time`, which required the
usage of `macro_rules!` macros to construct some of the hard-coded
times. I also took the opportunity to tidy some of the tests and change
the internal representation of `Rotation::NEVER` from year 9,999 to an
`Option::None`.

This branch changes `tracing-appender`'s MSRV from Rust 1.42 to Rust
1.51, the MSRV for the `time` crate when certain required feature flags
are enabled. This does *not* effect the MSRV for other crates in this
repository.
hawkw added a commit that referenced this pull request Oct 22, 2021
# 0.2.0 (October 22, 2021)

This breaking change release adds support for the new v0.3.x series of
`tracing-subscriber`. In addition, it resolves the security advisory for
the `chrono` crate, [RUSTSEC-2020-0159].

This release increases the minimum supported Rust version (MSRV) to
1.51.0.
### Breaking Changes

- Updated `tracing-subscriber` to v0.3.x ([#1677])
- Changed `NonBlocking::error_counter` to return an `ErrorCounter` type,
  rather than an `Arc<AtomicU64>` ([#1675])
  ### Changed

- Updated `tracing-subscriber` to v0.3.x ([#1677])
  ### Fixed

- **non-blocking**: Fixed compilation on 32-bit targets ([#1675])
- **rolling**: Replaced `chrono` dependency with `time` to resolve
  [RUSTSEC-2020-0159] ([#1652])
- **rolling**: Fixed an issue where `RollingFileAppender` would fail to
  print errors that occurred while flushing a previous logfile ([#1604])

Thanks to new contributors @dzvon and @zvkemp for contributing to this
release!

[RUSTSEC-2020-0159]: https://rustsec.org/advisories/RUSTSEC-2020-0159.html
[#1677]: #1677
[#1675]: #1675
[#1652]: #1675
[#1604]: #1604
hawkw added a commit that referenced this pull request Oct 23, 2021
# 0.2.0 (October 22, 2021)

This breaking change release adds support for the new v0.3.x series of
`tracing-subscriber`. In addition, it resolves the security advisory for
the `chrono` crate, [RUSTSEC-2020-0159].

This release increases the minimum supported Rust version (MSRV) to
1.51.0.
### Breaking Changes

- Updated `tracing-subscriber` to v0.3.x ([#1677])
- Changed `NonBlocking::error_counter` to return an `ErrorCounter` type,
  rather than an `Arc<AtomicU64>` ([#1675])
  ### Changed

- Updated `tracing-subscriber` to v0.3.x ([#1677])
  ### Fixed

- **non-blocking**: Fixed compilation on 32-bit targets ([#1675])
- **rolling**: Replaced `chrono` dependency with `time` to resolve
  [RUSTSEC-2020-0159] ([#1652])
- **rolling**: Fixed an issue where `RollingFileAppender` would fail to
  print errors that occurred while flushing a previous logfile ([#1604])

Thanks to new contributors @dzvon and @zvkemp for contributing to this
release!

[RUSTSEC-2020-0159]: https://rustsec.org/advisories/RUSTSEC-2020-0159.html
[#1677]: #1677
[#1675]: #1675
[#1652]: #1675
[#1604]: #1604
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
This PR continues the work started in
tokio-rs#1646 to replace `chrono` with
`time`. I'll refer to @hawkw's motivation:

> Currently, `tracing-subscriber` supports the `chrono` crate for
> timestamp formatting, via a default-on feature flag. When this code
> was initially added to `tracing-subscriber`, the `time` crate did not
> have support for the timestamp formatting options we needed.
>
> Unfortunately, the `chrono` crate's maintenance status is now in
> question (see tokio-rs#1598). Furthermore, `chrono` depends on version 0.1 of
> the `time` crate, which contains a security vulnerability
> (https://rustsec.org/advisories/RUSTSEC-2020-0071.html). This
> vulnerability is fixed in more recent releases of `time`, but `chrono`
> still uses v0.1.

I've replaced chrono with time 0.3. Unfortunately, some of chrono's
builders for DateTimes are not present in `time`, which required the
usage of `macro_rules!` macros to construct some of the hard-coded
times. I also took the opportunity to tidy some of the tests and change
the internal representation of `Rotation::NEVER` from year 9,999 to an
`Option::None`.

This branch changes `tracing-appender`'s MSRV from Rust 1.42 to Rust
1.51, the MSRV for the `time` crate when certain required feature flags
are enabled. This does *not* effect the MSRV for other crates in this
repository.
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.2.0 (October 22, 2021)

This breaking change release adds support for the new v0.3.x series of
`tracing-subscriber`. In addition, it resolves the security advisory for
the `chrono` crate, [RUSTSEC-2020-0159].

This release increases the minimum supported Rust version (MSRV) to
1.51.0.
### Breaking Changes

- Updated `tracing-subscriber` to v0.3.x ([tokio-rs#1677])
- Changed `NonBlocking::error_counter` to return an `ErrorCounter` type,
  rather than an `Arc<AtomicU64>` ([tokio-rs#1675])
  ### Changed

- Updated `tracing-subscriber` to v0.3.x ([tokio-rs#1677])
  ### Fixed

- **non-blocking**: Fixed compilation on 32-bit targets ([tokio-rs#1675])
- **rolling**: Replaced `chrono` dependency with `time` to resolve
  [RUSTSEC-2020-0159] ([tokio-rs#1652])
- **rolling**: Fixed an issue where `RollingFileAppender` would fail to
  print errors that occurred while flushing a previous logfile ([tokio-rs#1604])

Thanks to new contributors @dzvon and @zvkemp for contributing to this
release!

[RUSTSEC-2020-0159]: https://rustsec.org/advisories/RUSTSEC-2020-0159.html
[tokio-rs#1677]: tokio-rs#1677
[tokio-rs#1675]: tokio-rs#1675
[tokio-rs#1652]: tokio-rs#1675
[tokio-rs#1604]: tokio-rs#1604
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants