Skip to content

Add more tracing instrumentation #6380

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.

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

Merged
merged 10 commits into from
Apr 25, 2023
7 changes: 7 additions & 0 deletions cargo-registry-index/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ impl Repository {
/// - If cloning the crate index fails.
/// - If reading the global git config fails.
///
#[instrument(skip_all)]
pub fn open(repository_config: &RepositoryConfig) -> anyhow::Result<Self> {
let checkout_path = tempfile::Builder::new()
.prefix("git")
Expand Down Expand Up @@ -401,6 +402,7 @@ impl Repository {
///
/// Note that `modified_file` expects a file path **relative** to the
/// repository working folder!
#[instrument(skip_all, fields(message = %msg))]
fn perform_commit_and_push(&self, msg: &str, modified_file: &Path) -> anyhow::Result<()> {
// git add $file
let mut index = self.repository.index()?;
Expand All @@ -421,6 +423,7 @@ impl Repository {

/// Gets a list of files that have been modified since a given `starting_commit`
/// (use `starting_commit = None` for a list of all files).
#[instrument(skip_all)]
pub fn get_files_modified_since(
&self,
starting_commit: Option<&str>,
Expand Down Expand Up @@ -464,6 +467,7 @@ impl Repository {
}

/// Push the current branch to the provided refname
#[instrument(skip_all)]
fn push(&self, refspec: &str) -> anyhow::Result<()> {
let mut ref_status = Ok(());
let mut callback_called = false;
Expand Down Expand Up @@ -513,6 +517,7 @@ impl Repository {

/// Fetches any changes from the `origin` remote and performs a hard reset
/// to the tip of the `origin/master` branch.
#[instrument(skip_all)]
pub fn reset_head(&self) -> anyhow::Result<()> {
let mut origin = self.repository.find_remote("origin")?;
let original_head = self.head_oid()?;
Expand Down Expand Up @@ -549,6 +554,7 @@ impl Repository {
}

/// Reset `HEAD` to a single commit with all the index contents, but no parent
#[instrument(skip_all)]
pub fn squash_to_single_commit(&self, msg: &str) -> anyhow::Result<()> {
let tree = self.repository.find_commit(self.head_oid()?)?.tree()?;
let sig = self.repository.signature()?;
Expand Down Expand Up @@ -583,6 +589,7 @@ impl Repository {
///
/// This function also temporarily sets the `GIT_SSH_COMMAND` environment
/// variable to ensure that `git push` commands are able to succeed.
#[instrument(skip_all)]
pub fn run_via_cli(command: &mut Command, credentials: &Credentials) -> anyhow::Result<()> {
let temp_key_path = credentials
.ssh_key()
Expand Down
3 changes: 3 additions & 0 deletions src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,13 +213,15 @@ impl App {
}

/// Obtain a read/write database connection from the primary pool
#[instrument(skip_all)]
pub fn db_write(&self) -> Result<DieselPooledConn<'_>, PoolError> {
self.primary_database.get()
}

/// Obtain a readonly database connection from the replica pool
///
/// If the replica pool is disabled or unavailable, the primary pool is used instead.
#[instrument(skip_all)]
pub fn db_read(&self) -> Result<DieselPooledConn<'_>, PoolError> {
let read_only_pool = self.read_only_replica_database.as_ref();
match read_only_pool.map(|pool| pool.get()) {
Expand Down Expand Up @@ -248,6 +250,7 @@ impl App {
/// Obtain a readonly database connection from the primary pool
///
/// If the primary pool is unavailable, the replica pool is used instead, if not disabled.
#[instrument(skip_all)]
pub fn db_read_prefer_primary(&self) -> Result<DieselPooledConn<'_>, PoolError> {
match (
self.primary_database.get(),
Expand Down
4 changes: 4 additions & 0 deletions src/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ impl AuthCheck {
}
}

#[instrument(name = "auth.check", skip_all)]
pub fn check<T: RequestPartsExt>(
&self,
request: &T,
Expand Down Expand Up @@ -156,6 +157,7 @@ impl Authentication {
}
}

#[instrument(skip_all)]
fn authenticate_via_cookie<T: RequestPartsExt>(
req: &T,
conn: &mut PgConnection,
Expand All @@ -177,6 +179,7 @@ fn authenticate_via_cookie<T: RequestPartsExt>(
Ok(Some(CookieAuthentication { user }))
}

#[instrument(skip_all)]
fn authenticate_via_token<T: RequestPartsExt>(
req: &T,
conn: &mut PgConnection,
Expand Down Expand Up @@ -207,6 +210,7 @@ fn authenticate_via_token<T: RequestPartsExt>(
Ok(Some(TokenAuthentication { user, token }))
}

#[instrument(skip_all)]
fn authenticate<T: RequestPartsExt>(req: &T, conn: &mut PgConnection) -> AppResult<Authentication> {
controllers::util::verify_origin(req)?;

Expand Down
1 change: 1 addition & 0 deletions src/background_jobs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ impl Environment {
}
}

#[instrument(skip_all)]
pub fn lock_index(&self) -> Result<MutexGuard<'_, Repository>, PerformError> {
let repo = self.index.lock().unwrap_or_else(PoisonError::into_inner);
repo.reset_head()?;
Expand Down
2 changes: 2 additions & 0 deletions src/controllers/krate/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ pub fn missing_metadata_error_message(missing: &[&str]) -> String {
)
}

#[instrument(skip_all)]
pub fn add_dependencies(
conn: &mut PgConnection,
deps: &[EncodableCrateDependency],
Expand Down Expand Up @@ -425,6 +426,7 @@ pub fn add_dependencies(
Ok(git_deps)
}

#[instrument(skip_all, fields(%pkg_name))]
fn verify_tarball(
pkg_name: &str,
tarball: &[u8],
Expand Down
14 changes: 10 additions & 4 deletions src/controllers/krate/search.rs
Original file line number Diff line number Diff line change
Expand Up @@ -280,9 +280,12 @@ pub async fn search(app: AppState, req: Parts) -> AppResult<Json<Value>> {
//
// If this becomes a problem in the future the crates count could be denormalized, at least
// for the filterless happy path.
let total: i64 = crates::table.count().get_result(conn)?;
let total: i64 = info_span!("db.query", message = "SELECT COUNT(*) FROM crates")
.in_scope(|| crates::table.count().get_result(conn))?;

let results: Vec<(Crate, bool, Option<i64>)> = query.load(conn)?;
let results: Vec<(Crate, bool, Option<i64>)> =
info_span!("db.query", message = "SELECT ... FROM crates")
.in_scope(|| query.load(conn))?;

let next_page = if let Some(last) = results.last() {
let mut params = IndexMap::new();
Expand All @@ -298,7 +301,9 @@ pub async fn search(app: AppState, req: Parts) -> AppResult<Json<Value>> {
(total, next_page, None, results, conn)
} else {
let query = query.pages_pagination(pagination);
let data: Paginated<(Crate, bool, Option<i64>)> = query.load(conn)?;
let data: Paginated<(Crate, bool, Option<i64>)> =
info_span!("db.query", message = "SELECT ..., COUNT(*) FROM crates")
.in_scope(|| query.load(conn))?;
(
data.total(),
data.next_page_params().map(|p| req.query_with_params(p)),
Expand All @@ -315,7 +320,8 @@ pub async fn search(app: AppState, req: Parts) -> AppResult<Json<Value>> {
.collect::<Vec<_>>();
let crates = data.into_iter().map(|(c, _, _)| c).collect::<Vec<_>>();

let versions: Vec<Version> = crates.versions().load(conn)?;
let versions: Vec<Version> = info_span!("db.query", message = "SELECT ... FROM versions")
.in_scope(|| crates.versions().load(conn))?;
let versions = versions
.grouped_by(&crates)
.into_iter()
Expand Down
2 changes: 2 additions & 0 deletions src/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ impl DieselPool {
DieselPool::Test(Arc::new(Mutex::new(conn)))
}

#[instrument(name = "db.connect", skip_all)]
pub fn get(&self) -> Result<DieselPooledConn<'_>, PoolError> {
match self {
DieselPool::Pool {
Expand Down Expand Up @@ -112,6 +113,7 @@ impl DieselPool {
}
}

#[instrument(skip_all)]
pub fn wait_until_healthy(&self, timeout: Duration) -> Result<(), PoolError> {
match self {
DieselPool::Pool { pool, .. } | DieselPool::BackgroundJobPool { pool } => {
Expand Down
1 change: 1 addition & 0 deletions src/models/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,7 @@ impl Crate {
}

/// Returns (dependency, dependent crate name, dependent crate downloads)
#[instrument(skip_all, fields(krate.name = %self.name))]
pub(crate) fn reverse_dependencies(
&self,
conn: &mut PgConnection,
Expand Down
6 changes: 6 additions & 0 deletions src/uploaders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ impl Uploader {
///
/// This function can panic on an `Self::Local` during development.
/// Production and tests use `Self::S3` which should not panic.
#[instrument(skip_all, fields(%path))]
pub fn upload<R: Into<Body>>(
&self,
client: &Client,
Expand Down Expand Up @@ -149,6 +150,7 @@ impl Uploader {
}

/// Deletes a file using the configured uploader (either `S3`, `Local`).
#[instrument(skip_all, fields(%path))]
pub fn delete(&self, client: &Client, path: &str, upload_bucket: UploadBucket) -> Result<()> {
match *self {
Uploader::S3 {
Expand Down Expand Up @@ -176,6 +178,7 @@ impl Uploader {
}

/// Uploads a crate and returns the checksum of the uploaded crate file.
#[instrument(skip_all)]
pub fn upload_crate<R: Into<Body>>(
&self,
http_client: &Client,
Expand All @@ -201,6 +204,7 @@ impl Uploader {
Ok(())
}

#[instrument(skip_all)]
pub(crate) fn upload_readme(
&self,
http_client: &Client,
Expand All @@ -225,6 +229,7 @@ impl Uploader {
Ok(())
}

#[instrument(skip_all)]
pub(crate) fn upload_index(
&self,
http_client: &Client,
Expand All @@ -248,6 +253,7 @@ impl Uploader {
Ok(())
}

#[instrument(skip_all)]
pub(crate) fn delete_index(&self, http_client: &Client, crate_name: &str) -> Result<()> {
let path = Uploader::index_path(crate_name);
self.delete(http_client, &path, UploadBucket::Index)?;
Expand Down
6 changes: 6 additions & 0 deletions src/worker/readmes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use diesel::PgConnection;
use crate::background_jobs::Environment;
use crate::models::Version;

#[instrument(skip_all, fields(krate.name))]
pub fn perform_render_and_upload_readme(
conn: &mut PgConnection,
env: &Environment,
Expand All @@ -19,6 +20,8 @@ pub fn perform_render_and_upload_readme(
use crate::schema::*;
use diesel::prelude::*;

info!(?version_id, "Rendering README");

let rendered = text_to_html(text, readme_path, base_url, pkg_path_in_vcs);

conn.transaction(|conn| {
Expand All @@ -28,6 +31,9 @@ pub fn perform_render_and_upload_readme(
.inner_join(crates::table)
.select((crates::name, versions::num))
.first(conn)?;

tracing::Span::current().record("krate.name", tracing::field::display(&crate_name));

env.uploader
.upload_readme(env.http_client(), &crate_name, &vers, rendered)?;
Ok(())
Expand Down