Side-effect of clearing the gauge when no db rows are loaded.
This wasn't strictly necessary for the test since other manually-registered metrics are included to validate this test.

Is there a way to make this be non-nullable in the query?

Afraid not, looks like the latest_build attributes are the result of a left join:

I suppose by definition a workspace cannot exist without a build...

I see in workspace_builds:

create table if not exists public.workspace_builds
(
    id                  uuid                                                                                not null
        primary key,
    created_at          timestamp with time zone                                                            not null,
    updated_at          timestamp with time zone                                                            not null,
    workspace_id        uuid                                                                                not null
        references public.workspaces
            on delete cascade,
...

The workspace foreign key is not null, so I think this left join may be inappropriate?

Hhmm. I changed it and ran make gen but it's still being defined as a nullable field.
Also, one of its sibling fields latest_build_transition is not nullable and is derived from the same join, so actually all of the above about the join was ostensibly was invalid.

I'll need to dig a little further.

Ah! I was basically on the correct track!

provisioner_jobs was being left joined as well, and since that also has a non-nullable foreign key with workspace_builds.job_id, it is safe to use an inner join. I can actually leave the outer lateral join as it is but just change this to correct the generated code.

@johnstcn see b118044

follow-up idea: it could be useful to have this field in the codersdk.Workspace as well :-)

@johnstcn and I spoke offline to elaborate on this, apparently we convert between the database.Workspace and codersdk.Workspace types in some situations (convertWorkspace, for example), and it would be good to maintain parity between them. This would be an enhancement for a subsequent PR, we agreed.

Check this with @mafredri if it was not intentional 👍

I would avoid making this change. I believe there's a window of time when there exists a new build but it's not assigned to a job. So the nullability is something that should be handled.

This logic change essentially results in returning the previous build until the it's been assigned to a job.

I'm not sure if that's possible?

create table if not exists workspace_builds
(
    id                  uuid                                                                                not null
    ...
    workspace_id        uuid                                                                                not null
        references public.workspaces
            on delete cascade,
    ...
    job_id              uuid                                                                                not null
        unique
        references public.provisioner_jobs
            on delete cascade,
    ...
);

These keys are all non-nullable.
Is there something I'm missing?

I should say though that I'd rather not make this change if you in any way suspect this will cause issues.
The way I had it prior to #12762 (comment) worked fine.

Builds are inserted with their respective job in a single transaction. c.f. wsbuilder

Also, Danny's right that job_id is NOT NULL with a foreign key constraint, so even if we broke wsbuilder it still wouldn't be possible to have a build with no job.

Given the constraint, the change is perfectly fine 👍. I should’ve checked it myself before commenting. Sorry for the noise @dannykopping.

The DB schema should probably be updated to include NOT NULL on the foreign keys, though. Otherwise NULL entries are not enforced by the DB.

Actually, where did you get the schema @dannykopping you referenced above? Doesn’t look the same as our dump which has NOT NULL.

No worries @mafredri!

I just generated the DDL from an existing database in my IDE (GoLand). The above also has not null but you have to scroll over a bit to the right.

Haha, it was perfectly hidden by the code block size 😂.

I know right??! Sorry about that lol

Thinking loud:

During the last scale tests we hit 2000 workspaces, does it mean that we're going to swamp our Prometheus endpoint with details for all of them? If so, maybe we should make it configurable? cc @johnstcn

I think a time-series per workspace is excessive here. Operators want to know general counts of workspaces in different states. If they want to know specific workspace names they can just hit our API or look at the /workspaces page in our frontend.

This is especially true since the metrics will be reported by each Coderd, so if you have M workspaces and N Coderd replicas, that's M*N time-series that prometheus needs to track.

That's fair. I was basing the decision to include it from the original request so I'll defer to @bpmct before removing it.

See also Spike's comment: #12462 (comment)

I think what they want is a GUAGE that tracks the current number of workspaces in a particular state, not a counter of builds.

Oh wow, it seems I misunderstood the initial ask.

I was under the incorrect assumption that @bpmct wanted a metric to match the format described in the issue, and "the builds metric" which he was referring to was the existing coderd_api_workspace_latest_build_total metric 🤦

coderd_workspace_builds_total already exists!

# HELP coderd_workspace_builds_total The number of workspaces started, updated, or deleted.
# TYPE coderd_workspace_builds_total counter
coderd_workspace_builds_total{status="failed",template_name="docker",template_version="vibrant_davinci2",workspace_name="asfsasfafs",workspace_owner="danny",workspace_transition="START"} 1
coderd_workspace_builds_total{status="success",template_name="docker",template_version="beautiful_visvesvaraya1",workspace_name="2",workspace_owner="danny",workspace_transition="START"} 1
coderd_workspace_builds_total{status="success",template_name="docker",template_version="beautiful_visvesvaraya1",workspace_name="asfsasfafs",workspace_owner="danny",workspace_transition="START"} 1
coderd_workspace_builds_total{status="success",template_name="docker",template_version="beautiful_visvesvaraya1",workspace_name="asfsasfafs",workspace_owner="danny",workspace_transition="STOP"} 1
coderd_workspace_builds_total{status="success",template_name="docker",template_version="mystifying_driscoll3",workspace_name="2",workspace_owner="danny",workspace_transition="STOP"} 1

Plus we don't have to worry about cardinality because these metrics are written by the provisioner when executing a job, not reading from the database state.

I have amended the new metric to drop the workspace label.

I think this will satisfy the requirements but @bpmct please confirm.

# HELP coderd_workspace_latest_build_status The current workspace statuses by template, transition, and owner.
# TYPE coderd_workspace_latest_build_status gauge
coderd_workspace_latest_build_status{status="failed",template_name="docker",template_version="happy_feynman7",workspace_owner="danny",workspace_transition="stop"} 1
coderd_workspace_latest_build_status{status="succeeded",template_name="docker",template_version="beautiful_visvesvaraya1",workspace_owner="danny",workspace_transition="start"
} 2
coderd_workspace_latest_build_status{status="succeeded",template_name="docker",template_version="distracted_hopper6",workspace_owner="danny",workspace_transition="start"} 1
coderd_workspace_latest_build_status{status="succeeded",template_name="docker",template_version="ecstatic_greider3",workspace_owner="danny",workspace_transition="start"} 1

I think this will satisfy the requirements but @bpmct please confirm.

# HELP coderd_workspace_latest_build_status The current workspace statuses by template, transition, and owner.
# TYPE coderd_workspace_latest_build_status gauge
coderd_workspace_latest_build_status{status="failed",template_name="docker",template_version="happy_feynman7",workspace_owner="danny",workspace_transition="stop"} 1
coderd_workspace_latest_build_status{status="succeeded",template_name="docker",template_version="beautiful_visvesvaraya1",workspace_owner="danny",workspace_transition="start"
} 2
coderd_workspace_latest_build_status{status="succeeded",template_name="docker",template_version="distracted_hopper6",workspace_owner="danny",workspace_transition="start"} 1
coderd_workspace_latest_build_status{status="succeeded",template_name="docker",template_version="ecstatic_greider3",workspace_owner="danny",workspace_transition="start"} 1

Yep!

this has a race condition where if the registry collects the metrics before the for-loop below is complete, it will get partial data. This can result in glitchy stats were they are zeroed out for a single data point and then return to "normal." If anyone puts alerts on those stats it could result in false alerting in addition to visual noise on the dashboard.

I realize this is following the pattern of the existing metric, which has the same bug.

Instead these stats should be a custom collector that queries the database and returns the requested data on demand.

Instead these stats should be a custom collector that queries the database and returns the requested data on demand.
Definitely agree! Called this out in the description as well.

I'm not sure if we can fix this race without the change to the collector pattern. Do you have any ideas?
In reality the race would have to be on the nanosecond scale because this loop will iterate very quickly, but your point is absolutely valid.

I'll add a follow-up issue for this in any case.

#12792

nit: maybe logger.Error?

I wouldn't strictly call it an error because it could be an instance of sql.ErrNoRows which is valid but undesirable. To be the most correct I would make it warn in case of sql.ErrNoRows and error in all other cases, but that feels unnecessary.

which is valid but undesirable

Will it also return sql.ErrNoRows if there are zero workspaces = fresh deployment?

Believe so, yes.
Or if all workspaces have been deleted.