[jmcarp]

This patch adds two groups of metrics: silo_quota:{memory,cpu,disk}_capacity, which represents resource quota limits per silo; and silo_quota:{memory,cpu,disk}_provisioned, which represents resources charged against each quota.

We currently have two features that almost solve this problem, but aren't quite adequate: the collection_target metric, and the /v1/system/utilization/silos endpoint. The collection_target metric exposes quota utilization, mixing silos and projects. However, it's sparse, emitting values only when utilization changes, which doesn't play nicely with use cases that expect regular timeseries. It also doesn't expose quota limits. The silo utilization endpoint exposes quota limits, but isn't a metric.

We work around this problem in the OpenTelemetry metrics receiver, which can optionally emit synthetic capacity and utilization metrics by periodically querying Nexus. However, this requires users to run our otel collector, or implement their own synthetic metrics--neither of which is an ideal experience.

The metrics proposed here allow users to query silo quota capacity and utilization via OxQL or other metrics backends, without post-processing sparse metrics or deriving their own custom quota metrics.

Note: we collect these metrics using a new CapacityMetrics background task, which runs on each Nexus instance. As a result, we emit those metrics one per Nexus--in triplicate, by default. To handle that duplication, users should aggregate (max, mean) over nexus_id.

Part of oxidecomputer/customer-support#1383.

cc @smklein for quota context, @bnaecker for metrics context, @hermanol because we've had some support requests for this and related features. The new task is roughly modeled on InstanceWatcher, and hopefully isn't very interesting at a code level. I'd like to add some related metrics in the future, but wanted to establish a pattern for emitting capacity metrics first.

[bnaecker]

I'm going to look at the implementation tomorrow, but I feel like the naming could use a little bikeshedding. Maybe something like silo_quota:{memory,cpu,disk}_{capacity,provisioned}? E.g., silo_quota:memory_capacity and silo_quota:memory_provisioned? I might be nitpicking, but the names were pretty confusing to me a first glance.

[hawkw]

we should probably add an implementation for this in omdb::nexus::print_task_details, rather than letting it fall through to the "warning: unknown background task..." behavior.

[jmcarp]

Thanks, added.

[jmcarp]

I might be nitpicking, but the names were pretty confusing to me a first glance.

Nitpicking is welcome! Conceptually, I was thinking of a set of quota metrics capturing:

• silo | project
• capacity | provisioned
• memory | cpu | disk

I haven't added project-scoped metrics here, in the interest of keeping the patch small, but I'd like to get to that in a follow-up. So the full set of metric names under the proposed scheme would eventually look like:

• quota_capacity_silo:{memory,cpu,disk}
• quota_provisioned_silo:{memory,cpu,disk}
• quota_provisioned_project:{memory,cpu,disk}

So what you're proposing would probably look like:

• silo_quota:{memory,cpu,disk}_{capacity,provisioned}
• project_quota:{memory,cpu,disk}_provisioned

I've been staring at these names for too long, but I think yours make sense, and it feels more correct to capture capacity/provisioned in the metric name instead of the target name. I had started all the names with quota_ so that quota-related metrics would sort together, but quota_silo sounds kind of silly. Anyway, I'm happy to switch to the naming pattern you're suggesting--I'll just do that unless anybody has better ideas or wants to discuss more.

[jmcarp]

Anyway, I'm happy to switch to the naming pattern you're suggesting

Did this--and sorry, I force-pushed out of habit, although I also wanted to change the commit message so that it would get the new names right.

[bnaecker]

Nice, this is great. I have a few comments, though overall looks solid to me.

[bnaecker]

I think we can move this lock to after the loop. Compute all the metrics, lock once, and move them all onto the queue, then drop the lock.

[bnaecker]

I'm not sure about putting the nexus_id field on the timeseries. On the one hand, I get it that it's useful to dedupe the data. OTOH, it doesn't really provide any useful context for the data itself.

I think we have a few options:

• Leave it
• Remove it, relying on the timestamps themselves for deduplication. E.g, use something like mean_within() for alignment or the equivalent in a 3p tool / language.
• Remove it, and do something complicated to try to avoid reporting values from different Nexus "close by" in time.

I'm inclined toward the 2nd option, but WDYT?

[hawkw]

• Remove it, relying on the timestamps themselves for deduplication. E.g, use something like mean_within() for alignment or the equivalent in a 3p tool / language.

IIUC some external metrics systems (like Prometheus) get Very Sad when we have multiple data points in one timeseries with no labels to disambiguate them. I guess here, that would only happen if two nexuses HAPPENED to emit such a data point with exactly the same timestamp, which seems unlikely, but...

[jmcarp]

some external metrics systems (like Prometheus) get Very Sad when we have multiple data points in one timeseries with no labels to disambiguate them.

Yes--for example, oxidecomputer/hubris#2634. Prometheus also represents timestamps in ms, not ns as in oximeter, so collisions are more likely there (although still not at all likely, and users who want to export to prometheus-like stores can deduplicate on their end if necessary).

The robots also helpfully pointed out that the oximeter read path can panic on duplicate timestamps--although again, with nanosecond precision, this is extremely unlikely, and I believe we can fix that by changing a > 0 to >= 0, and it's not an architectural assumption in oximeter.

Anyway, I have a slight preference for keeping the nexus_id label, both because it avoids potential sadness in downstream metrics stores related to duplicate samples, and because--and I know this is a weak argument--labeling by nexus zone gives us n regularly-spaced series per silo, and omitting the label gives us a single series with weird and variable intervals between samples. I don't think either of these is particularly strong, which is why I'm calling it a slight preference, and I'm happy to drop that label if others prefer.

[bnaecker]

Is there a difference between "capacity" and "quota"? Should we use the same term in the name and description?

[jmcarp]

I think we have a few ways of describing these quantities already:

• In /v1/utilization, we say capacity for the amount of resource available to be provisioned, and provisioned for the amount actually provisioned.
• In /v1/system/utilization/silos, we use allocated and provisioned, respectively.
• In /v1/system/silo-quotas, we just use an object of cpus, memory, storage, without referring to quota, capacity, etc.

My thinking here was to try and stay consistent with existing APIs, although they're not entirely consistent as they are. I personally prefer capacity/provisioned to allocated/provisioned, so that's what I used to begin with. I think quota makes sense as well, but I think it's less consistent with existing APIs, and arguably that would mean overloading quota to mean both the resource defined in /v1/system/silo-quotas, as well as each per-resource limit within that resource. So I think I'd lean toward leaving the metric names as they are, and changing the descriptions to match.

[bnaecker]

Makes sense to me. I was mostly asking about the different terms in the names and descriptions. I agree the rest of the API is a little confused, but it's good this is all self-consistent.

[bnaecker]

Does this also include the snapshot usage? The description for disk_capacity makes it seem like it should, which we should make clear here too.

[jmcarp]

Yes, fixing this. I'm also realizing that I should have called these metrics storage and not disk to match existing APIs, so I'll change that too.

[smklein]

These instructions seem like they're intended for someone consuming the metrics through the API, but they aren't going to be visible in customer docs as written. Should this advice exist somewhere else?

Without this, it seems possible that the values will be jittery, as each Nexus races to insert values?

[jmcarp]

Yes, adding a note in silo-quota.toml.

[smklein]

At the cost of fine-pointed accuracy here - we COULD make the collection_target metric a background polling task too, if it would be more useful to have regularly-emitted values. I'm not asking for that change in this PR, but rather, offering that it seems fine to me to change that behavior.

[jmcarp]

Right, I'll make the comment more precise. In some sense, we could also think about turning collection_target:* into the new proposed metrics: sample it on an interval in a background task, add capacity, separate silo collections from project collections--but at least while oximeter metric labels are immutable, it seems easier to add a new metric than to reshape the existing one.

[smklein]

This filters out the default silo + non-discoverable silos, as an FYI. Is that intentional?

[jmcarp]

It's intentional, but not necessarily correct! My thought was to match /v1/system/utilization/silos. It sounds like the default silo concept is meant to be retired, so I didn't think too hard about it. And it does look like non-discoverable silos will be returned here, if they have non-zero quota capacity:

        .filter(
            dsl::silo_discoverable
                .eq(true)
                .or(dsl::cpus_allocated.gt(0))
                .or(dsl::memory_allocated.gt(0))
                .or(dsl::storage_allocated.gt(0)),
        )
        .filter(dsl::silo_id.ne(DEFAULT_SILO_ID))

My weakly-held opinion here is that the new metric should match /v1/system/utilization/silos, and I'm agnostic about whether we should show non-discoverable silos without quotas set--but I would lean toward showing those silos for both views, or neither.

[smklein]

Do we want to be using max_page, rather than paginating? This could make very expensive queries if we have a lot of silos

[jmcarp]

Switching to pagination.

[smklein]

I believe that cast_unsigned would convert a negative value into an extremely large value - would it be better to clamp this at zero?

[jmcarp]

Yes, fixing.

[smklein]

I know you're making this pub for a test, but it's doing the same thing as project_create, just skipping an authz lookup.

This seems a little sketchy to expose as pub - is it really not possible to use project_create in that test?

[jmcarp]

Reverting. I wound up creating a test user and giving it silo access, which is boilerplate I was trying to avoid, but seems better than changing method access just for the test.

[smklein]

You might want to look into oximeter::queue::BoundedQueue. Otherwise, if oximeter goes down, this will grow indefinitely

[jmcarp]

Switched to a BoundedQueue<Sample>.

[hawkw]

turbo nit: perhaps:

Suggested change

	"warning: failed to interpret task details: {:?}: {:?}",
	error, details
	"warning: failed to interpret task details: {error:?}: {details:?}",

[hawkw]

this is a nitpick, and it's not a huge deal here because the status object only has one field in it, but it's worth noting that a lot of the other background tasks have a single Rust type defined in nexus_types::internal_api::background that is used both when serializing the status object in Nexus and when deserializing it in omdb. We could do that here, and it might be a little bit nicer --- though that being nicer would matter a bit more if there was more data here...

[jmcarp]

Done. Maybe we'll add more data later--I do want to add more metrics here once we agree on the basics.

[hawkw]

• Remove it, relying on the timestamps themselves for deduplication. E.g, use something like mean_within() for alignment or the equivalent in a 3p tool / language.

IIUC some external metrics systems (like Prometheus) get Very Sad when we have multiple data points in one timeseries with no labels to disambiguate them. I guess here, that would only happen if two nexuses HAPPENED to emit such a data point with exactly the same timestamp, which seems unlikely, but...

[jmcarp]

Ready for another look--I think everything is addressed. I think the only open question is about whether or not to include the nexus_id label, because I can see the arguments either way, and we may not have consensus. Although I'm tentatively proposing that we keep it for reasons given above.

[jmcarp]

I chatted with @lgfa29 about this, and continue to think including the nexus_id label is less bad than omitting it. At least until we implement metrics schema versioning, we won't be able to add labels to these metrics after they make it into a release (unless we use a workaround like a silo_quota_v2). So if we omit the label and later discover that duplicate or near-duplicate samples cause problems in a user's tsdb, we wouldn't have any great options available. Maybe we'll make a change later on that makes the label less relevant--we could include a best-effort deduplication via lease, as in WebhookDeliverator, or move scheduled control plane metrics to a separate singleton service--in which case nexus_id would reduce to a label with cardinality of (almost always) one, but wouldn't cause any harm.

Anyway, let me know if the above is unconvincing, or if the patch needs other changes.

[jmcarp]

By the way, a quick sanity check that the new metrics work. Thanks @fakemonster for the new metrics explorer!

[smklein]

Since you're asking for naming nitpicking: One thing I'm realizing is that the usage of "storage" here refers to "virtual storage", not "physical storage". If we add metrics for "physical storage" later, it'll be hard to detangle these two.

As an example: Crucible storage being distributed, so asking for "1 GiB virtual" actually uses ~3.75 GiB physical storage.

Do we want to label this virtual_storage_capacity, or otherwise have some way of making clear that it isn't physical?

[jmcarp]

Yes, I think this is right, especially given we're talking about changing and/or adding storage quotas and quota metrics in https://rfd.shared.oxide.computer/rfd/0624. I'm going to rename the storage metrics, then merge.