feat(otel): add actor lifecycle + scheduler duration metrics#514
Conversation
|
|
||
| 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| s.instruments.recordLifecycleOp(ctx, ateattr.OperationDelete, start, err, | ||
| ateattr.TemplateNameKey.String(tmpl.GetActorTemplateName()), | ||
| ateattr.TemplateNamespaceKey.String(tmpl.GetActorTemplateNamespace()), | ||
| ) |
There was a problem hiding this comment.
i think the behavior of this might be confusing. If this method returns before tmpl is assigned it may create a record with empty name, namespace. I think it's better to just use the incoming values even if they don't exist, what do you think?
There was a problem hiding this comment.
Fair, although DeleteActorRequest is an ObjectRef so there's no template. I'll just drop template labels from delete entirely.
| // 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 { |
There was a problem hiding this comment.
Is there any reason this is public, seems like we could construct it locally rather than exporting it
There was a problem hiding this comment.
It's exported because main.go builds it and passes it to NewService like e.g. RegisterWorkerCount, so this seemed like the consistent way.
2 participants
This PR is the second slice of the platform-metrics split (#433).
It adds two duration histograms emitted by ateapi:
ate.actor.lifecycle.operation.duration: create/resume/suspend/pause/delete, labeled by operation, template, pool, sandbox class, and (on resume) snapshot kindate.scheduler.assignment.duration: worker-assignment step, labeled by outcome (assigned / no_free_worker / error) and poolTested e2e on a local kind cluste where: both metrics reach the otel-system collector with the expected labels (resume shows
snapshot_kind=golden, scheduler showsoutcome=assigned, noerror.typeon success).