feat: add orcid-fetcher-logger crate for flexible logging and log file output #10
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* creates the `orcid-fetcher-logger` crate based on `flexi_logger` * replace `tracing` with `orcid-fetcher-logger` * enhance logging functionality: flexible logging + logfile output
Reviewer's GuideThis PR introduces a new orcid-fetcher-logger crate providing console-level configuration and optional file output via flexi_logger, and refactors the orcid-works-cli to replace tracing with standard log macros, add verbosity and log-file flags, and enhance error reporting with UUID-tagged logs. Sequence diagram for logger initialization in orcid-works-clisequenceDiagram
participant main
participant Cli
participant orcid_fetcher_logger
main->>Cli: parse CLI arguments
main->>orcid_fetcher_logger: console_level(verbose, quiet)
main->>orcid_fetcher_logger: init_logger(console_level, log)
orcid_fetcher_logger-->>main: logger initialized or error
Class diagram for updated Cli struct in orcid-works-cliclassDiagram
class Cli {
+id: String
+out: PathBuf
+concurrency: u32
+rate_limit: u32
+user_agent_note: Option<String>
+force_fetch: bool
+verbose: u8
+quiet: u8
+log: Option<PathBuf>
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `crates/orcid-works-cli/src/main.rs:131` </location>
<code_context>
#[tokio::main]
async fn main() -> Result<()> {
- tracing_subscriber::fmt().init();
+ let cli = Cli::parse();
+
+ // initialise logger
</code_context>
<issue_to_address>
Cli is parsed twice, which may lead to inconsistent state or wasted computation.
Pass the parsed CLI arguments from `main` to `run` to avoid duplicate parsing and ensure consistency.
</issue_to_address>
### Comment 2
<location> `crates/orcid-works-cli/src/io.rs:105` </location>
<code_context>
- return Err(e);
+ if let Err(e) = temp.persist(path).map_err(|e| e.error) {
+ let eid = Uuid::new_v4();
+ debug!("E[{eid}] error: e");
+ error!("E[{eid}] atomic rename failure path={}", path.display());
+ return Err(e).with_context(|| {
</code_context>
<issue_to_address>
The debug log prints a literal 'e' instead of the error value.
Change the log statement to use `{e}` so the actual error is printed.
</issue_to_address>
### Comment 3
<location> `crates/orcid-works-cli/src/api.rs:48` </location>
<code_context>
+ }
+ };
if res.error_for_status_ref().is_err() {
let status = res.status();
let body = res.text().await.unwrap_or_default();
</code_context>
<issue_to_address>
The error body is always read, even for large responses, which may impact performance.
Limit the size of the response body read for error reporting, or truncate logged output to reduce memory usage.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
let status = res.status();
let body = res.text().await.unwrap_or_default();
let eid = Uuid::new_v4();
error!("E[{eid}] HTTP {status}");
debug!("E[{eid}] url={url}");
debug!("E[{eid}] response body={body}");
bail!("E[{eid}] HTTP {status}");
=======
use tokio::io::AsyncReadExt;
use std::cmp;
let status = res.status();
// Limit error body to 4096 bytes
let mut body = String::new();
let mut stream = res.bytes_stream();
let mut total_bytes = 0;
const MAX_ERROR_BODY: usize = 4096;
while let Some(chunk) = stream.next().await {
match chunk {
Ok(bytes) => {
let take = cmp::min(MAX_ERROR_BODY - total_bytes, bytes.len());
body.push_str(&String::from_utf8_lossy(&bytes[..take]));
total_bytes += take;
if total_bytes >= MAX_ERROR_BODY {
break;
}
}
Err(_) => break,
}
}
let truncated = total_bytes >= MAX_ERROR_BODY;
let eid = Uuid::new_v4();
error!("E[{eid}] HTTP {status}");
debug!("E[{eid}] url={url}");
if truncated {
debug!("E[{eid}] response body (truncated to {MAX_ERROR_BODY} bytes)={body}");
} else {
debug!("E[{eid}] response body={body}");
}
bail!("E[{eid}] HTTP {status}");
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summaries
This PR overhauls the logging functionality, enabling flexible logging and log file output.
What's changed
crates/orcid-fetcher-loggercrates/orcid-works-clitracingwithorcid-fetcher-loggercrates/orcid-works-modelHow to try it
Summary by Sourcery
Integrate a new flexible logging crate and revamp CLI logging by replacing tracing with log macros, exposing verbosity and log file options, and enhancing error reporting with unique IDs and contextual messages
New Features:
Enhancements:
Documentation: