Skip to content

refactor: migrate to tracing#12458

Merged
bors merged 8 commits intorust-lang:masterfrom
weihanglo:tracing
Aug 7, 2023
Merged

refactor: migrate to tracing#12458
bors merged 8 commits intorust-lang:masterfrom
weihanglo:tracing

Conversation

@weihanglo
Copy link
Member

@weihanglo weihanglo commented Aug 7, 2023 •
edited
Loading

What does this PR try to resolve?

The migrates CARGO_LOG logging from log to tracing.

tracing is already widely used among the ecosystem and rustc adopted it for a while. We've chatted about this a while back in the weekly meeting.

Last week in the office hour, epage, Muscraft, and arlosi have no objection to this when I mentioned this upcoming PR. ehuss seems to looking forward to the migration #12445 (comment). Hence posted this pull request.

How should we test and review this PR?

By commits. And please check if CARGO_LOG works for you.

Largely, a search & replace worked just fine. Only in util/network/http.rs I did a manual fix.

The format is also a bit different. Should we make it compatible? (I don't want to though)

# before
[2023-08-07T10:53:48Z DEBUG cargo::sources::registry::http_remote] loading config
# after
2023-08-07T10:43:47.867106Z DEBUG cargo::sources::registry::http_remote: loading config

By default log enables all levels, whereas the built-in fmt subscriber in tracing-subscriber enables up to INFO only. Is that a thing we need to consider? I do feel like INFO is a better default.

Additional information

-2 dependencies
+11 dependencies

Last year during 1.63.0 release cargo only had ~180 deps, but today it doubles the number to ~337 🥲.

@rustbot
Copy link
Collaborator

rustbot commented Aug 7, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-cache-messages Area: caching of compiler messages A-cfg-expr Area: Platform cfg expressions A-cli Area: Command-line interface, option parsing, etc. A-configuration Area: cargo config files and env vars A-crate-dependencies Area: [dependencies] of any kind A-dep-info Area: dep-info, .d files A-dependency-resolution Area: dependency resolution and the resolver A-documenting-cargo-itself Area: Cargo's documentation A-diagnostics Area: Error and warning messages generated by Cargo itself. A-features2 Area: issues specifically related to the v2 feature resolver A-future-incompat Area: future incompatible reporting A-git Area: anything dealing with git A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-layout Area: target output directory layout, naming, and organization A-lockfile Area: Cargo.lock issues A-manifest Area: Cargo.toml issues A-networking Area: networking issues, curl, etc. A-rebuild-detection Area: rebuild detection and fingerprinting A-registries Area: registries A-registry-authentication Area: registry authentication and authorization (authn authz) A-sparse-registry Area: http sparse registries A-timings Area: timings labels Aug 7, 2023
@weihanglo weihanglo added A-debugging-cargo Area: debugging cargo itself and removed A-cfg-expr Area: Platform cfg expressions A-layout Area: target output directory layout, naming, and organization A-features2 Area: issues specifically related to the v2 feature resolver Command-tree A-dep-info Area: dep-info, .d files A-future-incompat Area: future incompatible reporting A-sparse-registry Area: http sparse registries A-registry-authentication Area: registry authentication and authorization (authn authz) A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. labels Aug 7, 2023
@epage
Copy link
Contributor

epage commented Aug 7, 2023

The format is also a bit different. Should we make it compatible? (I don't want to though)

Compatibility only matters if we intend people to parse it which I don't think we do.

The only other issue is usability which it is probably good enough

epage
epage reviewed Aug 7, 2023
View reviewed changes
epage
epage reviewed Aug 7, 2023
View reviewed changes
@weihanglo
Copy link
Member Author

weihanglo commented Aug 7, 2023

I'd like to call out the other change. By default log enables all levels, whereas the built-in fmt subscriber in tracing-subscriber enables up to INFO only. Is that a thing we need to consider? I do feel like INFO is a better default.

@epage
Copy link
Contributor

epage commented Aug 7, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Aug 7, 2023

📌 Commit 964c083 has been approved by epage

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 7, 2023
@bors
Copy link
Contributor

bors commented Aug 7, 2023

⌛ Testing commit 964c083 with merge b19934c...

bors added a commit that referenced this pull request Aug 7, 2023
@bors
Auto merge of #12458 - weihanglo:tracing, r=epage
b19934c 
refactor: migrate to `tracing`
@bors
Copy link
Contributor

bors commented Aug 7, 2023

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 7, 2023
@weihanglo
Copy link
Member Author

weihanglo commented Aug 7, 2023

Timed out again…

@bors retry

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 7, 2023
@bors
Copy link
Contributor

bors commented Aug 7, 2023

⌛ Testing commit 964c083 with merge 211fd7e...

@bors
Copy link
Contributor

bors commented Aug 7, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 211fd7e to master...

This was referenced Aug 13, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@epage epage epage left review comments

@arlosi arlosi arlosi approved these changes

Assignees

@ehuss ehuss

Labels

A-debugging-cargo Area: debugging cargo itself S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.73.0

Development

Successfully merging this pull request may close these issues.

6 participants

@weihanglo @rustbot @epage @bors @arlosi @ehuss