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 thelatest_build attributes are the result of a left join:

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

I see inworkspace_builds:

createtableif not existspublic.workspace_builds(    id                  uuidnot nullprimary key,    created_attimestamp with time zonenot null,    updated_attimestamp with time zonenot null,    workspace_id        uuidnot nullreferencespublic.workspaceson delete cascade,...

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

Hhmm. I changed it and ranmake gen but it's still being defined as a nullable field.
Also, one of its sibling fieldslatest_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 withworkspace_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 seeb118044

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

@johnstcn and I spoke offline to elaborate on this, apparently we convert between thedatabase.Workspace andcodersdk.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?

createtableif not exists workspace_builds(    id                  uuidnot null    ...    workspace_id        uuidnot nullreferencespublic.workspaceson delete cascade,    ...    job_id              uuidnot null        uniquereferencespublic.provisioner_jobson 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 isNOT NULL with a foreign key constraint, so even if we brokewsbuilder 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 hasnot 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 metricto match the format described in the issue, and "the builds metric" which he was referring to was the existingcoderd_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 countercoderd_workspace_builds_total{status="failed",template_name="docker",template_version="vibrant_davinci2",workspace_name="asfsasfafs",workspace_owner="danny",workspace_transition="START"} 1coderd_workspace_builds_total{status="success",template_name="docker",template_version="beautiful_visvesvaraya1",workspace_name="2",workspace_owner="danny",workspace_transition="START"} 1coderd_workspace_builds_total{status="success",template_name="docker",template_version="beautiful_visvesvaraya1",workspace_name="asfsasfafs",workspace_owner="danny",workspace_transition="START"} 1coderd_workspace_builds_total{status="success",template_name="docker",template_version="beautiful_visvesvaraya1",workspace_name="asfsasfafs",workspace_owner="danny",workspace_transition="STOP"} 1coderd_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 theworkspace label.

Ithink 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 gaugecoderd_workspace_latest_build_status{status="failed",template_name="docker",template_version="happy_feynman7",workspace_owner="danny",workspace_transition="stop"} 1coderd_workspace_latest_build_status{status="succeeded",template_name="docker",template_version="beautiful_visvesvaraya1",workspace_owner="danny",workspace_transition="start"} 2coderd_workspace_latest_build_status{status="succeeded",template_name="docker",template_version="distracted_hopper6",workspace_owner="danny",workspace_transition="start"} 1coderd_workspace_latest_build_status{status="succeeded",template_name="docker",template_version="ecstatic_greider3",workspace_owner="danny",workspace_transition="start"} 1

Ithink 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 gaugecoderd_workspace_latest_build_status{status="failed",template_name="docker",template_version="happy_feynman7",workspace_owner="danny",workspace_transition="stop"} 1coderd_workspace_latest_build_status{status="succeeded",template_name="docker",template_version="beautiful_visvesvaraya1",workspace_owner="danny",workspace_transition="start"} 2coderd_workspace_latest_build_status{status="succeeded",template_name="docker",template_version="distracted_hopper6",workspace_owner="danny",workspace_transition="start"} 1coderd_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: maybelogger.Error?

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

which is valid but undesirable

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

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