atenet: preserve cold resume outcome and fallback empty template dimension - #1482
Conversation
…nsions (agent-substrate#1474) Fixes agent-substrate#1474 When atenet-router routes requests: - Mislabeled cold resume failures: Previously, failed resumes were unconditionally overwritten with ate.router.resume="none", distorting cold activation rates and durations. In-flight cold activation attempts (and joiners) now preserve "triggered" and "joined" respectively on capacity and transient errors. Definitive non-activation errors (e.g. NotFound, InvalidArgument), context cancellations, and non-resuming directions (egress) now report "unattempted", strictly reserving "none" for warm hits on running actors. - Missing template labels: Empty template namespace and name dimensions are normalized to "unknown" using ateattr.NormalizeTemplateDimension, satisfying metric registry invariants without using request-derived labels on failure paths. - Documentation: Updated registry metrics.yaml and substrate.yaml to reflect the "unattempted" resume value, mirror handler Result documentation for "unknown" template fallback (including egress), and account for platform-injected unknown dimensions.
1cbcde7 to
9546f66
Compare
|
|
||
| // NormalizeTemplateDimension ensures a template dimension (atespace or name) is | ||
| // non-empty, falling back to TemplateUnknown if unset. | ||
| func NormalizeTemplateDimension(dim string) string { |
There was a problem hiding this comment.
Nice, but it's still only called from the router though, and the group brief now promises unknown for all 7 metrics that ref these attrs.
There was a problem hiding this comment.
Thanks for catching it. This change would touch 7 metrics. I would prefer to do it in a follow-up PR for a cleaner chagne.
| // isDefinitiveResumeError reports whether err represents a failure where no cold | ||
| // activation could be attempted (e.g. the actor does not exist, bad request, or | ||
| // permission denied), as opposed to in-flight capacity or transient failures. | ||
| func isDefinitiveResumeError(err error) bool { | ||
| switch status.Code(err) { | ||
| case codes.NotFound, codes.InvalidArgument, codes.PermissionDenied, codes.Unauthenticated: | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we flip the default? Right now Internal, Unimplemented, Unknown etc. fall through to triggered, so an unrecognized code is as an activation in the latency series.
Unknown means we don't know, so unattempted seems safer.
Also wondering about FailedPrecondition, its registry brief is "the state of the actor did not permit a route" and with parking off it fails immediately, which sounds definitive. And retryable's comment just above already groups DeadlineExceeded with NotFound and PermissionDenied, so it's a bit weird to have two classifiers for the same codes I think.
Wdyt?
There was a problem hiding this comment.
Agreed on it. But I want to discuss more on the flipping bit.
If I flip it, I still need a list of codes that mean "an activation was in flight". This is not quite maintainable by looking at the code now.
My proposed change will be:
- Delete
isDefinitiveResumeError. This removes the classifier. - Make err == nil a condition for triggered and joined. A resume that fails reports the fourth value (added in this PR).
FailedPreconditionandResourceExhaustedboth go there. It also corrects the words "initiated cold activation" that you found. - Change the name
unattemptedtounknown. The value includes requests where an activation did operate. One example is a canceled leader. Its flight is separate from the request context, and it continues to restore the actor. A second example isDeadlineExceededduring a restore. Thusunattemptedis not true for these requests, and new words in the brief cannot correct this.
WDYT?
| // RouterResumeTriggered indicates this request won the singleflight lock and initiated cold activation. | ||
| RouterResumeTriggered = "triggered" |
There was a problem hiding this comment.
initiated cold activation
a request that is ResourceExhausted gets triggered without activating anything. Same wording in the registry brief.
| No cold activation was in flight. The request was invalid, the | ||
| actor did not exist, the direction does not resume, or the | ||
| request canceled before activation was attempted. |
There was a problem hiding this comment.
"No cold activation was in flight" and "request canceled" contradict each other, a canceled joiner was waiting on someone else's activation. Maybe "this request neither attempted nor observed an activation" is more accurate?
2 participants
Fixes #1474
When atenet-router routes requests: