add allocation binpacking duration metric #687
Conversation
|
✔️ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: e7213e5 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/6144fff406064000089faa32 |
| return packings | ||
| } | ||
|
|
||
| func (p *packer) pack(ctx context.Context, schedule *scheduling.Schedule, instances []cloudprovider.InstanceType) []*Packing { |
There was a problem hiding this comment.
To remove the need for a private function that just calls the public, you could place this at the top of original and still make minimal code additions to the core packing logic.
Pack:
startTime := time.Now()
defer packTimeHistogram.Observe(time.Since(startTime).Seconds())
The defer is advantageous on the Observe call anyways since you'll get observed durations even if a panic or some runtime errors wreak havoc.
There was a problem hiding this comment.
To remove the need for a private function that just calls the public [...]
It's the other way around: the public method calls the private method. The wrapper allows separating the metric logic from the binpacking logic. Is there an advantage to breaking the separation of concerns?
There was a problem hiding this comment.
yeah, sorry mistyped: Public calling Private.
Since the metric logic is tiny (two lines), seems a bit heavy to do the function proxying. The metric logic can be placed at the top with the defer to keep it together to separate it a bit for the reader. The reason to keep the private without metrics is if we have a use-case (maybe in testing) to not emit metrics and use just the pack func. But that doesn't seem like a great thing to do anyways. I would guess it would be configurable elsewhere to make the Observe func a noop, if we don't want metrics.
| @@ -63,6 +81,11 @@ type Packing struct { | |||
| // It follows the First Fit Decreasing bin packing technique, reference- | |||
| // https://en.wikipedia.org/wiki/Bin_packing_problem#First_Fit_Decreasing_(FFD) | |||
| func (p *packer) Pack(ctx context.Context, schedule *scheduling.Schedule, instances []cloudprovider.InstanceType) []*Packing { | |||
| startTime := time.Now() | |||
| defer func() { | |||
| packTimeHistogram.Observe(time.Since(startTime).Seconds()) | |||
| @@ -63,6 +81,11 @@ type Packing struct { | |||
| // It follows the First Fit Decreasing bin packing technique, reference- | |||
| // https://en.wikipedia.org/wiki/Bin_packing_problem#First_Fit_Decreasing_(FFD) | |||
| func (p *packer) Pack(ctx context.Context, schedule *scheduling.Schedule, instances []cloudprovider.InstanceType) []*Packing { | |||
| startTime := time.Now() | |||
| defer func() { | |||
There was a problem hiding this comment.
is there a reason to wrap this in a func rather than just doing:
startTime := time.Now()
defer packTimeHistogram.Observe(time.Since(startTime).Seconds())
There was a problem hiding this comment.
According to Defer, Panic, and Recover from the Go blog:
A deferred function’s arguments are evaluated when the defer statement is evaluated.
So the proposed code will not record the elapsed time because only the .Observe() call is defered; time.Since(startTime).Seconds() will be evaluated immediately.
1. Issue, if available:
#612 "Emit Metrics for Karpenter"
This PR does not fully resolve the issue. More changes will be needed.
2. Description of changes:
Add a histogram metric to record the duration of binpacking operations.
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.