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

Switch from the chrono clock feature to now #1569

Merged
merged 1 commit into from Feb 24, 2024

Conversation

edmorley
Copy link
Contributor

@edmorley edmorley commented Feb 23, 2024

In #1192, chrono was added as a dependency of the opentelemetry-stdout crate in order to support outputting timestamps in human readable format.

In that PR, all Chrono features were disabled apart from the clock feature.

However, since that change landed, chrono v0.4.32 has added support for an even finer-grained feature named now, which is a subset of the clock feature - that excludes timezone support, and so avoids pulling in many timezone related crates.

opentelemetry-stdout only uses chrono's UTC features, so we can safely switch from using the clock feature to using now instead.

After this change, the following transitive dependencies are no longer pulled in:

  • android-tzdata
  • android_system_properties
  • cc
  • core-foundation-sys
  • iana-time-zone
  • iana-time-zone-haiku
  • windows-core
  • windows-targets
  • windows_aarch64_gnullvm
  • windows_aarch64_msvc
  • windows_i686_gnu
  • windows_i686_msvc
  • windows_x86_64_gnu
  • windows_x86_64_gnullvm
  • windows_x86_64_msvc

See:
chronotope/chrono#1343
https://github.com/chronotope/chrono/blob/main/README.md#crate-features

@edmorley edmorley requested a review from a team as a code owner February 23, 2024 13:00
Copy link

linux-foundation-easycla bot commented Feb 23, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: edmorley / name: Ed Morley (0320780)

In open-telemetry#1192, `chrono` was added as a dependency of the `opentelemetry-stdout`
crate in order to support outputting timestamps in human readable format.

In that PR, all Chrono features were disabled apart from the `clock` feature.

However, since that change landed, `chrono` has added support for an even
finer-grained feature named `now`, which is a subset of the `clock` feature which
excludes timezone support, and so avoids pulling in many timezone related crates.

`opentelemetry-stdout` only uses chrono's UTC features, so we can switch
from using the `clock` feature to using `now` instead.

After this change, the following transitive dependencies are no longer pulled in:

- `android-tzdata`
- `android_system_properties`
- `cc`
- `core-foundation-sys`
- `iana-time-zone`
- `iana-time-zone-haiku`
- `windows-core`
- `windows-targets`
- `windows_aarch64_gnullvm`
- `windows_aarch64_msvc`
- `windows_i686_gnu`
- `windows_i686_msvc`
- `windows_x86_64_gnu`
- `windows_x86_64_gnullvm`
- `windows_x86_64_msvc`

See:
chronotope/chrono#1343
https://github.com/chronotope/chrono/blob/main/README.md#crate-features
Copy link
Contributor

@shaun-cox shaun-cox left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Thanks a lot! ❤️

@edmorley
Copy link
Contributor Author

The coverage CI job is failing for unrelated errors in another crate - any ideas?

 ---- exporter::config::collector::tests::test_set_collector_endpoint stdout ----
thread 'exporter::config::collector::tests::test_set_collector_endpoint' panicked at opentelemetry-jaeger/src/exporter/config/collector/mod.rs:575:9:
assertion `left == right` failed
  left: "ConfigError { pipeline_name: \"collector\", config_name: \"collector_endpoint\", reason: \"invalid uri from environment variable, invalid uri character\" }"
 right: "ConfigError { pipeline_name: \"collector\", config_name: \"collector_endpoint\", reason: \"invalid uri from the builder, invalid format\" }"

@hdost
Copy link
Contributor

hdost commented Feb 24, 2024

We have a bug open for it, it's a flaky test, don't worry.

@hdost hdost merged commit 188a26c into open-telemetry:main Feb 24, 2024
12 of 13 checks passed
@cijothomas
Copy link
Member

@hdost We should not merge PRs while release is pending. We need to resolve conflicts again in #1539

@hdost
Copy link
Contributor

hdost commented Feb 25, 2024

@hdost We should not merge PRs while release is pending. We need to resolve conflicts again in #1539

There were no conflicts from this merge. We were still waiting on the no defaults PR #1562 at the time, so even if there were conflicts it would have been 1 small line diff and as this is a leaf had no downstream affects on other crates.

In general, I agree we need to establish some sort of process, but there was only a net good considering how often we get asks for releases.

@cijothomas
Copy link
Member

@hdost We should not merge PRs while release is pending. We need to resolve conflicts again in #1539

There were no conflicts from this merge. We were still waiting on the no defaults PR #1562 at the time, so even if there were conflicts it would have been 1 small line diff and as this is a leaf had no downstream affects on other crates.

In general, I agree we need to establish some sort of process, but there was only a net good considering how often we get asks for releases.

Yes. I only meant to re-highlight the need to establish a process. Fully agree with your reasoning for merging this.

@edmorley edmorley deleted the chrono-features-now branch February 25, 2024 20:13
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.

None yet

4 participants