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.
There was a problem hiding this comment.
Dropped this one.
| // 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.
72daaf6 to
00fb7ca
Compare
26c376a to
cd0945a
Compare
|
cc. Da Huang (@git286), Julian Gutierrez Oschmann (@juli4n) for review. This has been up for a good while now, can you please review when you have a chance? |
cd0945a to
1c136ae
Compare
3 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 kindrpc.server.call.durationmetric covers this, but the meaningful dimensions are missing, so it's not really actionable. Extending its labels with extra, domain-specific labels is an OTel anti-pattern, hence the new metric.ate.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).