Repository navigation
Conversation
| .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) | ||
| } | ||
|
|
||
| pub async fn project_utilization_list( |
There was a problem hiding this comment.
This is meant to parallel silo_utilization_list. Arguably it's too parallel, though--we introduce project_quota view to match the existing silo_quota view, which doesn't (yet?) have any other consumers. If this is too much boilerplate, we can skip the new view and just write the equivalent sql with diesel here.
bnaecker
left a comment
There was a problem hiding this comment.
Looks pretty good! Some small questions and suggestions.
| now, | ||
| &target, | ||
| &project_quota::CpuProvisioned { | ||
| datum: u64::try_from(util.cpus_provisioned)?, |
There was a problem hiding this comment.
I might recommend we log this error to convert to a u64, but that we don't bail. That way we get whatever data we can, even if this conversion fails.
| s.cpus_allocated AS silo_cpus_allocated, | ||
| s.memory_allocated AS silo_memory_allocated, | ||
| s.storage_allocated AS silo_storage_allocated, |
There was a problem hiding this comment.
Do we need these columns? I don't see the silo allocations used when emitting the metrics.
There was a problem hiding this comment.
I'm dropping these columns from the view. Like you wrote below, we only use them for filtering, so I pushed that logic into the view and out of diesel.
| c.cpus_provisioned AS cpus_provisioned, | ||
| c.ram_provisioned AS memory_provisioned, | ||
| c.virtual_disk_bytes_provisioned AS storage_provisioned, | ||
| s.silo_discoverable as silo_discoverable |
There was a problem hiding this comment.
IIUC, we're filtering on this being true in the method that selects these records from the database. Do we need them, in that case? Or could we have a WHERE clause here that only selects those silos which are discoverable?
There was a problem hiding this comment.
Yep, added the WHERE here.
| pub project_id: Uuid, | ||
| pub project_name: Name, | ||
| pub silo_id: Uuid, |
There was a problem hiding this comment.
ugh, we actually do have a DbTypedUuid wrapper for db-based strongly-typed UUIDs, but I'm not sure we have typed uuids for projects or silos yet.
There was a problem hiding this comment.
Neat, I'll update here once that lands.
There was a problem hiding this comment.
Updated to use ProjectId rather than Uuid.
| } | ||
| } | ||
|
|
||
| let mut project_paginator = |
There was a problem hiding this comment.
wayyyy up on line 76 you declared the vec samples, which you're sharing for silos and projects.
I don't really have a beef with this re-use, but this structure means "an error with either side prevents any samples from being collected". Is this desired? I understand that it "highlights errors" more quickly, but it basically makes metrics collection sensitive to single-point-of-failure
There was a problem hiding this comment.
This is now out-of-date. I dunno if you want to add projects here, or reference a list somewhere else
There was a problem hiding this comment.
Updated to be more generic, and pointing to toml files for metric definitions rather than restating here.
| /** | ||
| * A view of the amount of provisioned resources on a given project. | ||
| */ | ||
| CREATE VIEW IF NOT EXISTS omicron.public.project_utilization |
There was a problem hiding this comment.
nitpick, should this exist a little closer to projects / silos?
| .or(dsl::silo_cpus_allocated.gt(0)) | ||
| .or(dsl::silo_memory_allocated.gt(0)) | ||
| .or(dsl::silo_storage_allocated.gt(0)), | ||
| ) |
There was a problem hiding this comment.
Could we do something like:
/// Silos whose utilization we report.
fn silo_utilization_reported() -> impl Expression<...> {
use nexus_db_schema::schema::silo_utilization::dsl;
dsl::silo_discoverable
.eq(true)
.or(dsl::cpus_allocated.gt(0))
.or(dsl::memory_allocated.gt(0))
.or(dsl::storage_allocated.gt(0))
.and(dsl::silo_id.ne(DEFAULT_SILO_ID))
}To share this query with the silo version?
There was a problem hiding this comment.
We could have, but I took @bnaecker's suggestion to do this filtering in the crdb view rather than DataStore, so now we don't need to repeat this filter for project_utilization_list at all.
|
|
||
| #[derive(Queryable, Debug, Clone, Selectable, Serialize, Deserialize)] | ||
| #[diesel(table_name = project_utilization)] | ||
| pub struct ProjectUtilization { |
There was a problem hiding this comment.
From what I can tell, we're never using the project_name or silo_name in this PR. Are we planning on using them?
There was a problem hiding this comment.
Oh, right, those are left over from the SiloUtilization struct I started from, and would only be used if we wanted a /v1/system/utilization/projects endpoint to match /v1/system/utilization/silos. I was thinking that would be worth adding for symmetry, but I can drop the columns here for now, and add them back if/when we add that endpoint.
1093cf4 to
2b2b73b
Compare
Analogous to #11500, but covering projects rather than silos. Project have utilization but not capacity, so we only emit utilization metrics. Note: to parallel the existing `DataStore::silo_utilization_list`, and the `silo_utilization` view that it queries, we add a `DataStore::project_utilization_list` method and `project_utilization` view. We don't add a `/v1/system/utilization/projects` endpoint to parallel `/v1/system/utilization/silos`, but we could in a future patch if useful. Part of oxidecomputer/customer-support#1383.
* Drop unused project_utilization fields, push filtering to view. * Return separate silo/project quota counts. * Clean up error handling.
2b2b73b to
bc7efb5
Compare
Analogous to #11500, but covering projects rather than silos. Project have utilization but not capacity, so we only emit utilization metrics.
Note: to parallel the existing
DataStore::silo_utilization_list, and thesilo_utilizationview that it queries, we add aDataStore::project_utilization_listmethod andproject_utilizationview. We don't add a/v1/system/utilization/projectsendpoint to parallel/v1/system/utilization/silos, but we could in a future patch if useful.Part of oxidecomputer/customer-support#1383.