metrics: report a worker with no sandbox class as unknown

[Jeff Luo (JeffLuoo)]

Fixes #1475

Control.CreateWorker does not validate Worker.sandbox_class, so a client can register a worker with an empty one (and any record written before this keeps it). RegisterWorkerCount tallied that raw value, so ate.workerpool.workers emitted ate_sandbox_class="" — not a member of the ate.sandbox.class registry vocabulary — alongside the pool's seeded series at 0.

This change normalizes the worker's class with the existing ateattr.NormalizeSandboxClass, so an empty class reports as unknown.

• Tests pass
• Appropriate changes to documentation are included in the PR

[Jeff Luo (JeffLuoo)]

Actually, before making this change I have a question about the scheduling - Julian Gutierrez Oschmann (@juli4n)

Worker.sandbox_class is optional:

substrate/pkg/proto/ateapipb/ateapi.proto

Lines 1688 to 1693 in a223367

	// sandbox_class mirrors the WorkerPool's sandboxClass; its values are the
	// CRD's own vocabulary, so it is only bounded, not validated. Mutable.
	//
	// +k8s:optional
	// +k8s:maxLength=63
	string sandbox_class = 8;

and it doesn't have a default value. But this field will be used by scheduler to schedule the worker. CreateWorker accepts "" and nothing fills it in. Should we validate it from the API level?

[Benjamin Elder (BenTheElder)]

So, I also want to mention that I think workers will have multiple sandbox classes in the future. Actors will be on a specific sandbox class but the idea that we reserve compute only for gvisor or microVM is an artificial distinction that probably won't hold up as we incrementally improve the dataplane.

would that suggest a different approach here?

[Jeff Luo (JeffLuoo)]

So, I also want to mention that I think workers will have multiple sandbox classes in the future. Actors will be on a specific sandbox class but the idea that we reserve compute only for gvisor or microVM is an artificial distinction that probably won't hold up as we incrementally improve the dataplane.

would that suggest a different approach here?

Thank you for the context. I patch the PR to drop the change in earlier revision had an empty-class worker inherit its pool's class. This PR only normalizes empty string sandbox class to unknown like other metrics.

For the impact of "workers will have multiple sandbox classes in the future" to the metric ate.workerpool.workers, the sum across sandbox classes stops equaling the fleet size. My guess is that worker counts lose the class label?

cc Krisztian F (@krisztianfekete) we may want to revisit some metrics when this happens.

[Krisztian F (krisztianfekete)]

Approach looks right to me, two things:

1. The same empty class is still on ate.actor.crashes.
   Same bug as #1475, on an attribute the registry marks required. Maybe we should have a tiny SandboxClassAttribute helper in
   ateattr so every emitter shares one rule. Fine as a follow up, but please file it
   from this PR so it doesn't close along with #1475.

2. sum by (pool) (ate_workerpool_workers{state="idle"}) still returns 3 for that
   pool, because the unknown series keeps the pool labels. Not a regression, as ""
   counted the same way, but as per #1475 these should not not count as pool capacity, and that only happoens if the query includes class.

[Krisztian F (krisztianfekete)]

So, I also want to mention that I think workers will have multiple sandbox classes in the future. Actors will be on a specific sandbox class but the idea that we reserve compute only for gvisor or microVM is an artificial distinction that probably won't hold up as we incrementally improve the dataplane.

would that suggest a different approach here?

I don't think it changes this PR. We should still do this regardless of how many classes a worker ends up having.

For the impact of "workers will have multiple sandbox classes in the future" to the metric ate.workerpool.workers, the sum across sandbox classes stops equaling the fleet size. My guess is that worker counts lose the class label?

Today, the instrument's whole contract is that the counts are summable, so sum by (pool) = pool size and sum over pools = fleet size, which is why it's an UpDownCounter and not a gauge. Once a worker has a classes, a per-class dimension will double count it. If we drop the class label all is correct, but we cannot answer the "capacity per kind" question, which is something we'd like to answer for sure.

So I'd expect it to split, so worker counts stay class-less and summable, and maybe eligibility-by-class becomes its own instrument, where overlaps are fine, and no one wants to sum them. That's ~what ate.scheduler.eligible_workers already is.

Not worth designing now while the dataplane is still evolving, and I'd land the label fix first and revisit when the multi-class model becomes stable. Do you mind opening on issue for this?