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.

Sign up for GitHub

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

observability: structured logging for faucet requests #4529

Merged
merged 1 commit into from
Sep 25, 2022
Merged

observability: structured logging for faucet requests #4529

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 14 additions & 28 deletions crates/aptos-faucet/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use aptos_sdk::{
use clap::Parser;
use futures::lock::Mutex;
use reqwest::StatusCode;
use std::{convert::Infallible, fmt, path::PathBuf, sync::Arc};
use std::{convert::Infallible, path::PathBuf, sync::Arc};
use url::Url;
use warp::{http, Filter, Rejection, Reply};

Expand Down Expand Up @@ -195,21 +195,23 @@ pub fn routes(
health
.or(mint)
.with(warp::log::custom(|info| {
let actual_ip = info
let forwarded_for = info
.request_headers()
.get("x-forwarded-for")
.map(|inner| inner.to_str().unwrap_or("-"));

info!(
"{} \"{} {} {:?}\" {} \"{}\" \"{}\" \"{}\" {:?}",
OptFmt(info.remote_addr()),
info.method(),
info.path(),
info.version(),
info.status().as_u16(),
OptFmt(info.referer()),
OptFmt(info.user_agent()),
OptFmt(actual_ip),
info.elapsed(),
remote_addr = info.remote_addr(),
forwarded_for = forwarded_for,
host = info.host(),
method = format!("{}", info.method()),
path = info.path(),
version = format!("{:?}", info.version()),
status = format!("{}", info.status()),
referer = info.referer(),
user_agent = info.user_agent(),
elapsed = info.elapsed(),
"mint request"
Comment on lines -203 to +214
Copy link
Contributor

Choose a reason for hiding this comment

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

yuck, I really hate this type of logging, but I guess we only have this service anyway...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why though? can we at least agree that this is million times better than 10 random strings concatenated by spaces like it is right now? 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, I did approve, didn't I?

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good to get aligned with @banool on the logging since he will also have something for Tap.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have an issue in tap to do structured logging too, so this works for me.

)
}))
.with(
Expand Down Expand Up @@ -242,22 +244,6 @@ async fn handle_health(service: Arc<Service>) -> Result<Box<dyn warp::Reply>, In
}
}

//
// Common Types
//

struct OptFmt<T>(Option<T>);

impl<T: fmt::Display> fmt::Display for OptFmt<T> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
if let Some(t) = &self.0 {
fmt::Display::fmt(t, f)
} else {
f.write_str("-")
}
}
}

/// The idea is that this may be happening concurrently. If we end up in such a race, the faucets
/// might attempt to send transactions with the same sequence number, in such an event, one will
/// succeed and the other will hit an unwrap. Eventually all faucets should get online.
Expand Down