feat: add db metrics #445
feat: add db metrics #445
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
| fields( | ||
| category = "db_queries", | ||
| result = "success", | ||
| name = "add_auction", |
There was a problem hiding this comment.
maybe we should name this something else, to distinguish it from the instrument name. This name field influences the name of the metrics, but the instrument/span name is what determines the naming of the traces.
| @@ -86,7 +86,7 @@ async fn main() -> Result<()> { | |||
| .compact() | |||
| .with_filter(LevelFilter::INFO) | |||
| .with_filter(filter::filter_fn(|metadata| { | |||
| !is_metrics(metadata) && is_internal(metadata) | |||
| is_internal(metadata) || is_metrics(metadata, true) | |||
There was a problem hiding this comment.
later we can add more flags to distinguish btwn logging and telemetry
| @@ -678,37 +697,59 @@ impl<T: ChainTrait> Database<T> for DB { | |||
| bid.profile_id, | |||
| serde_json::to_value(bid.metadata.clone()).expect("Failed to serialize metadata"), | |||
| ).execute(self) | |||
| .instrument(info_span!("db_add_bid")) | |||
| .await.map_err(|e| { | |||
| .await { | |||
There was a problem hiding this comment.
why not map_err with ? instead of using the if let?
There was a problem hiding this comment.
yes that makes sense, changed back to this format
| @@ -648,8 +645,18 @@ pub trait Database<T: ChainTrait>: Debug + Send + Sync + 'static { | |||
|
|
|||
| #[async_trait] | |||
| impl<T: ChainTrait> Database<T> for DB { | |||
| #[instrument( | |||
There was a problem hiding this comment.
RN in the traces, the name is add_auction. I think it's a good idea to set the name as db_add_auction in the traces. In the metrics I think it's good to keep the name as it is RN, because all of these queries are in the same category (db_queries).
If you set the name in the outer scope (same level as the target), it'll be used for tracing. the name in the fields variable is being used only by metrics as the traces is not using this field name for the trace name :D
So you can update the code like below and this issue should be fixed:
#[instrument(
target = "metrics",
name = "db_add_auction",
fields(
category = "db_queries",
result = "success",
name = "add_auction",
tracing_enabled
),
skip_all
)]
| @@ -229,12 +247,12 @@ impl<T: InMemoryStore> Database<T> for DB { | |||
| } | |||
| query.push(" ORDER BY creation_time ASC LIMIT "); | |||
| query.push_bind(super::OPPORTUNITY_PAGE_SIZE_CAP as i64); | |||
| let opps: Vec<models::Opportunity<<T::Opportunity as entities::Opportunity>::ModelMetadata>> = query | |||
| let opps: Vec<models::Opportunity<<T::Opportunity as entities::Opportunity>::ModelMetadata>,> = query | |||
There was a problem hiding this comment.
good catch, addressing
This PR adds two sets of metrics for substantive db queries to the bid, opportunity, and auction tables: