Add db entry metrics#1412
Conversation
Pull Request Test Coverage Report for Build 23306027115Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
spacebear21
left a comment
There was a problem hiding this comment.
cACK, but I have a suggestion for a different approach.
Now that the directory lives in payjoin-mailroom, we should be able to wrap a Metrics wrapper around the DB following a middleware pattern. Something like this:
// db/mod.rs
/// Db decorator that records write metrics.
#[derive(Clone)]
pub struct MetricsDb<D: Db> {
inner: D,
metrics: MetricsService,
}
impl<D: Db> Db for MetricsDb<D> {
type OperationalError = D::OperationalError;
async fn post_v2_payload(
&self,
mailbox_id: &ShortId,
data: Vec<u8>,
) -> Result<Option<()>, Error<Self::OperationalError>> {
let result = self.inner.post_v2_payload(mailbox_id, data).await?;
self.metrics.record_db_entry("2");
Ok(result)
}
// ...
}
// lib.rs
struct Services {
directory: directory::Service<MetricsDb<DbServiceAdapter>>,
relay: ohttp_relay::Service,
metrics: MetricsService,
// ...
}
async fn init_directory(
config: &Config,
sentinel_tag: SentinelTag,
metrics: MetricsService,
) -> anyhow::Result<directory::Service<MetricsDb<DbServiceAdapter>>> {
let files_db = FilesDb::init(config.timeout, config.storage_dir.clone()).await?;
files_db.spawn_background_prune().await;
let db = MetricsDb::new(DbServiceAdapter::new(files_db), metrics);
// ...
}This keeps metrics out of the directory and follows a more modular approach in line with tower patterns.
f21c0df to
ad71757
Compare
| let geoip = init_geoip(&config).await?; | ||
|
|
||
| let directory = init_directory(&config, sentinel_tag).await?; | ||
| let directory = init_directory(&config, sentinel_tag, metrics.clone()).await?; |
There was a problem hiding this comment.
This clones the metrics service, that would result in two separate instances of MetricsService tracking different things independently right? We should probably pass some type of reference to ensure there is only one source of truth.
this pr addresses payjoin#941 , it adds db entries metrics seperated by version (v1/v2)
Instead of kitchen-sinking the v1/_core payjoin feature just for this
spacebear21
left a comment
There was a problem hiding this comment.
ACK ca80b75, applied my suggestion from #1412 (comment)
this pr addresses the db entries part of #941 , it adds db entries metrics seperated by version (v1/v2). For now it's just simple incremental counter seperated by v1/v2.
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.