Refactoring logging and metrics #835

felix-zhe-huang · 2021-11-22T16:48:44Z

1. Issue, if available:
Logging and Metrics improvements #811
Closes #811

2. Description of changes:

Change controller names to lower cases
Reword log line to include reconcile resource name in the inline section
Add reconcile resource name (provisioner) as label in the prometheus measurement

3. Does this change impact docs?

Yes, PR includes docs updates
Yes, issue opened: link to issue
No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

netlify · 2021-11-22T16:48:50Z

✔️ Deploy Preview for karpenter-docs-prod canceled.

🔨 Explore the source changes: fd52ec2

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/619d1bddcd68c100070f0042

ellistarn · 2021-11-22T17:29:49Z

pkg/controllers/metrics/controller.go

@@ -75,7 +75,7 @@ func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (reco
 func (c *Controller) Register(_ context.Context, m manager.Manager) error {
 	return controllerruntime.
 		NewControllerManagedBy(m).
-		Named(controllerName).
+		Named(strings.ToLower(controllerName)).


consider just making the variable lower, rather than lowering it in both places.

ellistarn · 2021-11-22T17:30:48Z

pkg/controllers/node/controller.go

@@ -59,7 +59,7 @@ type Controller struct {

 // Reconcile executes a reallocation control loop for the resource
 func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) {
-	ctx = logging.WithLogger(ctx, logging.FromContext(ctx).Named("Node"))
+	ctx = logging.WithLogger(ctx, logging.FromContext(ctx).Named("node").With("node", req.String()))


With this change, I think we can change lines like 105 from

fmt.Errorf("patching node %s, %w", node.Name, err)

to

fmt.Errorf("patching node, %w", err)

ellistarn · 2021-11-22T17:36:46Z

pkg/controllers/provisioning/binpacking/packer.go

@@ -74,7 +75,7 @@ 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 {
-	defer metrics.Measure(packDuration)()
+	defer metrics.Measure(packDuration.WithLabelValues(ctx.Value("provisioner").(string)))()


consider making a helper function to avoid casting inline. Maybe something like:
inject.GetResource(ctx)
injection.WithResource(ctx)
There are some other helpers that you could move into this pattern like
injection.GetRestConfig(ctx) and injection.GetOptions

Check out the pattern defined by knative in /cmd/webhook/main.go injection.WithNamespaceScope.

Good idea. I created one under utility. There are a few similar helper functions under utility already. I learned from that pattern.

ellistarn · 2021-11-22T22:22:11Z

pkg/utils/reconcilename/reconcilename.go

+limitations under the License.
+*/
+
+package reconcilename


Can we merge all of these into a single injection package? I really like the knative approach I linked. I think it'll cause this to read really nicely.

injection.WithFoo(ctx, foo) injection.GetFoo(ctx)

ellistarn · 2021-11-22T22:25:40Z

pkg/utils/reconcilename/reconcilename.go

+type contextKey string
+
+func Inject(ctx context.Context, contextkey string, resourcename string) context.Context {
+	return context.WithValue(ctx, contextKey(contextkey), resourcename)


Ideally your callers wouldn't need to be aware of the string. The concept of a reconciled object name is general to every controller. There's a bit of a hiccup since some objects can have both names and namespaces, but for us provisioners and nodes are both un-namespaced concepts. I might name it something like

injection.WithResourceKey() // returns "name" or "namespace/name"

We could also go totally crazy and stuff in a pointer to the full object like

injection.WithObject(ctx, provisioner) injection.GetObject(ctx).GetName()

I'm not sure how I feel about this. It gives every piece of the code access to the resource that's being reconciled, which is kind of nice. It may be a little bit of an abuse of the context mechanism. Happy to discuss.

I agree injecting the entire object seems to be abusing the mechanism. Injecting the type.NamespacedName will give us a good starting point. As you suggested, now we have a unified injection package that handles options, restconfig and the resource namespaced name. It looks much cleaner this way.

This injection package will also be the perfect place to implement a generic injection function that allows us to pass multiple labels down the call path (one for each node when there are multiple nodes in the defragmentation reconcile loop). It will be very useful later when we implement the Karpenter health metrics dashboard. In that case, there may be a need to let karpenter manage the injection key. We may pass the injection key through argument dynamically. Let's revisit this later when we come to the implementation details.

ellistarn · 2021-11-23T05:31:24Z

cmd/webhook/main.go

@@ -20,6 +20,7 @@ import (
 	"github.com/awslabs/karpenter/pkg/apis"
 	"github.com/awslabs/karpenter/pkg/cloudprovider"
 	"github.com/awslabs/karpenter/pkg/cloudprovider/registry"
+	karpenterinjection "github.com/awslabs/karpenter/pkg/utils/injection"


consider injection for karpenter and kinjection or knativeinjection for knative

ellistarn · 2021-11-23T05:34:32Z

pkg/utils/injection/injection.go

+
+type resourceKey struct{}
+
+func WithResource(ctx context.Context, namespacedname types.NamespacedName) context.Context {


consider WithNamespacedName, GetNamespacedName

ellistarn · 2021-11-23T05:36:15Z

pkg/utils/injection/injection.go

+
+func GetResource(ctx context.Context) types.NamespacedName {
+	retval := ctx.Value(resourceKey{})
+	if retval == nil {


Why is this if check necessary compared to e.g. line 45

Good question. In fact there should be a nil check at line 45 as well. Or it will fail if GetOptions is called without calling WithOptions first. May need to beef up nil checking for all the places that call GetOptions.

ellistarn

Nice! Some optional comments

ellistarn · 2021-11-23T16:44:47Z

cmd/controller/main.go

@@ -67,8 +67,8 @@ func main() {

 	// Set up logger and watch for changes to log level
 	ctx := LoggingContextOrDie(config, clientSet)
-	ctx = restconfig.Inject(ctx, config)
-	ctx = options.Inject(ctx, opts)
+	ctx = karpenterinjection.WithConfig(ctx, config)


same here re: injection, knativeinjection

ellistarn

Nicely done!

felix-zhe-huang requested review from ellistarn and removed request for ellistarn November 22, 2021 16:48

ellistarn reviewed Nov 22, 2021

View reviewed changes

felix-zhe-huang closed this Nov 22, 2021

felix-zhe-huang reopened this Nov 22, 2021

felix-zhe-huang force-pushed the issue#811 branch from f6202b0 to d0aa1a6 Compare November 22, 2021 19:42

ellistarn reviewed Nov 22, 2021

View reviewed changes

felix-zhe-huang added 2 commits November 22, 2021 21:33

Refactoring logging and metrics

5f39dee

add utility functions that inject reconcile resource name into context

561e98c

felix-zhe-huang force-pushed the issue#811 branch from 87962e9 to bf5cf90 Compare November 23, 2021 02:40

ellistarn reviewed Nov 23, 2021

View reviewed changes

ellistarn previously approved these changes Nov 23, 2021

View reviewed changes

felix-zhe-huang dismissed ellistarn’s stale review via 0aa582e November 23, 2021 15:38

felix-zhe-huang force-pushed the issue#811 branch 2 times, most recently from 0aa582e to fa57c53 Compare November 23, 2021 15:47

ellistarn reviewed Nov 23, 2021

View reviewed changes

Add injecetion package that handles all context injection

fd52ec2

felix-zhe-huang force-pushed the issue#811 branch from fa57c53 to fd52ec2 Compare November 23, 2021 16:50

ellistarn approved these changes Nov 23, 2021

View reviewed changes

ellistarn merged commit 71ce076 into aws:main Nov 23, 2021

felix-zhe-huang deleted the issue#811 branch January 11, 2022 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring logging and metrics #835

Refactoring logging and metrics #835

felix-zhe-huang commented Nov 22, 2021 •

edited

Loading

netlify bot commented Nov 22, 2021 •

edited

Loading

ellistarn Nov 22, 2021

felix-zhe-huang Nov 22, 2021

ellistarn Nov 22, 2021

felix-zhe-huang Nov 22, 2021

ellistarn Nov 22, 2021

felix-zhe-huang Nov 22, 2021

ellistarn Nov 22, 2021

ellistarn Nov 22, 2021

felix-zhe-huang Nov 23, 2021

felix-zhe-huang Nov 23, 2021

ellistarn Nov 23, 2021

felix-zhe-huang Nov 23, 2021

ellistarn Nov 23, 2021

felix-zhe-huang Nov 23, 2021

ellistarn Nov 23, 2021

felix-zhe-huang Nov 23, 2021

ellistarn left a comment

ellistarn Nov 23, 2021

felix-zhe-huang Nov 23, 2021

ellistarn left a comment


		type resourceKey struct{}

		func WithResource(ctx context.Context, namespacedname types.NamespacedName) context.Context {

Refactoring logging and metrics #835

Refactoring logging and metrics #835

Conversation

felix-zhe-huang commented Nov 22, 2021 • edited Loading

netlify bot commented Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellistarn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellistarn left a comment

Choose a reason for hiding this comment

felix-zhe-huang commented Nov 22, 2021 •

edited

Loading

netlify bot commented Nov 22, 2021 •

edited

Loading