Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions cmd/atenet/internal/router/extproc/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ type Result struct {
TemplateName string

// Resume is the actor-resume outcome, as one of the ateattr.RouterResume*
// values. Empty means "none" — the direction never resumes an actor, or the
// request never got that far.
// values. Empty means "unattempted" — the direction never resumes an actor,
// or the request never got that far.
Resume string

// DynamicMetadata is attached to the ProcessingResponse alongside Response,
Expand All @@ -72,10 +72,10 @@ type Result struct {
}

// resume returns the resume label for the route-duration metric, defaulting an
// unset outcome to "none".
// unset outcome to "unattempted".
func (r Result) resume() string {
if r.Resume == "" {
return ateattr.RouterResumeNone
return ateattr.RouterResumeUnattempted
}
return r.Resume
}
2 changes: 2 additions & 0 deletions cmd/atenet/internal/router/extproc/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ func (s *Server) recordRouteDuration(ctx context.Context, d time.Duration, tmplN
if s.routeDuration == nil {
return
}
tmplNs = ateattr.NormalizeTemplateDimension(tmplNs)
tmplName = ateattr.NormalizeTemplateDimension(tmplName)
s.routeDuration.Record(ctx, d.Seconds(), metric.WithAttributes(
ateattr.TemplateAtespaceKey.String(tmplNs),
ateattr.TemplateNameKey.String(tmplName),
Expand Down
56 changes: 56 additions & 0 deletions cmd/atenet/internal/router/extproc/metrics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,3 +140,59 @@ func TestRecordRouteDuration_Attributes(t *testing.T) {
}
}
}

func TestRecordRouteDuration_NormalizesEmptyTemplateDimensions(t *testing.T) {
reader := sdkmetric.NewManualReader()
mp := sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader))
h, err := mp.Meter("atenet-router").Float64Histogram(routeDurationMetricName)
if err != nil {
t.Fatalf("failed to create histogram: %v", err)
}

s := NewServer(50051, h, nil)
s.recordRouteDuration(context.Background(), 5*time.Millisecond, "", "", classifyOutcome(errors.New("fail")), ateattr.RouterResumeUnattempted)

var rm metricdata.ResourceMetrics
if err := reader.Collect(context.Background(), &rm); err != nil {
t.Fatalf("Collect failed: %v", err)
}

dp := rm.ScopeMetrics[0].Metrics[0].Data.(metricdata.Histogram[float64]).DataPoints[0]
wantAttrs := map[string]string{
"ate.template.atespace": "unknown",
"ate.template.name": "unknown",
"ate.router.outcome": "resume_error",
"ate.router.resume": "unattempted",
}

for k, want := range wantAttrs {
val, exists := dp.Attributes.Value(attribute.Key(k))
if !exists {
t.Errorf("missing metric attribute %q", k)
} else if val.AsString() != want {
t.Errorf("attribute %q = %q, want %q", k, val.AsString(), want)
}
}
}

func TestResultResume_DefaultsToUnattempted(t *testing.T) {
tests := []struct {
name string
resume string
want string
}{
{name: "empty defaults to unattempted", resume: "", want: ateattr.RouterResumeUnattempted},
{name: "none preserved", resume: ateattr.RouterResumeNone, want: ateattr.RouterResumeNone},
{name: "triggered preserved", resume: ateattr.RouterResumeTriggered, want: ateattr.RouterResumeTriggered},
{name: "joined preserved", resume: ateattr.RouterResumeJoined, want: ateattr.RouterResumeJoined},
{name: "unattempted preserved", resume: ateattr.RouterResumeUnattempted, want: ateattr.RouterResumeUnattempted},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
r := Result{Resume: tt.resume}
if got := r.resume(); got != tt.want {
t.Errorf("Result.resume() = %q, want %q", got, tt.want)
}
})
}
}
52 changes: 36 additions & 16 deletions cmd/atenet/internal/router/ingress/resumer.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,10 @@ func (e *budgetExhaustedError) Unwrap() error { return e.lastErr }
type ResumeOutcome string

const (
ResumeOutcomeNone ResumeOutcome = ateattr.RouterResumeNone
ResumeOutcomeTriggered ResumeOutcome = ateattr.RouterResumeTriggered
ResumeOutcomeJoined ResumeOutcome = ateattr.RouterResumeJoined
ResumeOutcomeNone ResumeOutcome = ateattr.RouterResumeNone
ResumeOutcomeTriggered ResumeOutcome = ateattr.RouterResumeTriggered
ResumeOutcomeJoined ResumeOutcome = ateattr.RouterResumeJoined
ResumeOutcomeUnattempted ResumeOutcome = ateattr.RouterResumeUnattempted
)

type resumeCallResult struct {
Expand Down Expand Up @@ -160,6 +161,18 @@ func (r *ActorResumer) retryable(err error) bool {
}
}

// 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
}
}
Comment on lines +164 to +174

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Delete isDefinitiveResumeError. This removes the classifier.
  2. Make err == nil a condition for triggered and joined. A resume that fails reports the fourth value (added in this PR). FailedPrecondition and ResourceExhausted both go there. It also corrects the words "initiated cold activation" that you found.
  3. Change the name unattempted to unknown. 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 is DeadlineExceeded during a restore. Thus unattempted is not true for these requests, and new words in the brief cannot correct this.

WDYT?


// ResumeActor ensures the requested actor is running. It deduplicates concurrent
// requests within the process and, when parking is enabled, holds the request
// while retrying transient failures until the budget elapses.
Expand Down Expand Up @@ -244,34 +257,41 @@ func (r *ActorResumer) ResumeActor(ctx context.Context, actorRef resources.Actor
select {
case <-ctx.Done():
// The caller's request context was canceled before the singleflight resume completed.
// Return early with ResumeOutcomeNone ("none")
return nil, ResumeOutcomeNone, ctx.Err()
// Return early with ResumeOutcomeUnattempted ("unattempted")
return nil, ResumeOutcomeUnattempted, ctx.Err()
case res := <-ch:
callRes, _ := res.Val.(*resumeCallResult)
if callRes == nil {
if res.Err != nil {
return nil, ResumeOutcomeNone, res.Err
return nil, ResumeOutcomeUnattempted, res.Err
}
return nil, ResumeOutcomeNone, status.Error(codes.Internal, "resume call returned nil result")
}

// On error, return ResumeOutcomeNone ("none") so the failure is tagged
// under the 'outcome' label rather than misreported as an activation.
if callRes.err != nil {
return nil, ResumeOutcomeNone, callRes.err
return nil, ResumeOutcomeUnattempted, status.Error(codes.Internal, "resume call returned nil result")
}

// Disambiguate singleflight resume outcome:
// - ResumeOutcomeNone ("none"): resumed == false, actor was already active/running.
// - ResumeOutcomeTriggered ("triggered"): Cold activation leader (resumed == true, caller's reqID == leaderID).
// - ResumeOutcomeJoined ("joined"): Cold activation joiner (resumed == true, caller's reqID != leaderID).
// - ResumeOutcomeNone ("none"): resumed == false and err == nil, actor was already active/running (warm route).
// - ResumeOutcomeTriggered ("triggered"): Cold activation leader (resumed == true or in-flight activation errored, caller's reqID == leaderID).
// - ResumeOutcomeJoined ("joined"): Cold activation joiner (resumed == true or in-flight activation errored, caller's reqID != leaderID).
// - ResumeOutcomeUnattempted ("unattempted"): Definitive non-activation error (e.g. NotFound, InvalidArgument) where no activation was attempted.
outcome := ResumeOutcomeNone
if callRes.resumed {
if callRes.leaderID == reqID {
outcome = ResumeOutcomeTriggered
} else {
outcome = ResumeOutcomeJoined
}
} else if callRes.err != nil {
if isDefinitiveResumeError(callRes.err) {
outcome = ResumeOutcomeUnattempted
} else if callRes.leaderID == reqID {
outcome = ResumeOutcomeTriggered
} else {
outcome = ResumeOutcomeJoined
}
}

if callRes.err != nil {
return nil, outcome, callRes.err
}

return callRes.actor, outcome, nil
Expand Down
84 changes: 82 additions & 2 deletions cmd/atenet/internal/router/ingress/resumer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"
"errors"
"sync"
"sync/atomic"
"testing"
"testing/synctest"
"time"
Expand Down Expand Up @@ -146,8 +147,87 @@ func TestActorResumer_ResumeActor(t *testing.T) {
if got := status.Code(err); got != codes.NotFound {
t.Errorf("expected gRPC code NotFound, got %v (err=%v)", got, err)
}
if outcome != ResumeOutcomeNone {
t.Errorf("expected outcome %q on error, got %q", ResumeOutcomeNone, outcome)
if outcome != ResumeOutcomeUnattempted {
t.Errorf("expected outcome %q on definitive error for leader attempt, got %q", ResumeOutcomeUnattempted, outcome)
}
})

t.Run("ContextCanceled", func(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
cancel()

mock := &resumerMockClient{
resumeFn: func(ctx context.Context, in *ateapipb.ResumeActorRequest, opts ...grpc.CallOption) (*ateapipb.ResumeActorResponse, error) {
return &ateapipb.ResumeActorResponse{Resumed: true}, nil
},
}

resumer := NewActorResumer(mock)
_, outcome, err := resumer.ResumeActor(ctx, testActorRef)
if !errors.Is(err, context.Canceled) {
t.Errorf("expected context.Canceled, got %v", err)
}
if outcome != ResumeOutcomeUnattempted {
t.Errorf("expected outcome %q on context cancellation, got %q", ResumeOutcomeUnattempted, outcome)
}
})

t.Run("SingleflightDeduplication_ErrorDisambiguation", func(t *testing.T) {
var resumeCalled int
var mu sync.Mutex
const concurrentRequests = 10
var callersStarted atomic.Int32

mock := &resumerMockClient{
resumeFn: func(ctx context.Context, in *ateapipb.ResumeActorRequest, opts ...grpc.CallOption) (*ateapipb.ResumeActorResponse, error) {
mu.Lock()
resumeCalled++
mu.Unlock()
// Barrier: wait until all concurrent goroutines have launched ResumeActor
for callersStarted.Load() < concurrentRequests {
time.Sleep(time.Millisecond)
}
time.Sleep(10 * time.Millisecond)
return nil, status.Error(codes.ResourceExhausted, "no free workers available")
},
}

resumer := NewActorResumer(mock)

var wg sync.WaitGroup
outcomes := make([]ResumeOutcome, concurrentRequests)
errs := make([]error, concurrentRequests)

wg.Add(concurrentRequests)
for i := 0; i < concurrentRequests; i++ {
go func(idx int) {
defer wg.Done()
callersStarted.Add(1)
_, outcomes[idx], errs[idx] = resumer.ResumeActor(context.Background(), testActorRef)
}(i)
}
wg.Wait()

var triggeredCount, joinedCount int
for i := 0; i < concurrentRequests; i++ {
if got := status.Code(errs[i]); got != codes.ResourceExhausted {
t.Fatalf("request %d expected ResourceExhausted, got %v", i, errs[i])
}
switch outcomes[i] {
case ResumeOutcomeTriggered:
triggeredCount++
case ResumeOutcomeJoined:
joinedCount++
default:
t.Errorf("unexpected outcome for request %d: %q", i, outcomes[i])
}
}

if triggeredCount != 1 {
t.Errorf("expected exactly 1 request to have outcome 'triggered', got %d", triggeredCount)
}
if joinedCount != concurrentRequests-1 {
t.Errorf("expected %d requests to have outcome 'joined', got %d", concurrentRequests-1, joinedCount)
}
})

Expand Down
24 changes: 18 additions & 6 deletions docs/metrics/registry/metrics.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -69,19 +69,24 @@ groups:
type: attribute_group
brief: >
The identity of an ActorTemplate. The values are not limited to a list.
But an operator makes each template. No value comes from a request. Thus
the number of values stays small.
An operator makes each template (or "unknown" when the direction has no
template or the request failed before resolving one). No value comes from
a request. Thus the number of values stays small.
attributes:
- id: ate.template.atespace
stability: development
type: string
brief: The atespace of the ActorTemplate of the actor.
examples: [ate-demo-counter]
brief: >
The atespace of the ActorTemplate of the actor, or "unknown" when the
direction has no template (or the request failed before resolving one).
examples: [ate-demo-counter, unknown]
- id: ate.template.name
stability: development
type: string
brief: The name of the ActorTemplate of the actor.
examples: [counter]
brief: >
The name of the ActorTemplate of the actor, or "unknown" when the
direction has no template (or the request failed before resolving one).
examples: [counter, unknown]

- id: registry.ate.workerpool
type: attribute_group
Expand Down Expand Up @@ -422,6 +427,13 @@ groups:
A resume was already in operation. This request waited for it.
This value is separate because it is not a correct sample. One
cold start with 50 requests is one slow activation and not 51.
- id: unattempted
stability: development
value: unattempted
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.
Comment on lines +434 to +436

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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?

- id: ate.router.outcome
stability: development
brief: The result of the route attempt.
Expand Down
3 changes: 2 additions & 1 deletion docs/metrics/substrate.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,8 @@ cardinality_rules:
- id: bounded-or-catalog-scoped
brief: >
Each ate.* metric label has a list of permitted values, or it names an
object that an operator made. The names of the templates and the pools
object that an operator made (or "unknown" when a template cannot be
resolved or is not applicable). The names of the templates and the pools
never come from a request.
enforced: false
could_be_enforced_by: >
Expand Down
18 changes: 18 additions & 0 deletions internal/ateattr/ateattr.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,21 @@ const (
ActorVersionKey = attribute.Key("ate.actor.version")
)

// TemplateUnknown is the fallback for unresolvable or missing template dimensions.
// Note: "unknown" is syntactically a legal atespace/template name; like
// ate.sandbox.class="unknown", this trades potential collision with a real
// object named "unknown" for maintaining a bounded, non-empty metric dimension.
const TemplateUnknown = "unknown"
Comment thread
JeffLuoo marked this conversation as resolved.

// NormalizeTemplateDimension ensures a template dimension (atespace or name) is
// non-empty, falling back to TemplateUnknown if unset.
func NormalizeTemplateDimension(dim string) string {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if dim == "" {
return TemplateUnknown
}
return dim
}

// ReservedNamespace is substrate's. A producer that merges untrusted fields into a
// record drops everything under it, so nothing a workload sets can read as
// platform-issued attribution downstream.
Expand Down Expand Up @@ -197,6 +212,9 @@ const (
RouterResumeTriggered = "triggered"
// RouterResumeJoined indicates this request parked on an in-flight singleflight resume.
RouterResumeJoined = "joined"
// RouterResumeUnattempted indicates no cold activation was in flight (e.g.
// definitive errors before activation, request cancellation, or non-resuming route).
RouterResumeUnattempted = "unattempted"
)

// Values for ImageCacheOutcomeKey. A hit is a complete image record; a miss
Expand Down
Loading
Loading