fix CloudProvider metric #1031
fix CloudProvider metric #1031
Conversation
|
✔️ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: d62d1ee 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/61c0f3cc80c6760008f2a987 |
| @@ -67,9 +67,10 @@ func Decorate(cloudProvider cloudprovider.CloudProvider) cloudprovider.CloudProv | |||
| } | |||
|
|
|||
| func (d *decorator) Create(ctx context.Context, constraints *v1alpha5.Constraints, instanceTypes []cloudprovider.InstanceType, quantity int, callback func(*v1.Node) error) <-chan error { | |||
| recordLatency := metrics.Measure(methodDurationHistogramVec.WithLabelValues(getControllerName(ctx), "Create", d.Name())) | |||
There was a problem hiding this comment.
The cloud provider was originally written to be async (i.e. return a chan) in order to support batching on the cloud provider side. Instead, batching was built into the arguments (i.e. quantity).
I don't think it would be crazy to change the cloud provider to return an error instead of a chan(error), which would simplify the complexity in the metrics implementation.
There was a problem hiding this comment.
I have a noob question. Does removing the async design prohibit us from performing time consuming callbacks when a node is launched? For example, can i still wait until the node is ready in the callback function?
1. Issue, if available:
None
2. Description of changes:
Previously, the latency recorded for the
CloudProvider.Create()method may have missed some latency. Now the latency "start time" is set correctly.3. Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.