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
16 changes: 13 additions & 3 deletions cmd/ateapi/internal/controlapi/create_actor.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ import (
"context"
"errors"
"fmt"
"time"

"github.com/agent-substrate/substrate/cmd/ateapi/internal/store"
"github.com/agent-substrate/substrate/internal/ateattr"
"github.com/agent-substrate/substrate/internal/resources"
"github.com/agent-substrate/substrate/pkg/proto/ateapipb"
"google.golang.org/grpc/codes"
Expand All @@ -29,11 +31,19 @@ import (
"k8s.io/apimachinery/pkg/util/validation/field"
)

func (s *Service) CreateActor(ctx context.Context, req *ateapipb.CreateActorRequest) (*ateapipb.Actor, error) {
if err := validateCreateActorRequest(req); err != nil {
func (s *Service) CreateActor(ctx context.Context, req *ateapipb.CreateActorRequest) (created *ateapipb.Actor, err error) {

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.

why are we swapping everything to be named return args, that's not a very common pattern. If you're trying to capture err can you just do var err error. I don't feel super duper strongly about this, but I personally find it a bit harder to read.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's intentional, because with a var err error the defer would capture the pre-mapping error and also misqualify error.type. Would you say a comment would make this clearer?

start := time.Now()
in := req.GetActor()
defer func() {
s.instruments.recordLifecycleOp(ctx, ateattr.OperationCreate, start, err,
ateattr.TemplateNameKey.String(in.GetActorTemplateName()),
ateattr.TemplateNamespaceKey.String(in.GetActorTemplateNamespace()),
)
}()

if err = validateCreateActorRequest(req); err != nil {
return nil, err
}
in := req.GetActor()
templateNamespace := in.GetActorTemplateNamespace()
templateName := in.GetActorTemplateName()

Expand Down
23 changes: 16 additions & 7 deletions cmd/ateapi/internal/controlapi/delete_actor.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,26 @@ import (
"context"
"errors"
"fmt"
"time"

"github.com/agent-substrate/substrate/cmd/ateapi/internal/store"
"github.com/agent-substrate/substrate/internal/ateattr"
"github.com/agent-substrate/substrate/internal/resources"
"github.com/agent-substrate/substrate/pkg/proto/ateapipb"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"k8s.io/apimachinery/pkg/util/validation/field"
)

func (s *Service) DeleteActor(ctx context.Context, req *ateapipb.DeleteActorRequest) (*ateapipb.Actor, error) {
if err := validateDeleteActorRequest(req); err != nil {
func (s *Service) DeleteActor(ctx context.Context, req *ateapipb.DeleteActorRequest) (deleted *ateapipb.Actor, err error) {
start := time.Now()
// No template dims here: a delete request names only the actor, and the
// template is unknown on the paths that return before the record loads.
defer func() {
s.instruments.recordLifecycleOp(ctx, ateattr.OperationDelete, start, err)
}()

if err = validateDeleteActorRequest(req); err != nil {
return nil, err
}
setSpanActorRefAttributes(ctx, req.GetActor().GetAtespace(), req.GetActor().GetName())
Expand All @@ -49,17 +58,17 @@ func (s *Service) DeleteActor(ctx context.Context, req *ateapipb.DeleteActorRequ
return nil, status.Errorf(codes.Internal, "while deleting actor volumes: %v", err)
}

deleted, err := s.persistence.DeleteActor(ctx, atespace, name)
deleted, err = s.persistence.DeleteActor(ctx, atespace, name)
if err != nil {
if errors.Is(err, store.ErrNotFound) {
return nil, status.Errorf(codes.NotFound, "Actor %s not found", req.GetActor().GetName())
return nil, status.Errorf(codes.NotFound, "Actor %s not found", name)
}
if errors.Is(err, store.ErrFailedPrecondition) {
current, getErr := s.persistence.GetActor(ctx, req.GetActor().GetAtespace(), req.GetActor().GetName())
current, getErr := s.persistence.GetActor(ctx, atespace, name)
if getErr == nil {
return nil, status.Errorf(codes.FailedPrecondition, "Actor %s is not suspended (status: %v)", req.GetActor().GetName(), current.GetStatus())
return nil, status.Errorf(codes.FailedPrecondition, "Actor %s is not suspended (status: %v)", name, current.GetStatus())
}
return nil, status.Errorf(codes.FailedPrecondition, "Actor %s is not suspended", req.GetActor().GetName())
return nil, status.Errorf(codes.FailedPrecondition, "Actor %s is not suspended", name)
}
if errors.Is(err, store.ErrPersistenceRetry) {
return nil, status.Error(codes.Aborted, "concurrent update conflict, please retry")
Expand Down
9 changes: 8 additions & 1 deletion cmd/ateapi/internal/controlapi/functional_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/redis/go-redis/v9"
sdkmetric "go.opentelemetry.io/otel/sdk/metric"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/credentials"
Expand Down Expand Up @@ -320,7 +321,13 @@ func setupTest(t *testing.T, ns string) *testContext {
return insecure.NewCredentials(), nil
}

service := NewService(persistence, wc, actorTemplateLister, workerPoolLister, sandboxConfigLister, dialer, k8sClient)
instruments, err := NewInstruments(sdkmetric.NewMeterProvider(sdkmetric.WithReader(sdkmetric.NewManualReader())).Meter("ateapi"))
if err != nil {
cancel()
mr.Close()
t.Fatalf("failed to create metric instruments: %v", err)
}
service := NewService(persistence, wc, actorTemplateLister, workerPoolLister, sandboxConfigLister, dialer, k8sClient, instruments)

// 5. Start REAL gRPC Server for ATE API
grpcServer := grpc.NewServer(grpc.UnaryInterceptor(ateinterceptors.ServerUnaryInterceptor))
Expand Down
104 changes: 103 additions & 1 deletion cmd/ateapi/internal/controlapi/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,22 @@ package controlapi
import (
"context"
"fmt"
"time"

"github.com/agent-substrate/substrate/internal/ateattr"
atev1alpha1 "github.com/agent-substrate/substrate/pkg/api/v1alpha1"
"github.com/agent-substrate/substrate/pkg/proto/ateapipb"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/metric"
"google.golang.org/grpc/status"
"k8s.io/apimachinery/pkg/labels"
)

const workerpoolWorkersMetric = "ate.workerpool.workers"
const (
workerpoolWorkersMetric = "ate.workerpool.workers"
lifecycleOpDurationMetric = "ate.actor.lifecycle.operation.duration"
schedulerAssignmentMetric = "ate.scheduler.assignment.duration"
)

// RegisterWorkerCount wires the ate.workerpool.workers observable against workers
// (workercache.Cache.Workers in prod) and listPools (a WorkerPool lister's List,
Expand Down Expand Up @@ -84,3 +91,98 @@ func RegisterWorkerCount(meter metric.Meter, workers func() ([]*ateapipb.Worker,
}
return nil
}

// Instruments holds ateapi's actor-lifecycle and scheduler duration histograms.
// A nil *Instruments is a valid no-op, so call sites need no guard. Worker-count
// is registered separately (RegisterWorkerCount): a callback-driven observable,
// not a synchronous instrument.
type Instruments struct {

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.

Is there any reason this is public, seems like we could construct it locally rather than exporting it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's exported because main.go builds it and passes it to NewService like e.g. RegisterWorkerCount, so this seemed like the consistent way.

lifecycleOpDuration metric.Float64Histogram
schedulerAssignmentDuration metric.Float64Histogram
}

// NewInstruments builds the two histograms against meter. Assignment buckets are
// an order of magnitude finer than lifecycle's: it is a sub-second in-memory pick,
// not a multi-second restore.
func NewInstruments(meter metric.Meter) (*Instruments, error) {
lifecycleOpDuration, err := meter.Float64Histogram(
lifecycleOpDurationMetric,
metric.WithUnit("s"),
metric.WithDescription("Duration of an actor lifecycle operation (create, resume, suspend, pause, delete) handled by ateapi."),
metric.WithExplicitBucketBoundaries(0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.15, 0.25, 0.5, 1, 2.5, 5, 10, 30),
)
if err != nil {
return nil, fmt.Errorf("create %s histogram: %w", lifecycleOpDurationMetric, err)
}

schedulerAssignmentDuration, err := meter.Float64Histogram(
schedulerAssignmentMetric,
metric.WithUnit("s"),
metric.WithDescription("Duration of the scheduler's worker-assignment step during actor resume."),
metric.WithExplicitBucketBoundaries(0.0005, 0.001, 0.0025, 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1),
)
if err != nil {
return nil, fmt.Errorf("create %s histogram: %w", schedulerAssignmentMetric, err)
}

return &Instruments{
lifecycleOpDuration: lifecycleOpDuration,
schedulerAssignmentDuration: schedulerAssignmentDuration,
}, nil
}

// recordLifecycleOp records op's duration. A non-nil err is classified onto
// error.type via its gRPC status code; error.type's absence marks success, so
// there is no parallel failure counter. extraAttrs carries the per-operation
// dimensions (template, pool, class, snapshot kind).
func (i *Instruments) recordLifecycleOp(ctx context.Context, op string, start time.Time, err error, extraAttrs ...attribute.KeyValue) {
if i == nil || i.lifecycleOpDuration == nil {
return
}
attrs := make([]attribute.KeyValue, 0, len(extraAttrs)+2)
attrs = append(attrs, ateattr.ActorOperationNameKey.String(op))
attrs = append(attrs, extraAttrs...)
if err != nil {
attrs = append(attrs, ateattr.ErrorTypeKey.String(status.Code(err).String()))
}
i.lifecycleOpDuration.Record(ctx, time.Since(start).Seconds(), metric.WithAttributes(attrs...))
}

// lifecycleOpAttrs builds the resume/suspend/pause dimensions from workflow
// state. Nil-safe, and omits the pool and snapshot-kind labels until they are
// known so a failure before the assign/restore steps never emits an empty-string
// series. snapshotKind is empty for suspend/pause, which do not restore.
func lifecycleOpAttrs(actor *ateapipb.Actor, template *atev1alpha1.ActorTemplate, snapshotKind string) []attribute.KeyValue {
attrs := []attribute.KeyValue{
ateattr.TemplateNameKey.String(actor.GetActorTemplateName()),
ateattr.TemplateNamespaceKey.String(actor.GetActorTemplateNamespace()),
}
if pool := actor.GetWorkerPoolName(); pool != "" {
attrs = append(attrs, ateattr.WorkerPoolNameKey.String(pool))
}
if template != nil {
attrs = append(attrs, ateattr.SandboxClassKey.String(string(template.Spec.SandboxClass)))
}
if snapshotKind != "" {
attrs = append(attrs, ateattr.SnapshotKindKey.String(snapshotKind))
}
return attrs
}

// recordSchedulerAssignment records one assignment attempt. pool is stamped only
// when a worker was assigned and error.type only for the Error outcome, so
// no_free_worker (a capacity signal, not a failure) carries neither.
func (i *Instruments) recordSchedulerAssignment(ctx context.Context, start time.Time, outcome, pool string, err error) {
if i == nil || i.schedulerAssignmentDuration == nil {
return
}
attrs := make([]attribute.KeyValue, 0, 3)
attrs = append(attrs, ateattr.SchedulerOutcomeKey.String(outcome))
if pool != "" {
attrs = append(attrs, ateattr.WorkerPoolNameKey.String(pool))
}
if outcome == ateattr.SchedulerOutcomeError && err != nil {
attrs = append(attrs, ateattr.ErrorTypeKey.String(status.Code(err).String()))
}
i.schedulerAssignmentDuration.Record(ctx, time.Since(start).Seconds(), metric.WithAttributes(attrs...))
}
Loading