feat(tracing): migrate logging to tracing#1308
feat(tracing): migrate logging to tracing#1308ognis1205 wants to merge 17 commits intoorhun:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1308 +/- ##
==========================================
+ Coverage 47.40% 47.41% +0.01%
==========================================
Files 24 25 +1
Lines 2131 2135 +4
==========================================
+ Hits 1010 1012 +2
- Misses 1121 1123 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
10 seconds ago I was happy that my GitHub notifications were clear and then Shingo comes in with a crazy PR!!!!! +0 additions -0 deletions though. Love it. LGTM |
|
planning to do any progress on this soon? (added to 2.12.0 milestone) |
Yeah, I'm planning to open a PR later this week. |
|
yup, 2.12.0 will happen in 2026 :) |
|
Sorry for the delay. I'll start working on this PR today. |
|
No worries at all! |
5bb0f7b to
f6b930d
Compare
f6b930d to
b273940
Compare
b273940 to
b1e9f80
Compare
|
This is ready for review now @orhun . I consider this a first draft, and there are likely areas open for discussion. When you have time, I'd really appreciate your thoughts and feedback. |
75b4d8d to
b53b100
Compare
| static MAX_MODULE_WIDTH: AtomicUsize = AtomicUsize::new(0); | ||
|
|
||
| /// Unicode braille spinner frames used by indicatif. | ||
| const SPINNER_TICKS: &[&str] = &["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]; |
There was a problem hiding this comment.
Would be great if we could keep the old arrow-styled progress indicator (for the first line in the logs at least)
I really like the detailed logging btw!
Let me know when this is ready for review
There was a problem hiding this comment.
Would be great if we could keep the old arrow-styled progress indicator (for the first line in the logs at least)
That should be doable.
We can bring back the old arrow-style progress indicator, but when implemented on top of tracing / span-based instrumentation via tracing-indicatif, it would look slightly different from the previous approach:
If that's acceptable, I'm happy to revert the change accordingly. In addition to the progress indicator itself, we can of course also emit explicit logs at the beginning and end of the processing step.
In an ideal world, I'd like to include a progress indicator with percentages as well. However, with the current design, where spans and logging live inside the core SDK, that starts to get a bit messy from a design perspective. It's technically possible, but I intentionally avoided it for now to keep the core clean.
Let me know when this is ready for review
Regarding ANSI/ASCII color codes on Windows or non-TTY environments: since we were already using colored output before, I think it's reasonable to review this as-is for now.
If you're open to it, I'd really appreciate review feedback on the following points in the context of adding this to git-cliff-core as an SDK:
- Which functions should be annotated with
#[instrument](all vs. a subset) - Whether
spannames should be explicitly set - Which fields are appropriate to capture on spans
From a "keep git-cliff-core as a clean SDK" perspective, another option could be to explore whether progress reporting can be handled in a less hacky way on top of env_logger, if that's feasible. Just putting it out there as an alternative.
59d45cd to
4abd838
Compare
4abd838 to
e003355
Compare
3a03774 to
d86b39e
Compare
6bf6f42 to
c665d67
Compare
orhun
left a comment
There was a problem hiding this comment.
I like how this turned out!
Some points of discussion:
- The spinner gets red quickly I think. Wouldn't be nice to adjust the timing there?
- The main spinner messages seem developer oriented now. Previously, it was something like "fetching data from GitHub..." but now it says things like
add_remote_data,get_github_metadataand so on. I know git-cliff is mostly directed at developers, but I still think that we could avoid programmatic-looking info-level messages. Wdyt?
|
Thanks for the feedback!
No objections here. I agree it turns red a bit too quickly. I'll slow down the timing so the transition happens later.
There is actually a reason for the current behavior. Since I implemented using That said, I don't have any objection to making the messages more human-readable. In the future, when we have a chance to refactor, I'd like to explore the idea of making For now, I agree with your point and will update the messages to be more readable. |
fdc3100 to
b17faa0
Compare
Updated the timing so that the transition now occurs after 32 seconds (8 seconds previously), making the progress display less abrupt.
Implemented a macro to output non-developer friendly progress messages in tracing spans. |
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
…ntics Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
a50976a to
7f6b875
Compare
Description
This PR migrates the logging implementation from
env_loggertotracing.The migration preserves the existing log format while extending it to include span information, enabling more structured and contextual logging without changing the default user experience.
Motivation and Context
This change is motivated by the following discussion and issues:
env_loggertotracing+tracing-subscriber#1268The goal is to modernize
git-cliff's logging infrastructure by adoptingtracing, while maintaining backward compatibility and avoiding unnecessary complexity in the core crate.What Has Changed
git-cliff-corenow depends ontracingbehind a feature flag.logoutput is produced.tracing-based implementation usingtracing-indicatif.tracing, implementing such behavior cleanly would require API changes outside ofgit-cliff-core.How Has This Been Tested?
Screenshots / Logs (if applicable)
N/A
Types of Changes
Checklist: