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

chore: use tracing instead of log crate #195

Merged
merged 1 commit into from
Jun 23, 2024
Merged

Conversation

joshka
Copy link
Contributor

@joshka joshka commented Jun 21, 2024

Fixes: #194

@joshka joshka deleted the branch Smithay:master June 21, 2024 09:26
@joshka joshka closed this Jun 21, 2024
@joshka joshka deleted the master branch June 21, 2024 09:26
@joshka joshka restored the master branch June 21, 2024 09:26
@joshka
Copy link
Contributor Author

joshka commented Jun 21, 2024

Tracing logs for a sample app that hits these -
image

vs without tracing:

image

@joshka joshka reopened this Jun 21, 2024
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

My only hesitancy is for the long-term maintenance of the tracing crate. I know that there was a period where tracing was effectively unmaintained. Looking at the repo it looks like it's maintained better now, but I'm just worried about needing to rollback to log if trading ever becomes unmaintained again.

Looks good to me otherwise.

@joshka
Copy link
Contributor Author

joshka commented Jun 21, 2024

Fixed a log::warn that I'd missed. The CI still breaks due to the log downgrade. The MSRV of the current version of log (1.60.0) seems to be lower than calloop's MSRV (1.63.0). Does this mean that this is not necessary any more?

My only hesitancy is for the long-term maintenance of the tracing crate. I know that there was a period where tracing was effectively unmaintained. Looking at the repo it looks like it's maintained better now, but I'm just worried about needing to rollback to log if trading ever becomes unmaintained again.

The other thing I noted (in #194 in case you missed it) is that this brings back in the proc-macro2 and syn crates to the dependency list (which I saw you were trying to remove by avoiding thiserror).

@notgull
Copy link
Member

notgull commented Jun 22, 2024

The CI still breaks due to the log downgrade. The MSRV of the current version of log (1.60.0) seems to be lower than calloop's MSRV (1.63.0). Does this mean that this is not necessary any more?

Yes, it's probably not necessary.

The other thing I noted (in #194 in case you missed it) is that this brings back in the proc-macro2 and syn crates to the dependency list (which I saw you were trying to remove by avoiding thiserror).

If you import tracing without default features it doesn't have these dependencies though, right?

@joshka
Copy link
Contributor Author

joshka commented Jun 22, 2024

If you import tracing without default features it doesn't have these dependencies though, right?

Actually yep, you're right - I must have checked before I added that

calloop v0.14.0 (/Users/joshka/local/calloop)
├── bitflags v2.5.0
├── polling v3.7.2
│   ├── cfg-if v1.0.0
│   ├── rustix v0.38.34
│   │   ├── bitflags v2.5.0
│   │   ├── errno v0.3.9
│   │   │   └── libc v0.2.155
│   │   └── libc v0.2.155
│   └── tracing v0.1.40
│       ├── log v0.4.21
│       ├── pin-project-lite v0.2.14
│       └── tracing-core v0.1.32
├── rustix v0.38.34 (*)
├── slab v0.4.9
│   [build-dependencies]
│   └── autocfg v1.3.0
└── tracing v0.1.40 (*)

@notgull
Copy link
Member

notgull commented Jun 22, 2024

Please rebase on master to fix the CI issues. Then we should be good to go.

@joshka
Copy link
Contributor Author

joshka commented Jun 23, 2024

Please rebase on master to fix the CI issues. Then we should be good to go.

Done

Copy link

codecov bot commented Jun 23, 2024

Codecov Report

Attention: Patch coverage is 23.07692% with 20 lines in your changes missing coverage. Please review.

Project coverage is 86.39%. Comparing base (5433747) to head (3ce54aa).

Files Patch % Lines
src/loop_logic.rs 25.00% 15 Missing ⚠️
src/sources/ping/eventfd.rs 0.00% 2 Missing ⚠️
src/sources/signals.rs 0.00% 2 Missing ⚠️
src/sources/mod.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
+ Coverage   85.55%   86.39%   +0.84%     
==========================================
  Files          13       15       +2     
  Lines        1862     2051     +189     
==========================================
+ Hits         1593     1772     +179     
- Misses        269      279      +10     
Flag Coverage Δ
macos-latest 85.71% <27.27%> (?)
ubuntu-latest 85.99% <20.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Thanks!

@notgull notgull merged commit 3ef1bf2 into Smithay:master Jun 23, 2024
13 of 14 checks passed
@notgull notgull mentioned this pull request Sep 6, 2024
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.

tracing vs log
2 participants