Paths

Table of Contentst

-
services/identity/src/
-
identity/
-
src/
-
client_service.rs
-
constants.rs
-
database.rs
-
database/
-
device_list.rs
-
farcaster.rs
-
one_time_keys.rs
-
token.rs
-
grpc_services/
-
authenticated.rs
-
siwe.rs
-
sync_identity_search.rs
-
tunnelbroker.rs
-
websockets/
-
auth.rs
-
errors.rs
-
mod.rs
-
send.rs

Differential D11852

[identity] Add prefixes to error logs for filtering
ClosedPublic
Actions

Authored by will on May 1 2024, 8:03 PM.

Details

Reviewers

bartek

Commits

rCOMM6190b986ef59: [identity] Add prefixes to error logs for filtering

Summary

This adds prefixes to our error! tracing logs on the identity service

Depends on D11831

Test Plan

cargo check

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

will created this revision.May 1 2024, 8:03 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptMay 1 2024, 8:03 PM

will added a child revision: D11853: [terraform] metric filters for error logs in identity service.May 1 2024, 8:10 PM

will edited the summary of this revision. (Show Details)

will added a parent revision: D11831: [identity] add span tracing to identity farcaster.rs.

Harbormaster completed remote builds in B28618: Diff 39725.May 1 2024, 8:20 PM

will requested review of this revision.May 1 2024, 8:20 PM

I was imagining that all the prefixes (e.g. DB Error) would be defined in a module in constants.rs and then used like this:

pub const DB_ERROR_LOG_PREFIX: &str = "DB Error";

error!(DB_ERROR_LOG_PREFIX, "Encountered an unexpected error: {}", e);

This revision now requires changes to proceed.May 2 2024, 11:28 AM

varun mentioned this in D11854: [terraform] identity service cloudwatch alarms.May 2 2024, 11:34 AM

In D11852#339861, @varun wrote:
I was imagining that all the prefixes (e.g. DB Error) would be defined in a module in constants.rs and then used like this:
pub const DB_ERROR_LOG_PREFIX: &str = "DB Error";

error!(DB_ERROR_LOG_PREFIX, "Encountered an unexpected error: {}", e);

So that would look something like this on output:

{"message":"Encountered an unexpected error: e","DB_ERROR_LOG_PREFIX":"DB Error"}

Currently my alarms match on the message field. I think it overcomplicates the cloudwatch patterns if we're accessing each possible prefix variable name so what might be best is each error log specifies some field:

error!(errorType=DB_ERROR_LOG_PREFIX, "Encountered an unexpected error: {}", e);

would get us:

{"message":"Encountered an unexpected error: e", "errorType":"DB Error"}

I thought about how we'd be able to store the key value "errorType = DB_ERROR_LOG_PREFIX" in our const file so we wouldn't have to include errorType each time, but I don't think it's possible unless we do something really hacky with mods

Another option is a new error log helper function?

@varun's concern is valid to me. I like the errorType = ... approach.

In D11852#339879, @will wrote:
what might be best is each error log specifies some field:
error!(errorType=DB_ERROR_LOG_PREFIX, "Encountered an unexpected error: {}", e);
would get us:
{"message":"Encountered an unexpected error: e", "errorType":"DB Error"}
I thought about how we'd be able to store the key value "errorType = DB_ERROR_LOG_PREFIX" in our const file so we wouldn't have to include errorType each time, but I don't think it's possible unless we do something really hacky with mods

I think including errorType in each log is okay for now. If this gets too annoying, we can think about creating a custom error! macro

bartek mentioned this in D11853: [terraform] metric filters for error logs in identity service.May 5 2024, 10:49 PM

review feedback

Harbormaster completed remote builds in B28716: Diff 39870.May 6 2024, 12:45 PM

bartek accepted this revision.May 7 2024, 2:54 AM

bartek added inline comments.

services/identity/src/constants.rs
196–206 ↗	(On Diff #39870)	Optionally it could be done this way, so instead of doing `errorType = GENERIC_DB_LOG_ERROR_TYPE`, it would be `errorType = error_types::GENERIC_DB_LOG`. Both approaches are okay to me

will added inline comments.May 7 2024, 7:18 AM

services/identity/src/constants.rs
196–206 ↗	(On Diff #39870)	I like your way better. It's more organized. Will include in the next rebase

review feedback and whitespace

Harbormaster completed remote builds in B28736: Diff 39894.May 7 2024, 8:19 AM

@varun is off but his feedback has been addressed so unblocking this stack

This revision is now accepted and ready to land.May 7 2024, 9:47 AM

Closed by commit rCOMM6190b986ef59: [identity] Add prefixes to error logs for filtering (authored by will). · Explain WhyMay 8 2024, 10:33 AM

This revision was automatically updated to reflect the committed changes.

will marked an inline comment as done.

will added a commit: rCOMM6190b986ef59: [identity] Add prefixes to error logs for filtering.

Revision Contents
Changeset List

Path

Size

services/

identity/

src/

client_service.rs

10 lines

constants.rs

16 lines

database.rs

27 lines

database/

105 lines

6 lines

19 lines

8 lines

grpc_services/

authenticated.rs

33 lines

siwe.rs

22 lines

sync_identity_search.rs

6 lines

tunnelbroker.rs

6 lines

websockets/

17 lines

14 lines

33 lines

7 lines

Diff 39949

View Options

services/identity/src/client_service.rs

Show All 30 Lines
use super::DatabaseClient;		use super::DatabaseClient;

impl DatabaseClient {		impl DatabaseClient {
/// Gets the next one-time key for the account and then, in a transaction,		/// Gets the next one-time key for the account and then, in a transaction,
/// deletes the key and updates the key count		/// deletes the key and updates the key count
///		///
/// Returns the retrieved one-time key if it exists and a boolean indicating		/// Returns the retrieved one-time key if it exists and a boolean indicating
/// whether the `spawn_refresh_keys_task`` was called		/// whether the `spawn_refresh_keys_task`` was called
		#[tracing::instrument(skip_all)]
pub(super) async fn get_one_time_key(		pub(super) async fn get_one_time_key(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
account_type: OlmAccountType,		account_type: OlmAccountType,
can_request_more_keys: bool,		can_request_more_keys: bool,
) -> Result<(Option<String>, bool), Error> {		) -> Result<(Option<String>, bool), Error> {
use crate::constants::devices_table;		use crate::constants::devices_table;
▲ Show 20 Lines • Show All 102 Lines • ▼ Show 20 Lines	loop {
);		);
return Err(Error::AwsSdk(dynamo_db_error));		return Err(Error::AwsSdk(dynamo_db_error));
}		}
}		}
}		}
}		}
}		}

		#[tracing::instrument(skip_all)]
async fn get_one_time_keys(		async fn get_one_time_keys(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
account_type: OlmAccountType,		account_type: OlmAccountType,
num_keys: Option<usize>,		num_keys: Option<usize>,
) -> Result<Vec<OTKRow>, Error> {		) -> Result<Vec<OTKRow>, Error> {
use crate::constants::one_time_keys_table::*;		use crate::constants::one_time_keys_table::*;
Show All 33 Lines	if let Some(limit) = num_keys {
error!("There are fewer one-time keys than the number requested");		error!("There are fewer one-time keys than the number requested");
return Err(Error::NotEnoughOneTimeKeys);		return Err(Error::NotEnoughOneTimeKeys);
}		}
}		}

Ok(otk_rows)		Ok(otk_rows)
}		}

		#[tracing::instrument(skip_all)]
pub async fn append_one_time_prekeys(		pub async fn append_one_time_prekeys(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
content_one_time_keys: &Vec<String>,		content_one_time_keys: &Vec<String>,
notif_one_time_keys: &Vec<String>,		notif_one_time_keys: &Vec<String>,
) -> Result<(), Error> {		) -> Result<(), Error> {
use crate::constants::retry;		use crate::constants::retry;
▲ Show 20 Lines • Show All 115 Lines • ▼ Show 20 Lines	loop {
}		}
}		}
}		}
}		}

Ok(())		Ok(())
}		}

		#[tracing::instrument(skip_all)]
async fn get_otk_count(		async fn get_otk_count(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
account_type: OlmAccountType,		account_type: OlmAccountType,
) -> Result<usize, Error> {		) -> Result<usize, Error> {
use crate::constants::devices_table;		use crate::constants::devices_table;

▲ Show 20 Lines • Show All 142 Lines • Show Last 20 Lines

View Options

services/identity/src/constants.rs

Show All 30 Lines
use super::DatabaseClient;		use super::DatabaseClient;

impl DatabaseClient {		impl DatabaseClient {
/// Gets the next one-time key for the account and then, in a transaction,		/// Gets the next one-time key for the account and then, in a transaction,
/// deletes the key and updates the key count		/// deletes the key and updates the key count
///		///
/// Returns the retrieved one-time key if it exists and a boolean indicating		/// Returns the retrieved one-time key if it exists and a boolean indicating
/// whether the `spawn_refresh_keys_task`` was called		/// whether the `spawn_refresh_keys_task`` was called
		#[tracing::instrument(skip_all)]
pub(super) async fn get_one_time_key(		pub(super) async fn get_one_time_key(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
account_type: OlmAccountType,		account_type: OlmAccountType,
can_request_more_keys: bool,		can_request_more_keys: bool,
) -> Result<(Option<String>, bool), Error> {		) -> Result<(Option<String>, bool), Error> {
use crate::constants::devices_table;		use crate::constants::devices_table;
▲ Show 20 Lines • Show All 102 Lines • ▼ Show 20 Lines	loop {
);		);
return Err(Error::AwsSdk(dynamo_db_error));		return Err(Error::AwsSdk(dynamo_db_error));
}		}
}		}
}		}
}		}
}		}

		#[tracing::instrument(skip_all)]
async fn get_one_time_keys(		async fn get_one_time_keys(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
account_type: OlmAccountType,		account_type: OlmAccountType,
num_keys: Option<usize>,		num_keys: Option<usize>,
) -> Result<Vec<OTKRow>, Error> {		) -> Result<Vec<OTKRow>, Error> {
use crate::constants::one_time_keys_table::*;		use crate::constants::one_time_keys_table::*;
Show All 33 Lines	if let Some(limit) = num_keys {
error!("There are fewer one-time keys than the number requested");		error!("There are fewer one-time keys than the number requested");
return Err(Error::NotEnoughOneTimeKeys);		return Err(Error::NotEnoughOneTimeKeys);
}		}
}		}

Ok(otk_rows)		Ok(otk_rows)
}		}

		#[tracing::instrument(skip_all)]
pub async fn append_one_time_prekeys(		pub async fn append_one_time_prekeys(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
content_one_time_keys: &Vec<String>,		content_one_time_keys: &Vec<String>,
notif_one_time_keys: &Vec<String>,		notif_one_time_keys: &Vec<String>,
) -> Result<(), Error> {		) -> Result<(), Error> {
use crate::constants::retry;		use crate::constants::retry;
▲ Show 20 Lines • Show All 115 Lines • ▼ Show 20 Lines	loop {
}		}
}		}
}		}
}		}

Ok(())		Ok(())
}		}

		#[tracing::instrument(skip_all)]
async fn get_otk_count(		async fn get_otk_count(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
account_type: OlmAccountType,		account_type: OlmAccountType,
) -> Result<usize, Error> {		) -> Result<usize, Error> {
use crate::constants::devices_table;		use crate::constants::devices_table;

▲ Show 20 Lines • Show All 142 Lines • Show Last 20 Lines

View Options

services/identity/src/database.rs

Show All 30 Lines
use super::DatabaseClient;		use super::DatabaseClient;

impl DatabaseClient {		impl DatabaseClient {
/// Gets the next one-time key for the account and then, in a transaction,		/// Gets the next one-time key for the account and then, in a transaction,
/// deletes the key and updates the key count		/// deletes the key and updates the key count
///		///
/// Returns the retrieved one-time key if it exists and a boolean indicating		/// Returns the retrieved one-time key if it exists and a boolean indicating
/// whether the `spawn_refresh_keys_task`` was called		/// whether the `spawn_refresh_keys_task`` was called
		#[tracing::instrument(skip_all)]
pub(super) async fn get_one_time_key(		pub(super) async fn get_one_time_key(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
account_type: OlmAccountType,		account_type: OlmAccountType,
can_request_more_keys: bool,		can_request_more_keys: bool,
) -> Result<(Option<String>, bool), Error> {		) -> Result<(Option<String>, bool), Error> {
use crate::constants::devices_table;		use crate::constants::devices_table;
▲ Show 20 Lines • Show All 102 Lines • ▼ Show 20 Lines	loop {
);		);
return Err(Error::AwsSdk(dynamo_db_error));		return Err(Error::AwsSdk(dynamo_db_error));
}		}
}		}
}		}
}		}
}		}

		#[tracing::instrument(skip_all)]
async fn get_one_time_keys(		async fn get_one_time_keys(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
account_type: OlmAccountType,		account_type: OlmAccountType,
num_keys: Option<usize>,		num_keys: Option<usize>,
) -> Result<Vec<OTKRow>, Error> {		) -> Result<Vec<OTKRow>, Error> {
use crate::constants::one_time_keys_table::*;		use crate::constants::one_time_keys_table::*;
Show All 33 Lines	if let Some(limit) = num_keys {
error!("There are fewer one-time keys than the number requested");		error!("There are fewer one-time keys than the number requested");
return Err(Error::NotEnoughOneTimeKeys);		return Err(Error::NotEnoughOneTimeKeys);
}		}
}		}

Ok(otk_rows)		Ok(otk_rows)
}		}

		#[tracing::instrument(skip_all)]
pub async fn append_one_time_prekeys(		pub async fn append_one_time_prekeys(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
content_one_time_keys: &Vec<String>,		content_one_time_keys: &Vec<String>,
notif_one_time_keys: &Vec<String>,		notif_one_time_keys: &Vec<String>,
) -> Result<(), Error> {		) -> Result<(), Error> {
use crate::constants::retry;		use crate::constants::retry;
▲ Show 20 Lines • Show All 115 Lines • ▼ Show 20 Lines	loop {
}		}
}		}
}		}
}		}

Ok(())		Ok(())
}		}

		#[tracing::instrument(skip_all)]
async fn get_otk_count(		async fn get_otk_count(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
account_type: OlmAccountType,		account_type: OlmAccountType,
) -> Result<usize, Error> {		) -> Result<usize, Error> {
use crate::constants::devices_table;		use crate::constants::devices_table;

▲ Show 20 Lines • Show All 142 Lines • Show Last 20 Lines

View Options

services/identity/src/database/device_list.rs

Show All 30 Lines
use super::DatabaseClient;		use super::DatabaseClient;

impl DatabaseClient {		impl DatabaseClient {
/// Gets the next one-time key for the account and then, in a transaction,		/// Gets the next one-time key for the account and then, in a transaction,
/// deletes the key and updates the key count		/// deletes the key and updates the key count
///		///
/// Returns the retrieved one-time key if it exists and a boolean indicating		/// Returns the retrieved one-time key if it exists and a boolean indicating
/// whether the `spawn_refresh_keys_task`` was called		/// whether the `spawn_refresh_keys_task`` was called
		#[tracing::instrument(skip_all)]
pub(super) async fn get_one_time_key(		pub(super) async fn get_one_time_key(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
account_type: OlmAccountType,		account_type: OlmAccountType,
can_request_more_keys: bool,		can_request_more_keys: bool,
) -> Result<(Option<String>, bool), Error> {		) -> Result<(Option<String>, bool), Error> {
use crate::constants::devices_table;		use crate::constants::devices_table;
▲ Show 20 Lines • Show All 102 Lines • ▼ Show 20 Lines	loop {
);		);
return Err(Error::AwsSdk(dynamo_db_error));		return Err(Error::AwsSdk(dynamo_db_error));
}		}
}		}
}		}
}		}
}		}

		#[tracing::instrument(skip_all)]
async fn get_one_time_keys(		async fn get_one_time_keys(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
account_type: OlmAccountType,		account_type: OlmAccountType,
num_keys: Option<usize>,		num_keys: Option<usize>,
) -> Result<Vec<OTKRow>, Error> {		) -> Result<Vec<OTKRow>, Error> {
use crate::constants::one_time_keys_table::*;		use crate::constants::one_time_keys_table::*;
Show All 33 Lines	if let Some(limit) = num_keys {
error!("There are fewer one-time keys than the number requested");		error!("There are fewer one-time keys than the number requested");
return Err(Error::NotEnoughOneTimeKeys);		return Err(Error::NotEnoughOneTimeKeys);
}		}
}		}

Ok(otk_rows)		Ok(otk_rows)
}		}

		#[tracing::instrument(skip_all)]
pub async fn append_one_time_prekeys(		pub async fn append_one_time_prekeys(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
content_one_time_keys: &Vec<String>,		content_one_time_keys: &Vec<String>,
notif_one_time_keys: &Vec<String>,		notif_one_time_keys: &Vec<String>,
) -> Result<(), Error> {		) -> Result<(), Error> {
use crate::constants::retry;		use crate::constants::retry;
▲ Show 20 Lines • Show All 115 Lines • ▼ Show 20 Lines	loop {
}		}
}		}
}		}
}		}

Ok(())		Ok(())
}		}

		#[tracing::instrument(skip_all)]
async fn get_otk_count(		async fn get_otk_count(
&self,		&self,
user_id: &str,		user_id: &str,
device_id: &str,		device_id: &str,
account_type: OlmAccountType,		account_type: OlmAccountType,
) -> Result<usize, Error> {		) -> Result<usize, Error> {
use crate::constants::devices_table;		use crate::constants::devices_table;

▲ Show 20 Lines • Show All 142 Lines • Show Last 20 Lines