feat(executor): add OTel metrics for the TaskAction garbage collector

[davidlin20dev]

What changes were proposed in this pull request?

Adds OpenTelemetry metrics for the executor's TaskAction garbage collector, registered on the executor's existing meter provider.

metric	type	labels
taskaction.gc.deletions	Int64Counter	outcome = deleted | already_gone | failed
taskaction.gc.sweep.duration	Float64Histogram (ms)	error = true | false

This replaces the promutils implementation this PR originally carried, per review feedback that OTel is the direction for executor metrics. The pattern follows executor/pkg/controller/metrics.go from #7483.

A few design notes:

• Registration failure is non-fatal. If instrument registration fails, the GC runs unmetered rather than blocking startup, matching registerTaskActionMetrics.
• Deletions are labeled by outcome, not a boolean. A delete attempt has three results: it succeeded, the object was already gone (Kubernetes cascade-deleted it when the GC removed its parent earlier in the same sweep), or it failed. A boolean can't express the middle case, and skipping it undercounts what the sweep actually removed.
• The sweep histogram keeps a boolean error label, since a sweep has only two outcomes.
• The histogram overrides the SDK's default bucket boundaries, which top out at 10s. A sweep over a large backlog runs for minutes, so the defaults would collapse every slow sweep into the overflow bucket and make it impossible to tell a 12-second sweep from a 4-minute one.

Why are the changes needed?

The executor gets reconcile counts and workqueue metrics for free from controller-runtime, but nothing reports garbage collection activity: how much is being deleted, whether deletes are failing, or how long a sweep takes. See #7455.

How was this patch tested?

• Unit tests for the instruments: a noop provider returns no metrics, deletions record under each of the three outcomes, sweep duration records with its error label, and recording through a nil metrics struct is a safe no-op.
• An envtest integration spec that runs the real collect() against a live API server with a real meter provider and asserts the recorded values, so the call sites in collect() are covered rather than just the helpers.
• Verified the spec catches regressions: it fails if a recording call is removed from collect().

Labels

Related to #7455

[Sovietaced]

Is it possible to rework these using OTEL instead of prometheus?

[davidlin20dev]

Is it possible to rework these using OTEL instead of prometheus?

Hi @Sovietaced, happy to switch these to OTel.

I went with Prometheus because #7445 and #7455 (umbrella issue) are both centered around promutils, so that's
the direction I followed. Before I rework it, though, I just want to make sure I'm building the right thing.

@pingsutw, since you opened these issues: could you confirm whether OTel is the preferred
direction now, and whether the issues should be updated to reflect that? Happy to rework this
on the meter provider (mirroring #7483) once I know which way to go.

Thanks both, appreciate the help!

[pingsutw]

@davidlin20dev thanks for working it. Sorry for the late reply, I was OOO. yes, we decide to use OTEL

[davidlin20dev]

@davidlin20dev thanks for working it. Sorry for the late reply, I was OOO. yes, we decide to use OTEL

@pingsutw thanks for confirming! quick question: should I wait for the issue to be updated, or is it fine to start the OTel rework now?

[pingsutw]

It's fine to start the work now

[davidlin20dev]

Hi @pingsutw, I'm working on the OTel rework now, and there's one design decision I'm debating on the deletions counter.

Right now I'm labeling it error=true|false, and skipping NotFound entirely. If I'm reading the comment in collect() right, those are usually objects k8s already cascade-deleted when the parent was removed in the same sweep, so the count might end up lower than what actually got cleaned up.

I'm thinking of labeling by outcome with three values (deleted, already_gone, failed) instead, so the total reflects everything that was actually deleted. Let me know if this makes sense, and I'd appreciate your input on this!

[davidlin20dev]

Pushed the OTel rework. On the question above, I went ahead and implemented the outcome version (deleted / already_gone / failed) so there's something concrete to look at. I can switch it back to the boolean if needed. Please take a look.

[pingsutw]

LGTM, thanks. Could you also test it in the devbox and help update the grafana dashboard?
I added a make target for the devbox to install the grafana stack, so you can easily to test it in the devbox.

[davidlin20dev]

Thanks! Yeah sure, I'll take a look at the devbox and the dashboard.

[davidlin20dev]

LGTM, thanks. Could you also test it in the devbox and help update the grafana dashboard? I added a make target for the devbox to install the grafana stack, so you can easily to test it in the devbox.

Hi @pingsutw , I just submitted a PR addressing this: #7836. Please take a look. Thank you!