feat(tvix): add instance_name to instrumentation of *Services

Currently it is not possible to distinguish between tracing of the same
*Service type whenever there are multiple of them. Now the instance_name
of ServiceBuilder is passed into the *Service and used in the existing
instrument as the `instance_name` field.

Places that did not already have a instance_name in its context use
`"default"`. In tests I used `"test"`.

Change-Id: Ia20bf2a7bb849a781e370d087ba7ddb3be79f654
Reviewed-on: https://cl.tvl.fyi/c/depot/+/12739
Tested-by: BuildkiteCI
Autosubmit: Bob van der Linden <bobvanderlinden@gmail.com>
Reviewed-by: flokli <flokli@flokli.de>
This commit is contained in:
Bob van der Linden 2024-11-06 23:13:33 +01:00 committed by clbot
parent 951d25676b
commit cfa4154131
23 changed files with 270 additions and 137 deletions

View file

@ -35,6 +35,7 @@ const CELL_SIZE_LIMIT: u64 = 10 * 1024 * 1024;
/// "unimplemented" error.
#[derive(Clone)]
pub struct BigtablePathInfoService {
instance_name: String,
client: bigtable::BigTable,
params: BigtableParameters,
@ -47,7 +48,10 @@ pub struct BigtablePathInfoService {
impl BigtablePathInfoService {
#[cfg(not(test))]
pub async fn connect(params: BigtableParameters) -> Result<Self, bigtable::Error> {
pub async fn connect(
instance_name: String,
params: BigtableParameters,
) -> Result<Self, bigtable::Error> {
let connection = bigtable::BigTableConnection::new(
&params.project_id,
&params.instance_name,
@ -58,13 +62,17 @@ impl BigtablePathInfoService {
.await?;
Ok(Self {
instance_name,
client: connection.client(),
params,
})
}
#[cfg(test)]
pub async fn connect(params: BigtableParameters) -> Result<Self, bigtable::Error> {
pub async fn connect(
instance_name: String,
params: BigtableParameters,
) -> Result<Self, bigtable::Error> {
use std::time::Duration;
use async_process::{Command, Stdio};
@ -133,6 +141,7 @@ impl BigtablePathInfoService {
)?;
Ok(Self {
instance_name: instance_name.to_string(),
client: connection.client(),
params,
emulator: (tmpdir, emulator_process).into(),
@ -148,7 +157,7 @@ fn derive_pathinfo_key(digest: &[u8; 20]) -> String {
#[async_trait]
impl PathInfoService for BigtablePathInfoService {
#[instrument(level = "trace", skip_all, fields(path_info.digest = nixbase32::encode(&digest)))]
#[instrument(level = "trace", skip_all, fields(path_info.digest = nixbase32::encode(&digest), instance_name = %self.instance_name))]
async fn get(&self, digest: [u8; 20]) -> Result<Option<PathInfo>, Error> {
let mut client = self.client.clone();
let path_info_key = derive_pathinfo_key(&digest);
@ -244,7 +253,7 @@ impl PathInfoService for BigtablePathInfoService {
Ok(Some(path_info))
}
#[instrument(level = "trace", skip_all, fields(path_info.root_node = ?path_info.node))]
#[instrument(level = "trace", skip_all, fields(path_info.root_node = ?path_info.node, instance_name = %self.instance_name))]
async fn put(&self, path_info: PathInfo) -> Result<PathInfo, Error> {
let mut client = self.client.clone();
let path_info_key = derive_pathinfo_key(path_info.store_path.digest());
@ -410,11 +419,11 @@ impl ServiceBuilder for BigtableParameters {
type Output = dyn PathInfoService;
async fn build<'a>(
&'a self,
_instance_name: &str,
instance_name: &str,
_context: &CompositionContext,
) -> Result<Arc<dyn PathInfoService>, Box<dyn std::error::Error + Send + Sync>> {
Ok(Arc::new(
BigtablePathInfoService::connect(self.clone()).await?,
BigtablePathInfoService::connect(instance_name.to_string(), self.clone()).await?,
))
}
}

View file

@ -15,13 +15,18 @@ use super::{PathInfo, PathInfoService};
/// There is no negative cache.
/// Inserts and listings are not implemented for now.
pub struct Cache<PS1, PS2> {
instance_name: String,
near: PS1,
far: PS2,
}
impl<PS1, PS2> Cache<PS1, PS2> {
pub fn new(near: PS1, far: PS2) -> Self {
Self { near, far }
pub fn new(instance_name: String, near: PS1, far: PS2) -> Self {
Self {
instance_name,
near,
far,
}
}
}
@ -31,7 +36,7 @@ where
PS1: PathInfoService,
PS2: PathInfoService,
{
#[instrument(level = "trace", skip_all, fields(path_info.digest = nixbase32::encode(&digest)))]
#[instrument(level = "trace", skip_all, fields(path_info.digest = nixbase32::encode(&digest), instance_name = %self.instance_name))]
async fn get(&self, digest: [u8; 20]) -> Result<Option<PathInfo>, Error> {
match self.near.get(digest).await? {
Some(path_info) => {
@ -84,7 +89,7 @@ impl ServiceBuilder for CacheConfig {
type Output = dyn PathInfoService;
async fn build<'a>(
&'a self,
_instance_name: &str,
instance_name: &str,
context: &CompositionContext,
) -> Result<Arc<dyn PathInfoService>, Box<dyn std::error::Error + Send + Sync + 'static>> {
let (near, far) = futures::join!(
@ -92,6 +97,7 @@ impl ServiceBuilder for CacheConfig {
context.resolve::<Self::Output>(self.far.clone())
);
Ok(Arc::new(Cache {
instance_name: instance_name.to_string(),
near: near?,
far: far?,
}))
@ -114,10 +120,10 @@ mod test {
let far = MemoryPathInfoService::default();
// … and an instance of a "near" PathInfoService.
let near = LruPathInfoService::with_capacity(NonZeroUsize::new(1).unwrap());
let near = LruPathInfoService::with_capacity("test".into(), NonZeroUsize::new(1).unwrap());
// create a Pathinfoservice combining the two and return it.
super::Cache::new(near, far)
super::Cache::new("test".into(), near, far)
}
/// Getting from the far backend is gonna insert it into the near one.

View file

@ -17,6 +17,8 @@ use tvix_castore::Node;
/// Connects to a (remote) tvix-store PathInfoService over gRPC.
#[derive(Clone)]
pub struct GRPCPathInfoService<T> {
instance_name: String,
/// The internal reference to a gRPC client.
/// Cloning it is cheap, and it internally handles concurrent requests.
grpc_client: proto::path_info_service_client::PathInfoServiceClient<T>,
@ -26,9 +28,13 @@ impl<T> GRPCPathInfoService<T> {
/// construct a [GRPCPathInfoService] from a [proto::path_info_service_client::PathInfoServiceClient].
/// panics if called outside the context of a tokio runtime.
pub fn from_client(
instance_name: String,
grpc_client: proto::path_info_service_client::PathInfoServiceClient<T>,
) -> Self {
Self { grpc_client }
Self {
instance_name,
grpc_client,
}
}
}
@ -40,7 +46,7 @@ where
<T::ResponseBody as tonic::codegen::Body>::Error: Into<tonic::codegen::StdError> + Send,
T::Future: Send,
{
#[instrument(level = "trace", skip_all, fields(path_info.digest = nixbase32::encode(&digest)))]
#[instrument(level = "trace", skip_all, fields(path_info.digest = nixbase32::encode(&digest), instance_name = %self.instance_name))]
async fn get(&self, digest: [u8; 20]) -> Result<Option<PathInfo>, Error> {
let path_info = self
.grpc_client
@ -62,7 +68,7 @@ where
}
}
#[instrument(level = "trace", skip_all, fields(path_info.root_node = ?path_info.node))]
#[instrument(level = "trace", skip_all, fields(path_info.root_node = ?path_info.node, instance_name = %self.instance_name))]
async fn put(&self, path_info: PathInfo) -> Result<PathInfo, Error> {
let path_info = self
.grpc_client
@ -99,6 +105,7 @@ where
#[instrument(level = "trace", skip_all)]
fn nar_calculation_service(&self) -> Option<Box<dyn NarCalculationService>> {
Some(Box::new(GRPCPathInfoService {
instance_name: self.instance_name.clone(),
grpc_client: self.grpc_client.clone(),
}) as Box<dyn NarCalculationService>)
}
@ -163,13 +170,16 @@ impl ServiceBuilder for GRPCPathInfoServiceConfig {
type Output = dyn PathInfoService;
async fn build<'a>(
&'a self,
_instance_name: &str,
instance_name: &str,
_context: &CompositionContext,
) -> Result<Arc<dyn PathInfoService>, Box<dyn std::error::Error + Send + Sync + 'static>> {
let client = proto::path_info_service_client::PathInfoServiceClient::new(
tvix_castore::tonic::channel_from_url(&self.url.parse()?).await?,
);
Ok(Arc::new(GRPCPathInfoService::from_client(client)))
Ok(Arc::new(GRPCPathInfoService::from_client(
instance_name.to_string(),
client,
)))
}
}

View file

@ -14,12 +14,14 @@ use tvix_castore::Error;
use super::{PathInfo, PathInfoService};
pub struct LruPathInfoService {
instance_name: String,
lru: Arc<RwLock<LruCache<[u8; 20], PathInfo>>>,
}
impl LruPathInfoService {
pub fn with_capacity(capacity: NonZeroUsize) -> Self {
pub fn with_capacity(instance_name: String, capacity: NonZeroUsize) -> Self {
Self {
instance_name,
lru: Arc::new(RwLock::new(LruCache::new(capacity))),
}
}
@ -27,12 +29,12 @@ impl LruPathInfoService {
#[async_trait]
impl PathInfoService for LruPathInfoService {
#[instrument(level = "trace", skip_all, fields(path_info.digest = nixbase32::encode(&digest)))]
#[instrument(level = "trace", skip_all, fields(path_info.digest = nixbase32::encode(&digest), instance_name = %self.instance_name))]
async fn get(&self, digest: [u8; 20]) -> Result<Option<PathInfo>, Error> {
Ok(self.lru.write().await.get(&digest).cloned())
}
#[instrument(level = "trace", skip_all, fields(path_info.root_node = ?path_info.node))]
#[instrument(level = "trace", skip_all, fields(path_info.root_node = ?path_info.node, instance_name = %self.instance_name))]
async fn put(&self, path_info: PathInfo) -> Result<PathInfo, Error> {
self.lru
.write()
@ -76,10 +78,13 @@ impl ServiceBuilder for LruPathInfoServiceConfig {
type Output = dyn PathInfoService;
async fn build<'a>(
&'a self,
_instance_name: &str,
instance_name: &str,
_context: &CompositionContext,
) -> Result<Arc<dyn PathInfoService>, Box<dyn std::error::Error + Send + Sync + 'static>> {
Ok(Arc::new(LruPathInfoService::with_capacity(self.capacity)))
Ok(Arc::new(LruPathInfoService::with_capacity(
instance_name.to_string(),
self.capacity,
)))
}
}
@ -103,7 +108,7 @@ mod test {
#[tokio::test]
async fn evict() {
let svc = LruPathInfoService::with_capacity(NonZeroUsize::new(1).unwrap());
let svc = LruPathInfoService::with_capacity("test".into(), NonZeroUsize::new(1).unwrap());
// pathinfo_1 should not be there
assert!(svc

View file

@ -11,12 +11,13 @@ use tvix_castore::Error;
#[derive(Default)]
pub struct MemoryPathInfoService {
instance_name: String,
db: Arc<RwLock<HashMap<[u8; 20], PathInfo>>>,
}
#[async_trait]
impl PathInfoService for MemoryPathInfoService {
#[instrument(level = "trace", skip_all, fields(path_info.digest = nixbase32::encode(&digest)))]
#[instrument(level = "trace", skip_all, fields(path_info.digest = nixbase32::encode(&digest), instance_name = %self.instance_name))]
async fn get(&self, digest: [u8; 20]) -> Result<Option<PathInfo>, Error> {
let db = self.db.read().await;
@ -26,7 +27,7 @@ impl PathInfoService for MemoryPathInfoService {
}
}
#[instrument(level = "trace", skip_all, fields(path_info.root_node = ?path_info.node))]
#[instrument(level = "trace", skip_all, fields(path_info.root_node = ?path_info.node, instance_name = %self.instance_name))]
async fn put(&self, path_info: PathInfo) -> Result<PathInfo, Error> {
// This overwrites existing PathInfo objects with the same store path digest.
let mut db = self.db.write().await;
@ -69,9 +70,12 @@ impl ServiceBuilder for MemoryPathInfoServiceConfig {
type Output = dyn PathInfoService;
async fn build<'a>(
&'a self,
_instance_name: &str,
instance_name: &str,
_context: &CompositionContext,
) -> Result<Arc<dyn PathInfoService>, Box<dyn std::error::Error + Send + Sync + 'static>> {
Ok(Arc::new(MemoryPathInfoService::default()))
Ok(Arc::new(MemoryPathInfoService {
instance_name: instance_name.to_string(),
db: Default::default(),
}))
}
}

View file

@ -32,6 +32,7 @@ use url::Url;
/// [PathInfoService::put] is not implemented and returns an error if called.
/// TODO: what about reading from nix-cache-info?
pub struct NixHTTPPathInfoService<BS, DS> {
instance_name: String,
base_url: url::Url,
http_client: reqwest_middleware::ClientWithMiddleware,
@ -44,8 +45,14 @@ pub struct NixHTTPPathInfoService<BS, DS> {
}
impl<BS, DS> NixHTTPPathInfoService<BS, DS> {
pub fn new(base_url: url::Url, blob_service: BS, directory_service: DS) -> Self {
pub fn new(
instance_name: String,
base_url: url::Url,
blob_service: BS,
directory_service: DS,
) -> Self {
Self {
instance_name,
base_url,
http_client: reqwest_middleware::ClientBuilder::new(reqwest::Client::new())
.with(tvix_tracing::propagate::reqwest::tracing_middleware())
@ -69,7 +76,7 @@ where
BS: BlobService + Send + Sync + Clone + 'static,
DS: DirectoryService + Send + Sync + Clone + 'static,
{
#[instrument(skip_all, err, fields(path.digest=nixbase32::encode(&digest)))]
#[instrument(skip_all, err, fields(path.digest=nixbase32::encode(&digest), instance_name=%self.instance_name))]
async fn get(&self, digest: [u8; 20]) -> Result<Option<PathInfo>, Error> {
let narinfo_url = self
.base_url
@ -241,7 +248,7 @@ where
}))
}
#[instrument(skip_all, fields(path_info=?_path_info))]
#[instrument(skip_all, fields(path_info=?_path_info, instance_name=%self.instance_name))]
async fn put(&self, _path_info: PathInfo) -> Result<PathInfo, Error> {
Err(Error::InvalidRequest(
"put not supported for this backend".to_string(),
@ -314,7 +321,7 @@ impl ServiceBuilder for NixHTTPPathInfoServiceConfig {
type Output = dyn PathInfoService;
async fn build<'a>(
&'a self,
_instance_name: &str,
instance_name: &str,
context: &CompositionContext,
) -> Result<Arc<Self::Output>, Box<dyn std::error::Error + Send + Sync + 'static>> {
let (blob_service, directory_service) = futures::join!(
@ -322,6 +329,7 @@ impl ServiceBuilder for NixHTTPPathInfoServiceConfig {
context.resolve::<dyn DirectoryService>(self.directory_service.clone())
);
let mut svc = NixHTTPPathInfoService::new(
instance_name.to_string(),
Url::parse(&self.base_url)?,
blob_service?,
directory_service?,

View file

@ -19,6 +19,7 @@ const PATHINFO_TABLE: TableDefinition<[u8; 20], Vec<u8>> = TableDefinition::new(
/// redb stores all of its data in a single file with a K/V pointing from a path's output hash to
/// its corresponding protobuf-encoded PathInfo.
pub struct RedbPathInfoService {
instance_name: String,
// We wrap db in an Arc to be able to move it into spawn_blocking,
// as discussed in https://github.com/cberner/redb/issues/789
db: Arc<Database>,
@ -27,7 +28,7 @@ pub struct RedbPathInfoService {
impl RedbPathInfoService {
/// Constructs a new instance using the specified file system path for
/// storage.
pub async fn new(path: PathBuf) -> Result<Self, Error> {
pub async fn new(instance_name: String, path: PathBuf) -> Result<Self, Error> {
if path == PathBuf::from("/") {
return Err(Error::StorageError(
"cowardly refusing to open / with redb".to_string(),
@ -41,17 +42,23 @@ impl RedbPathInfoService {
})
.await??;
Ok(Self { db: Arc::new(db) })
Ok(Self {
instance_name,
db: Arc::new(db),
})
}
/// Constructs a new instance using the in-memory backend.
pub fn new_temporary() -> Result<Self, Error> {
pub fn new_temporary(instance_name: String) -> Result<Self, Error> {
let db =
redb::Database::builder().create_with_backend(redb::backends::InMemoryBackend::new())?;
create_schema(&db)?;
Ok(Self { db: Arc::new(db) })
Ok(Self {
instance_name,
db: Arc::new(db),
})
}
}
@ -68,7 +75,7 @@ fn create_schema(db: &redb::Database) -> Result<(), redb::Error> {
#[async_trait]
impl PathInfoService for RedbPathInfoService {
#[instrument(level = "trace", skip_all, fields(path_info.digest = BASE64.encode(&digest)))]
#[instrument(level = "trace", skip_all, fields(path_info.digest = BASE64.encode(&digest), instance_name = %self.instance_name))]
async fn get(&self, digest: [u8; 20]) -> Result<Option<PathInfo>, Error> {
let db = self.db.clone();
@ -93,7 +100,7 @@ impl PathInfoService for RedbPathInfoService {
.await?
}
#[instrument(level = "trace", skip_all, fields(path_info.root_node = ?path_info.node))]
#[instrument(level = "trace", skip_all, fields(path_info.root_node = ?path_info.node, instance_name = %self.instance_name))]
async fn put(&self, path_info: PathInfo) -> Result<PathInfo, Error> {
let db = self.db.clone();
@ -193,14 +200,16 @@ impl ServiceBuilder for RedbPathInfoServiceConfig {
type Output = dyn PathInfoService;
async fn build<'a>(
&'a self,
_instance_name: &str,
instance_name: &str,
_context: &CompositionContext,
) -> Result<Arc<dyn PathInfoService>, Box<dyn std::error::Error + Send + Sync + 'static>> {
match self {
RedbPathInfoServiceConfig {
is_temporary: true,
path: None,
} => Ok(Arc::new(RedbPathInfoService::new_temporary()?)),
} => Ok(Arc::new(RedbPathInfoService::new_temporary(
instance_name.to_string(),
)?)),
RedbPathInfoServiceConfig {
is_temporary: true,
path: Some(_),
@ -215,7 +224,9 @@ impl ServiceBuilder for RedbPathInfoServiceConfig {
RedbPathInfoServiceConfig {
is_temporary: false,
path: Some(path),
} => Ok(Arc::new(RedbPathInfoService::new(path.to_owned()).await?)),
} => Ok(Arc::new(
RedbPathInfoService::new(instance_name.to_string(), path.to_owned()).await?,
)),
}
}
}

View file

@ -27,6 +27,7 @@ use super::MemoryPathInfoService;
///
/// The service signs the [PathInfo] **only if it has a narinfo attribute**
pub struct SigningPathInfoService<T, S> {
instance_name: String,
/// The inner [PathInfoService]
inner: T,
/// The key to sign narinfos
@ -34,8 +35,12 @@ pub struct SigningPathInfoService<T, S> {
}
impl<T, S> SigningPathInfoService<T, S> {
pub fn new(inner: T, signing_key: Arc<SigningKey<S>>) -> Self {
Self { inner, signing_key }
pub fn new(instance_name: String, inner: T, signing_key: Arc<SigningKey<S>>) -> Self {
Self {
instance_name,
inner,
signing_key,
}
}
}
@ -45,7 +50,7 @@ where
T: PathInfoService,
S: ed25519::signature::Signer<ed25519::Signature> + Sync + Send,
{
#[instrument(level = "trace", skip_all, fields(path_info.digest = nixbase32::encode(&digest)))]
#[instrument(level = "trace", skip_all, fields(path_info.digest = nixbase32::encode(&digest), instance_name = %self.instance_name))]
async fn get(&self, digest: [u8; 20]) -> Result<Option<PathInfo>, Error> {
self.inner.get(digest).await
}
@ -101,7 +106,7 @@ impl ServiceBuilder for KeyFileSigningPathInfoServiceConfig {
type Output = dyn PathInfoService;
async fn build<'a>(
&'a self,
_instance_name: &str,
instance_name: &str,
context: &CompositionContext,
) -> Result<Arc<dyn PathInfoService>, Box<dyn std::error::Error + Send + Sync + 'static>> {
let inner = context.resolve::<Self::Output>(self.inner.clone()).await?;
@ -110,7 +115,11 @@ impl ServiceBuilder for KeyFileSigningPathInfoServiceConfig {
.map_err(|e| Error::StorageError(e.to_string()))?
.0,
);
Ok(Arc::new(SigningPathInfoService { inner, signing_key }))
Ok(Arc::new(SigningPathInfoService {
instance_name: instance_name.to_string(),
inner,
signing_key,
}))
}
}
@ -118,6 +127,7 @@ impl ServiceBuilder for KeyFileSigningPathInfoServiceConfig {
pub(crate) fn test_signing_service() -> Arc<dyn PathInfoService> {
let memory_svc: Arc<dyn PathInfoService> = Arc::new(MemoryPathInfoService::default());
Arc::new(SigningPathInfoService {
instance_name: "test".into(),
inner: memory_svc,
signing_key: Arc::new(
parse_keypair(DUMMY_KEYPAIR)

View file

@ -26,7 +26,7 @@ use self::utils::make_bigtable_path_info_service;
let (_, _, svc) = make_grpc_path_info_service_client().await;
svc
})]
#[case::redb(RedbPathInfoService::new_temporary().unwrap())]
#[case::redb(RedbPathInfoService::new_temporary("test".into()).unwrap())]
#[case::signing(test_signing_service())]
#[cfg_attr(all(feature = "cloud",feature="integration"), case::bigtable(make_bigtable_path_info_service().await))]
pub fn path_info_services(#[case] svc: impl PathInfoService) {}

View file

@ -53,16 +53,19 @@ pub async fn make_grpc_path_info_service_client() -> (
// Create a client, connecting to the right side. The URI is unused.
let mut maybe_right = Some(right);
let path_info_service = GRPCPathInfoService::from_client(PathInfoServiceClient::new(
Endpoint::try_from("http://[::]:50051")
.unwrap()
.connect_with_connector(tower::service_fn(move |_: Uri| {
let right = maybe_right.take().unwrap();
async move { Ok::<_, std::io::Error>(TokioIo::new(right)) }
}))
.await
.unwrap(),
));
let path_info_service = GRPCPathInfoService::from_client(
"default".into(),
PathInfoServiceClient::new(
Endpoint::try_from("http://[::]:50051")
.unwrap()
.connect_with_connector(tower::service_fn(move |_: Uri| {
let right = maybe_right.take().unwrap();
async move { Ok::<_, std::io::Error>(TokioIo::new(right)) }
}))
.await
.unwrap(),
),
);
(blob_service, directory_service, path_info_service)
}
@ -73,7 +76,7 @@ pub(crate) async fn make_bigtable_path_info_service(
use crate::pathinfoservice::bigtable::BigtableParameters;
use crate::pathinfoservice::BigtablePathInfoService;
BigtablePathInfoService::connect(BigtableParameters::default_for_tests())
BigtablePathInfoService::connect("test".into(), BigtableParameters::default_for_tests())
.await
.unwrap()
}