feat: Enhance logging with requestIds for traceability #455
Conversation
Signed-off-by: Dylan Kilkenny <[email protected]>
WalkthroughSwapped simplelog for the tracing ecosystem; added logging constants, tracing-based logger (stdout/file/rolling/JSON/pretty), observability modules (request_id, RequestIdMiddleware, job_tracing macro), attached request IDs to Jobs, instrumented job handlers, and updated server middleware and auth flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant App as Actix App
participant RID as RequestIdMiddleware
participant TL as TracingLogger
participant Auth as Auth Check
participant H as Handler
C->>App: HTTP request
App->>RID: transform(request)
RID->>RID: resolve/set request_id (header | ext | UUID)
RID->>TL: forward request (request_id in context)
TL->>Auth: evaluate is_public + check_authorization_header
alt authorized
Auth-->>H: proceed (context contains request_id)
H-->>C: 2xx/4xx/5xx (+ x-request-id)
else unauthorized
Auth-->>App: unauthorized
App-->>C: 401 JSON (+ x-request-id)
end
sequenceDiagram
autonumber
participant Web as HTTP handler
participant Obs as observability::request_id
participant Prod as JobProducer
participant Q as Queue
participant W as Worker
Web->>Obs: set_request_id(x-request-id)
Web->>Prod: produce job
Prod->>Obs: get_request_id()
Obs-->>Prod: Option<String>
Prod->>Q: enqueue `Job::new(...).with_request_id(rid)`
W->>Q: dequeue(job)
W->>W: setup_job_tracing!(job, attempt) -- enters span with request_id
W-->>W: process job (traced)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/jobs/handlers/transaction_submission_handler.rs (1)
53-56: Usecancel_transactionin Cancel branch
Replace the call torelayer_transaction.submit_transaction(transaction).await?with
relayer_transaction.cancel_transaction(transaction).await?(thecancel_transactionAPI only takes the transaction).src/jobs/job_producer.rs (2)
52-66: Clone implementation can panic; prefer Arc<Mutex>
try_lock().expect(...)may panic under load. Store the queue inArc<Mutex<_>>and derive/implementCloneby cloning theArc.Apply:
@@ -#[derive(Debug)] -pub struct JobProducer { - queue: Mutex<Queue>, -} +#[derive(Debug, Clone)] +pub struct JobProducer { + queue: std::sync::Arc<Mutex<Queue>>, +} @@ -impl Clone for JobProducer { - fn clone(&self) -> Self { - // We can't clone the Mutex directly, but we can create a new one with a cloned Queue - // This requires getting the lock first - let queue = self - .queue - .try_lock() - .expect("Failed to lock queue for cloning") - .clone(); - - Self { - queue: Mutex::new(queue), - } - } -} +// Custom Clone no longer needed due to Arc; remove impl @@ impl JobProducer { pub fn new(queue: Queue) -> Self { - Self { - queue: Mutex::new(queue.clone()), - } + Self { + queue: std::sync::Arc::new(Mutex::new(queue)), + } } }
123-127: Use tracing macros and include request_id in logsTo preserve span context and correlate logs, switch to
tracingmacros and addrequest_idas a field.- use log::{error, info}; + use tracing::{error, info}; @@ - info!( - "Producing transaction request job: {:?}", - transaction_process_job - ); + info!(request_id = ?get_request_id(), job = ?transaction_process_job, "Producing transaction request job"); @@ - info!("Transaction job produced successfully"); + info!(request_id = ?get_request_id(), "Transaction Request job produced"); @@ - info!("Transaction Submit job produced successfully"); + info!(request_id = ?get_request_id(), "Transaction Submit job produced"); @@ - info!("Transaction Status Check job produced successfully"); + info!(request_id = ?get_request_id(), "Transaction Status Check job produced"); @@ - info!("Notification Send job produced successfully"); + info!(request_id = ?get_request_id(), "Notification Send job produced"); @@ - info!("Solana token swap job produced successfully"); + info!(request_id = ?get_request_id(), "Solana token swap job produced");Also applies to: 142-145, 164-167, 188-190, 210-212, 235-237
🧹 Nitpick comments (16)
src/jobs/job.rs (1)
36-39: Add a focused test for the builder and serialization behavior.Assert that
with_request_id(Some(...))sets the field and that the None case omits the field from JSON (after applying the skip-serialize change).Example test to add:
#[test] fn test_job_with_request_id() { let job = Job::new(JobType::TransactionRequest, TransactionRequest::new("tx", "rel")) .with_request_id(Some("req-123".to_string())); assert_eq!(job.request_id.as_deref(), Some("req-123")); let json = serde_json::to_value(&job).unwrap(); assert_eq!(json.get("request_id").unwrap(), "req-123"); let none_json = serde_json::to_value( &Job::new(JobType::TransactionRequest, TransactionRequest::new("tx", "rel")) ).unwrap(); assert!(none_json.get("request_id").is_none()); }src/observability/request_id.rs (1)
18-30: Also search ancestor spans for a propagated request ID.When the current span doesn’t carry the extension (common if set on an outer HTTP/request span), walk the scope to the nearest ancestor that has it.
SpanRef::scope()allows traversing parents; prefer nearest ancestor to keep locality. (docs.rs)pub fn get_request_id() -> Option<String> { - let mut out = None; - Span::current().with_subscriber(|(span_id, subscriber)| { - if let Some(reg) = subscriber.downcast_ref::<Registry>() { - if let Some(span_ref) = reg.span(span_id) { - if let Some(r) = span_ref.extensions().get::<RequestId>() { - out = Some(r.0.clone()); - } - } - } - }); - out + let mut out = None; + Span::current().with_subscriber(|(span_id, subscriber)| { + if let Some(reg) = subscriber.downcast_ref::<Registry>() { + if let Some(span_ref) = reg.span(span_id) { + // Check current span first. + if let Some(r) = span_ref.extensions().get::<RequestId>() { + out = Some(r.0.clone()); + return; + } + // Then check parents (nearest first). + for s in span_ref.scope() { + if let Some(r) = s.extensions().get::<RequestId>() { + out = Some(r.0.clone()); + break; + } + } + } + } + }); + out }src/constants/logging.rs (2)
18-22: Normalize DOCKER_LOG_DIR; avoid trailing slash for consistent path joiningThe trailing slash on DOCKER_LOG_DIR is inconsistent with DEFAULT_LOG_DIR and can bite if paths are concatenated with string formatting instead of Path::join.
- pub const DOCKER_LOG_DIR: &str = "logs/"; + pub const DOCKER_LOG_DIR: &str = "logs";
9-14: Use typed config (enum) for format/mode (and validate inputs) instead of free-form stringsA small enum for format (compact/pretty/json) and mode (stdout/file) prevents invalid values from slipping in and simplifies downstream matching.
If you’d like, I can sketch a tiny serde-backed Config with enums and a FromEnv loader.
src/jobs/handlers/solana_swap_request_handler.rs (3)
33-34: Nice: per-job span initialization; align logs with tracing and add structured fieldsGreat to see setup_job_tracing!(job, attempt). To maximize value:
- Prefer tracing::info! over log::info! so events inherit span fields (request_id, job_type, attempt).
- Emit structured fields instead of {:?} dumps (can be noisy and leak data).
Example (no diff, illustrative):
use tracing::info; // … info!(relayer_id=%job.data.relayer_id, attempt=%attempt.current(), "handling solana token swap request");
35-36: Reduce duplicate “handling …” logs; keep one at handler or inside handle_requestBoth the handler and handle_request log essentially the same message. Keep one (prefer handler with span context) and downgrade the other to debug! or remove.
51-57: Instrument cron handler with a span to keep logs correlatedCron jobs don’t have Job, but you can still create a span:
use tracing::info_span; let span = info_span!("job", job_type="SolanaTokenSwapCron", attempt=%attempt.current()); let _e = span.enter();src/jobs/handlers/transaction_status_handler.rs (2)
26-27: Good span setup; switch to tracing macros and structured fieldsReplace log::info! with tracing::info! and add fields (transaction_id, relayer_id) so they appear under the job span.
99-108: Tests: make Attempt bindings mutable before increment()These calls will require mutable bindings.
- let second_attempt = Attempt::default(); + let mut second_attempt = Attempt::default(); second_attempt.increment(); - let final_attempt = Attempt::default(); + let mut final_attempt = Attempt::default(); for _ in 0..WORKER_DEFAULT_MAXIMUM_RETRIES { final_attempt.increment(); }src/lib.rs (1)
35-35: Update crate docs to list the new observability moduleThe module list at the top omits observability; add a bullet describing request_id, middleware, and job_tracing for discoverability.
I can push a quick doc patch if you’d like.
src/jobs/handlers/transaction_request_handler.rs (1)
27-28: Good: job span; refine logging level and structure to avoid noise/PII
- Switch log::info! → tracing::info!/debug! so span fields (request_id, attempt) flow.
- Avoid {:?} dumps of job.data/ctx/worker at info level; either emit selected fields or lower to debug.
Example (illustrative):
use tracing::{info, debug, instrument}; info!(tx_id=%job.data.transaction_id, relayer_id=%job.data.relayer_id, attempt=%attempt.current(), "handling transaction request"); debug!(?worker, %task_id, ?ctx, "worker context");Optionally, annotate handle_request:
#[instrument(skip(state), fields(tx_id=%request.transaction_id, relayer_id=%request.relayer_id))] async fn handle_request(/* ... */) -> Result<()> { /* ... */ }src/constants/mod.rs (1)
41-43: LGTM: constants/logging re-export is clear and scoped.Publicly exposing logging defaults from
constants::logginglooks good. Watch for name collisions withcrate::loggingmodule imports in callers.src/jobs/job_producer.rs (1)
313-337: Add tests asserting request_id propagationAdd one test that sets a request_id on the current span and asserts the produced
Job<T>.request_id.is_some(). This guards against regressions in middleware ordering.I can draft a
#[tokio::test]that usestracing::info_span!+set_request_id("test-id")and verifies the field afterproduce_*.Also applies to: 338-356, 358-379, 381-399, 401-425, 428-452, 454-467, 469-482, 484-522
src/observability/middleware.rs (1)
11-13: Derive Copy/Clone/Default for middlewareZero-sized middleware can be
#[derive(Copy, Clone, Default)]for ergonomics.-/// Middleware that adds request ID tracking to all HTTP requests -pub struct RequestIdMiddleware; +/// Middleware that adds request ID tracking to all HTTP requests +#[derive(Copy, Clone, Default)] +pub struct RequestIdMiddleware;src/main.rs (2)
152-158: Align metrics server with main middleware (tracing, request_id, metrics, order)Add the same stack so metrics endpoints are logged, traced, and carry
x-request-id.- HttpServer::new(|| { - App::new() - .wrap(middleware::Compress::default()) - .wrap(middleware::NormalizePath::trim()) - .wrap(middleware::DefaultHeaders::new()) - .configure(api::routes::metrics::init) - }) + HttpServer::new(|| { + App::new() + .wrap(TracingLogger::default()) + .wrap(RequestIdMiddleware) + .wrap(MetricsMiddleware) + .wrap(middleware::DefaultHeaders::new()) + .wrap(middleware::NormalizePath::trim()) + .wrap(middleware::Compress::default()) + .configure(api::routes::metrics::init) + })
95-101: Use tracing macros consistentlyReplace
log::info!usages withtracing::info!for span‑aware logs.- info!("Initializing relayers"); + tracing::info!("Initializing relayers"); @@ - info!("Starting server on {}:{}", config.host, config.port); + tracing::info!("Starting server on {}:{}", config.host, config.port); @@ - log::info!("Metrics server enabled, starting metrics server..."); + tracing::info!("Metrics server enabled, starting metrics server..."); @@ - log::info!("Metrics server disabled"); + tracing::info!("Metrics server disabled");Also applies to: 109-111, 150-173
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.toml(1 hunks)src/constants/logging.rs(1 hunks)src/constants/mod.rs(1 hunks)src/jobs/handlers/notification_handler.rs(2 hunks)src/jobs/handlers/solana_swap_request_handler.rs(2 hunks)src/jobs/handlers/transaction_request_handler.rs(2 hunks)src/jobs/handlers/transaction_status_handler.rs(2 hunks)src/jobs/handlers/transaction_submission_handler.rs(2 hunks)src/jobs/job.rs(2 hunks)src/jobs/job_producer.rs(6 hunks)src/lib.rs(1 hunks)src/logging/mod.rs(2 hunks)src/main.rs(3 hunks)src/observability/job_tracing.rs(1 hunks)src/observability/middleware.rs(1 hunks)src/observability/mod.rs(1 hunks)src/observability/request_id.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/observability/middleware.rs (1)
src/observability/request_id.rs (1)
set_request_id(7-16)
src/observability/job_tracing.rs (1)
src/observability/request_id.rs (1)
set_request_id(7-16)
src/main.rs (3)
src/utils/auth.rs (1)
check_authorization_header(13-34)src/logging/mod.rs (1)
setup_logging(68-208)src/observability/middleware.rs (1)
req(48-50)
src/jobs/job_producer.rs (2)
src/observability/request_id.rs (1)
get_request_id(18-30)src/jobs/job.rs (5)
new(26-35)new(62-68)new(151-157)new(172-177)new(186-188)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: clippy
- GitHub Check: test
- GitHub Check: msrv
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (rust)
🔇 Additional comments (7)
src/observability/mod.rs (1)
1-6: Good modularization and re-export.Clean public surface for observability;
RequestIdMiddlewarere-export is handy for app wiring.src/jobs/job_producer.rs (1)
128-130: Request ID propagation onto Job — LGTMChaining
.with_request_id(get_request_id())on all produced jobs is correct and side‑effect free whenNone.Also applies to: 153-155, 175-180, 198-200, 220-222
src/main.rs (1)
118-141: Middleware order breaks tracing/metrics/rate‑limit for unauthorized requests and request_id attachment
wrap_fnshort‑circuits beforeTracingLogger,MetricsMiddleware,Governor, andRequestIdMiddleware, so unauthorized responses aren’t logged/metric’d/rate‑limited and don’t getx-request-id. Reorder as below.- app - .wrap_fn(move |req, srv| { + app + .wrap(TracingLogger::default()) + .wrap(RequestIdMiddleware) + .wrap(MetricsMiddleware) + .wrap(Governor::new(&rate_limit_config)) + .wrap(middleware::DefaultHeaders::new()) + .wrap(middleware::NormalizePath::trim()) + .wrap(middleware::Compress::default()) + .wrap_fn(move |req, srv| { let path = req.path(); let is_public = PUBLIC_ENDPOINTS.iter().any(|p| path.starts_with(p)); let authorized = is_public || check_authorization_header(&req, &config.api_key); if authorized { Either::Left(srv.call(req)) } else { - let res = HttpResponse::Unauthorized() + let res = HttpResponse::Unauthorized() .body(r#"{"success":false,"code":401,"error":"Unauthorized","message":"Unauthorized"}"#) .map_into_boxed_body(); Either::Right(ready(Ok(req.into_response(res)))) } }) - .wrap(RequestIdMiddleware) - .wrap(TracingLogger::default()) - .wrap(MetricsMiddleware) - .wrap(Governor::new(&rate_limit_config)) - .wrap(middleware::DefaultHeaders::new()) - .wrap(middleware::NormalizePath::trim()) - .wrap(middleware::Compress::default()) .app_data(app_state.clone())Likely an incorrect or invalid review comment.
src/logging/mod.rs (4)
121-164: LGTM! Comprehensive format support with proper layer configuration.The format handling provides three well-configured options:
- Pretty format: Enhanced readability with thread IDs, file locations, and line numbers
- JSON format: Structured logging with span context for machine processing
- Compact format: Clean default format with reduced verbosity
Each format is properly configured with appropriate tracing layers including ErrorLayer for error context.
167-204: LGTM! Consistent stdout logging implementation.The stdout logging branch mirrors the file logging format options with identical configuration, ensuring consistent behavior between logging modes. The layer setup is appropriate for each format type.
207-207: LGTM! Structured completion logging.The final log message uses structured logging with field formatting (
mode=%log_mode, format=%format), which is consistent with the tracing ecosystem and provides useful initialization confirmation.
284-334: Smart test design handling single logger initialization.The test correctly handles the constraint that tracing subscribers can only be initialized once per process using
std::sync::Once. The approach of testing both stdout and file configurations within a single test while usingINIT_LOGGER.call_once()is pragmatic and avoids initialization conflicts.The file configuration test focuses on the directory creation logic without reinitializing the logger, which is a sensible approach.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/observability/middleware.rs (1)
47-60: Correct priority and safe parsing — nice.Header precedence, fallback to Actix’s ID, and UUID generation look good;
to_str().ok()avoids panics and meets prior feedback.
🧹 Nitpick comments (5)
src/observability/request_id.rs (2)
7-16: Optionally also record the ID into the span field (if present).This helps formatters/exporters that rely on span fields rather than extensions.
pub fn set_request_id(id: impl Into<String>) { - let id = id.into(); + let id = id.into(); + let id_for_record = id.clone(); Span::current().with_subscriber(|(span_id, subscriber)| { if let Some(reg) = subscriber.downcast_ref::<Registry>() { if let Some(span_ref) = reg.span(span_id) { span_ref.extensions_mut().replace(RequestId(id)); } } }); + // If the current span defines a `request_id` field, record it for log output. + if let Some(meta) = Span::current().metadata() { + if let Some(field) = meta.fields().field("request_id") { + Span::current().record(field, &tracing::field::display(&id_for_record)); + } + } }
38-67: Test a no-span scenario for clarity.Add a test asserting
get_request_id()returnsNonewhen called outside any span; documents the no-op behavior.src/observability/middleware.rs (3)
65-74: x-request-id not added when inner service returns Err.If
self.service.call(req)yieldsErr(Error), this middleware returns early and the response produced by Actix may miss the header. Decide if the header must be present on all responses (including extractor failures). If yes, consider handling errors viaErrorHandlersto attach the header post-conversion, or restructure to build aServiceResponseonErr(requires changingResponseto anEitherBodyto reconcile body types).Do you require x-request-id on error responses generated from
Err? If so, I can propose a concreteErrorHandlerssnippet.
47-71: Minor: de-duplicate the header name.Define a single constant to avoid string duplication.
-use actix_web::{ +use actix_web::{ dev::{forward_ready, Service, ServiceRequest, ServiceResponse, Transform}, http::header::{HeaderName, HeaderValue}, Error, HttpMessage, }; @@ +const X_REQUEST_ID: &str = "x-request-id"; @@ - let rid = req - .headers() - .get("x-request-id") + let rid = req + .headers() + .get(X_REQUEST_ID) @@ - if let Ok(header_value) = HeaderValue::from_str(&rid) { - res.headers_mut() - .insert(HeaderName::from_static("x-request-id"), header_value); + if let Ok(header_value) = HeaderValue::from_str(&rid) { + res.headers_mut() + .insert(HeaderName::from_static(X_REQUEST_ID), header_value); }
77-162: Add a test for the ActixRequestId fallback and an extractor-error case.
- Ensure we propagate/echo when
TracingLoggerinjects anActixRequestIdand no header is provided.- Decide whether the header should appear when a handler/extractor returns
Err(not justHttpResponse::InternalServerError()), and add a test accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/jobs/job.rs(2 hunks)src/observability/middleware.rs(1 hunks)src/observability/request_id.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/jobs/job.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-10T13:02:08.896Z
Learnt from: dylankilkenny
PR: OpenZeppelin/openzeppelin-relayer#455
File: src/observability/middleware.rs:47-56
Timestamp: 2025-09-10T13:02:08.896Z
Learning: In Actix Web middleware registration, middleware registered later executes earlier due to reverse execution order. When RequestIdMiddleware is registered before TracingLogger, TracingLogger creates the request span first, then RequestIdMiddleware runs within that span.
Applied to files:
src/observability/middleware.rs
📚 Learning: 2025-09-10T13:02:08.896Z
Learnt from: dylankilkenny
PR: OpenZeppelin/openzeppelin-relayer#455
File: src/observability/middleware.rs:47-56
Timestamp: 2025-09-10T13:02:08.896Z
Learning: In Actix Web, middleware is executed in reverse order of registration. If middleware A is registered first, then middleware B, the execution order during request processing is B -> A.
Applied to files:
src/observability/middleware.rs
🧬 Code graph analysis (1)
src/observability/middleware.rs (1)
src/observability/request_id.rs (1)
set_request_id(7-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: msrv
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Analyze (rust)
- GitHub Check: Pages changed - openzeppelin-relayer
🔇 Additional comments (1)
src/observability/request_id.rs (1)
12-12: Idempotent span extension write — good fix.Using
replaceavoids the panic frominsertwhen the extension already exists. This addresses the prior concern cleanly.
Signed-off-by: Dylan Kilkenny <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/main.rs (4)
127-131: 401 JSON body missing Content-Type header.Clients may not treat the body as JSON. Set Content-Type to application/json.
Apply this diff:
- let res = HttpResponse::Unauthorized() - .body(r#"{"success":false,"code":401,"error":"Unauthorized","message":"Unauthorized"}"#) - .map_into_boxed_body(); + use actix_web::http::header::ContentType; + let res = HttpResponse::Unauthorized() + .insert_header(ContentType::json()) + .body(r#"{"success":false,"code":401,"error":"Unauthorized","message":"Unauthorized"}"#) + .map_into_boxed_body();
133-136: Ensure request ID is returned on 401 responses.Given middleware execution order in actix-web, RequestIdMiddleware should run before this auth gate; still, add a test to verify X-Request-Id is present on unauthorized responses.
Example test you can add:
#[actix_web::test] async fn unauthorized_includes_request_id() { use actix_web::{test, web, App, http::header}; let app = test::init_service( App::new() .wrap(RequestIdMiddleware) .wrap_fn(|req, srv| { let res = actix_web::HttpResponse::Unauthorized().finish(); futures::future::Either::Right(futures::future::ready(Ok(req.into_response(res)))) }) .service(web::resource("/").to(|| async { "ok" })), ).await; let req = test::TestRequest::get().uri("/").to_request(); let resp = test::call_service(&app, req).await; assert!(resp.headers().contains_key("x-request-id")); }
138-139: Align middleware order with the metrics server for consistency.Main app uses NormalizePath then Compress; metrics server registers Compress then NormalizePath. Prefer consistent order across servers.
Apply this diff in the metrics server builder:
- .wrap(middleware::Compress::default()) - .wrap(middleware::NormalizePath::trim()) + .wrap(middleware::NormalizePath::trim()) + .wrap(middleware::Compress::default())
44-44: Unify on tracing macros (or bridge log -> tracing).setup_logging configures tracing; log::info! here may be dropped unless LogTracer is installed. Prefer tracing::info! in this crate.
Apply this diff:
- use log::info; + use tracing::info;- log::info!("Metrics server enabled, starting metrics server..."); + info!("Metrics server enabled, starting metrics server...");- log::info!("Metrics server disabled"); + info!("Metrics server disabled");Alternatively, install tracing-log’s LogTracer in setup_logging to capture log crate events from dependencies.
Also applies to: 92-92, 150-150, 171-171
src/logging/mod.rs (4)
7-8: Docs mismatch: LOG_DATA_DIR is a directory, not a full file path.The code joins LOG_DATA_DIR with LOG_FILE_NAME. Update docs to reflect semantics.
Apply this diff:
-//! - LOG_DATA_DIR: when using file mode, the path of the log file (default "logs/relayer.log") +//! - LOG_DATA_DIR: when using file mode, the directory for log files (default "logs/") +//! - LOG_FILE_NAME: base log file name used in file mode (default "relayer.log")
108-115: Simplify file open with OpenOptions::create(true).Avoids pre-check race and reduces branches.
Apply this diff:
- let file = if Path::new(&final_path).exists() { - OpenOptions::new() - .append(true) - .open(&final_path) - .expect("Failed to open log file") - } else { - File::create(&final_path).expect("Failed to create log file") - }; + let file = OpenOptions::new() + .create(true) + .append(true) + .open(&final_path) + .expect("Failed to open/create log file");
56-63: Minor: rollover threshold and shadowed identifier.
- Consider rolling when size >= max_size to avoid writing exactly-at-limit files.
- Rename the local metadata binding to avoid shadowing the imported fn.
Apply this diff:
- while let Ok(metadata) = metadata(&final_path) { - if metadata.len() > max_size { + while let Ok(meta) = metadata(&final_path) { + if meta.len() >= max_size { final_path = compute_rolled_file_path(base_file_path, date_str, index); index += 1; } else { break; } }
67-75: Optional: bridge log crate records into tracing.This ensures logs from dependencies using the log crate appear in tracing output.
Add the following near the start of setup_logging and include the dependency tracing-log:
pub fn setup_logging() { + // Forward `log` records to `tracing` + let _ = tracing_log::LogTracer::init();Cargo.toml:
[dependencies] tracing-log = "0.2"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.toml(1 hunks)src/logging/mod.rs(2 hunks)src/main.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (1)
src/main.rs (3)
src/utils/auth.rs (1)
check_authorization_header(13-34)src/logging/mod.rs (1)
setup_logging(68-211)src/observability/middleware.rs (1)
req(55-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: msrv
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
src/main.rs (2)
30-30: Imports and observability wiring look good.Adding HttpResponse, Either/ready, RequestIdMiddleware, and TracingLogger aligns with the new tracing stack.
Also applies to: 33-33, 39-39, 57-57, 60-60
69-75: Good: initialize tracing before installing color-eyre.This ensures ErrorLayer is present for richer reports.
src/logging/mod.rs (1)
120-167: Subscriber composition looks correct.EnvFilter + ErrorLayer + fmt layers are wired properly for both file and stdout modes; non_blocking guard is kept alive. Good.
Also applies to: 169-207, 210-210
| let is_public = PUBLIC_ENDPOINTS.iter().any(|p| path.starts_with(p)); | ||
| let authorized = is_public || check_authorization_header(&req, &config.api_key); | ||
|
|
There was a problem hiding this comment.
Security: prefix match for public endpoints can unintentionally expose private routes.
starts_with will treat /api/v1/healthz as public if "/api/v1/health" is listed. Use a segment‑boundary check (exact match or prefix followed by '/').
Apply this diff and helper:
- let is_public = PUBLIC_ENDPOINTS.iter().any(|p| path.starts_with(p));
- let authorized = is_public || check_authorization_header(&req, &config.api_key);
+ let is_public = PUBLIC_ENDPOINTS.iter().any(|p| has_path_prefix(path, p));
+ let authorized = is_public || check_authorization_header(&req, &config.api_key);Add this helper (place near other module‑local fns):
fn has_path_prefix(path: &str, prefix: &str) -> bool {
let p = prefix.trim_end_matches('/');
path == p || path.starts_with(p) && path.as_bytes().get(p.len()) == Some(&b'/')
}🤖 Prompt for AI Agents
In src/main.rs around lines 121 to 123, the current prefix check uses
starts_with which can incorrectly mark routes like "/api/v1/healthz" as public
when "/api/v1/health" is listed; add the helper function has_path_prefix near
the other module-local functions and replace the PUBLIC_ENDPOINTS check so it
uses has_path_prefix(path, p) for each prefix (i.e., compute is_public =
PUBLIC_ENDPOINTS.iter().any(|p| has_path_prefix(path, p))); the helper should
trim trailing slashes on the prefix, return true for exact matches, and
otherwise ensure path.starts_with(prefix) only counts if the next byte is '/' to
enforce segment boundaries.
Signed-off-by: Dylan Kilkenny <[email protected]>
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (60.5%) is below the target coverage (90.0%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #455 +/- ##
=======================================
- Coverage 93.0% 93.0% -0.1%
=======================================
Files 222 225 +3
Lines 77105 77270 +165
=======================================
+ Hits 71758 71862 +104
- Misses 5347 5408 +61
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:
|
dylankilkenny
deleted the
plat-6830-implement-correlation-ids-in-logging-for-end-to-end
branch
Summary
tracingandtracing-subscriberfor improved log management.Testing Process
Checklist
Summary by CodeRabbit
New Features
Behavior
Refactor