This metric counts workspaces whose first build succeeded. If the first build failed and a later build succeeded, that workspace is not included. We could change the query to count the first successful build instead (wd.transition='start' & pj.job_status='succeeded'); however, we would still need to exclude prebuild-created workspaces by verifying that the first build was not initiated by the prebuilds system user.
It is guaranteed that the first build of a prebuilt workspace will always be initiated by the prebuilds system user, right?

It is guaranteed that the first build of a prebuilt workspace will always be initiated by the prebuilds system user, right?

This assumption would fail if an admin were to create a workspace 'on behalf of' the prebuilds user. I don't see a logical reason why someone would do this though, as the reconciler would likely try to delete it?

This assumption would fail if an admin were to create a workspace 'on behalf of' the prebuilds user.

Is this possible? If yes, should we allow this?

as the reconciler would likely try to delete it?

The reconciliation loop would detect if the number of desired instances matches the number of running instances. If that were not the case, it would determine there is an extraneous instance, but it wouldn't necessarily delete this one; it just deletes the oldest prebuilt workspace: https://github.com/coder/coder/blob/main/coderd/prebuilds/preset_snapshot.go#L327

Is this possible?

Yes, it's possible and supported to create a workspace for another user:

$ coder create --help                          
coder v2.25.1-devel+dd867bd74

USAGE:
  coder create [flags] [workspace]

  Create a workspace

    - Create a workspace for another user (if you have permission):
  
        $ coder create <username>/<workspace_name> 

If yes, should we allow this?

Good question :) It gets very weird to reason about especially if the workspace that gets created is a prebuilt workspace...

I would consider it out of scope of this issue.

Addressed in: 11d7ed3
Updated the SQL to get the first successful start build for each workspace, then exclude workspaces where that build was initiated by the prebuilds system user.

This PR introduces histogram metrics that are registered at the API layer but observed in provisionerdserver. Because provisionerdserver doesn’t own a Prometheus registry, we inject a callback from the API to record observations. Both metrics are updated in provisionerdserver’s completeWorkspaceBuildJob.
Let me know what you think, or if it would be better to have the metrics on provisionerdserver or maybe even update them in another more appropriate place.

We already have set a precedent in enterprise/coderd/prebuilds.NewStoreReconciler, where we inject api.PrometheusRegistry. Would it make sense to do the same here for consistency?

Yes, that makes sense 👍 Just wanted to check whether there would be a more appropriate place to introduce these metrics, before making that change

Addressed in b59ebad
Introduced a new metrics file in provisionerdserver package. Since each provisioner daemon gets its own provisionerdserver instance, the provisionerdserver metrics are created once in enablePrometheus() and passed down to each provisionerdserver instance. This ensures all daemons share the same metrics objects and avoids Prometheus registration conflicts.

This follows the existing pattern used for provisionerd.Metrics, which are created once in cli/server.go and shared across all in-memory provisioner daemons: https://github.com/coder/coder/blob/main/cli/server.go#L1004

These are the native histogram parameters. If we choose this approach, we need to (edit: should) ensure our Prometheus server supports native histograms (still experimental) before deploying to dogfood. Additionally, we should also update the documentation to note that we use on this Prometheus feature.
As described in the PR, if Prometheus is not started with native histograms enabled, the metrics will automatically fall back to standard bucketed histograms using the bucket scheme listed above (aligned with the existing provisionerd metrics).

Note: these native histograms were tested locally with Prometheus v3.5.0

we need to ensure our Prometheus server supports native histograms (still experimental)

I wouldn't assume that existing customers have an experimental Prometheus feature enabled. But as you say, there is a transparent fallback so this shouldn't be an issue?

But as you say, there is a transparent fallback so this shouldn't be an issue?

This shouldn’t be an issue while customers are using classic histograms, but there will inevitably be a migration phase once customers want to adopt native histograms (or if we decide to enforce them). From https://prometheus.io/docs/specs/native_histograms/#migration-considerations

Classic and native histograms cannot be aggregated with each other. A change from classic to native histograms at a certain point in time makes it hard to create dashboards that work across the transition point, and range vectors that contain the transition point will inevitably be incomplete (i.e. a range vector selecting classic histograms will only contain data points in the earlier part of the range, and a range vector selecting native histograms will only contain data points in the later part of the range).

From my perspective, we have three paths forward:

1. Enforce native histograms only:

• Customers would need to explicitly enable the feature on their Prometheus server today, to be able to see these metrics.
• This is risky, since not all customers may be willing (or able) to do so yet.
• Given that this feature was requested by a customer, this option might be a non-starter.

2. Support both native and classic histograms (the approach proposed in this PR):

• Provides the best compatibility: customers without native enabled continue using classic, while those with it enabled get the benefits.

• At some point, there would still need to be a migration once customers decide to use native histograms (either by explicitly enabling it or once it becomes the default)

• There are some interesting considerations here: https://prometheus.io/docs/specs/native_histograms/#migration-considerations

• For our dogfood setup, it might make sense to mirror the expected customer journey:

  1. Run without native histograms.
  2. Enable native histograms in parallel with classic.
  3. Fully transition to native.

• This way, we can validate the migration path ourselves and uncover issues before customers do.

3. Stick with classic histograms only:

• Safest and simplest option in the short term.
• We can still migrate later once the feature becomes stable.
• At that point, we could migrate all histogram metrics together, not just these new ones.

Without fully understanding the implications yet, my weak preference is to tread the expected path and find issues first!

I prefer option #2. A graceful fallback seems like a good idea, particularly given how wide you've made the fallback buckets. The metrics will be less useful, but still directionally helpful (i.e. you probably don't care if a claim took 5m or 6m or 7m or 10m to claim; anything above 5m is just awful).

We should add a follow-up to add the native histogram flag to coder/observability, and ensure everything works out-of-the-box. I'd prefer we did that before we merge this, otherwise proof of local validation using the flag will suffice.

Native histograms successfully tested in coder/observability: coder/observability#50
Will finish this to make it ready for the release, and then continue with the observability draft PR to also introduce Grafana dashboards for the metrics here introduced.

I prefer option #2. A graceful fallback seems like a good idea, particularly given how wide you've made the fallback buckets. The metrics will be less useful, but still directionally helpful (i.e. you probably don't care if a claim took 5m or 6m or 7m or 10m to claim; anything above 5m is just awful).

We should add a follow-up to add the native histogram flag to coder/observability, and ensure everything works out-of-the-box. I'd prefer we did that before we merge this, otherwise proof of local validation using the flag will suffice.

I'd prefer we have higher resolution between 1 and 5m, since that'll actually be helpful, and cap at 5m because anything beyond 5m is so horrific that prebuilds are basically useless.

That’s a good point. I wasn’t completely sure what appropriate claim time values would be, since that can vary a lot from customer to customer. For this first draft, I just reused the same buckets we already use for other histograms, like https://github.com/coder/coder/blob/main/provisionerd/provisionerd.go#L203-L211 for workspace build times.

I agree with having higher resolution between 1m and 5m, I’m just not sure about capping at 5m 🤔 Looking at those existing values, it seems we’ve had builds that take between 30 minutes and 1 hour. Do we still expect claim times to reliably stay under 5 minutes even in those cases? Just curious how you arrived at the 5m mark 🙂

Looking at those existing values, it seems we’ve had builds that take between 30 minutes and 1 hour

For claims?

Do we still expect claim times to reliably stay under 5 minutes even in those cases? Just curious how you arrived at the 5m mark 🙂

If a prebuild claim takes longer than 5m, there's no point in using prebuilds. I chose 5m since it was the most realistic upper bound of the buckets you listed.

For claims?

No, the bucket definitions I was referring to are for workspace builds, more precisely: coderd_provisionerd_workspace_build_timings_seconds https://github.com/coder/coder/blob/main/provisionerd/provisionerd.go#L203-L211.
These record the time taken for a workspace to build, and I believe they were introduced before prebuilds existed. Based on these buckets, my assumption was that some workspace builds (without prebuilds) have taken on the order of 30m–1h, so I was curious whether in those cases a claim from a prebuilt workspace would still reliably fall under 5m.

Let's come back to the point of these metrics... Operators need to know when claims are taking too long, right? Any prebuilt workspace claim which exceeds 5m is pointless. Why do we need any more resolution above a threshold like this?

We're talking only about the fallback case here, btw. If we have native histograms then operators can see the "real" distribution, if they really need to.

We're talking only about the fallback case here, btw. If we have native histograms then operators can see the "real" distribution, if they really need to.

True, but my guess is that most customers with Prometheus are not using native histograms, so they will most likely use the classic histograms.

Why do we need any more resolution above a threshold like this?

I'm ok with capping at 5m. I was mainly curious if there’s already an agreement that 5 minutes is the maximum acceptable claim time for prebuilt workspaces to still be considered effective (especially taking into account longer build times for workspaces without prebuilds)

Addressed the bucket definition for the claim histogram in 11d7ed3

It feels a bit awkward to have this namespaced in this way as opposed to under coderd_prebuilt_workspaces.

Yes, I agree. Right now, the histogram metrics are set in the provisionerd server, which is why the subsystem was originally defined as provisionerd. I can rename this one to prebuilt_workspace, but then for consistency, we should also update workspace_creation_duration_seconds.

An alternative would be to drop the provisionerd subsystem altogether and just go with:

• coderd_workspace_creation_duration_seconds
• coderd_prebuilt_workspace_claim_duration_seconds

An alternative

Yeah that seems like a good solution. We already have metrics like coderd_workspace_builds_total.

Addressed in 11d7ed3

Note: The tests included in this PR are not exhaustive. I plan to add additional test cases to cover more use cases in a follow-up PR. Keeping this PR focused helps avoid delaying the release of this feature.

This SQL query follows a similar pattern as GetPrebuildMetrics

This query (in combination with others in prometheusmetrics) will be executed every defaultRefreshRate, which is currently set to 1 minute. An EXPLAIN was run in dogfood's database and the results are here: https://explain.dalibo.com/plan/6f96bfac3baac3d6

Is it necesary to look at all workspace builds ever, or do we need to look at builds in a certain time window?

The explain output shows a number of seq scans which we probably want to turn into at least index scans.

Is it necesary to look at all workspace builds ever, or do we need to look at builds in a certain time window?

This is to account for the edge case where a workspace was not successfully provisioned on the first workspace build, that is why we are getting the first successful start workspace build. I would say that in most cases the first workspace build will be successful, so maybe we don't need this level of detail for the counter and we can just look at the first workspace build (like we did in the initial version of the query)

The explain output shows a number of seq scans which we probably want to turn into at least index scans.

Yes, that is the main issue here 😕 I can try to create some indexes to improve the performance here.
(edit: actually it seems the biggest bottleneck here is the ORDER BY wb.workspace_id, wb.build_number, wb.id)

It would be nice if we could add metadata to describe if this is the first regular workspace build, similar to what we do with PrebuiltWorkspaceBuildStage_CREATE 🤔