Skip to content

Commit

Permalink
fix: reverse log level ordering for zap/otel-agent (#1102)
Browse files Browse the repository at this point in the history
The zap log levels become more verbose as the log level decreases, which
is the inverse of how our other log levels operate. This change reverses
the order of our logLevel API when translating it to a zap log level.
This is intended to create more consistent API behavior between the
different containers. The default continues to be INFO, which maps to
the integer value of 5 in our new scheme.
  • Loading branch information
sdowell authored and google-oss-prow[bot] committed Jan 9, 2024
1 parent bd28cd9 commit 83aa714
Show file tree
Hide file tree
Showing 7 changed files with 144 additions and 36 deletions.
10 changes: 5 additions & 5 deletions e2e/testcases/override_log_level_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,14 @@ func TestOverrideRootSyncLogLevel(t *testing.T) {
}

// apply override to all containers and validate
nt.MustMergePatch(rootSyncV1, `{"spec": {"override": {"logLevels": [{"containerName": "reconciler", "logLevel": 5}, {"containerName": "git-sync", "logLevel": 7}, {"containerName": "otel-agent", "logLevel": -1}, {"containerName": "hydration-controller", "logLevel": 9}]}}}`)
nt.MustMergePatch(rootSyncV1, `{"spec": {"override": {"logLevels": [{"containerName": "reconciler", "logLevel": 5}, {"containerName": "git-sync", "logLevel": 7}, {"containerName": "otel-agent", "logLevel": 0}, {"containerName": "hydration-controller", "logLevel": 9}]}}}`)

err = nt.Watcher.WatchObject(kinds.Deployment(),
rootReconcilerName.Name, rootReconcilerName.Namespace,
[]testpredicates.Predicate{
testpredicates.DeploymentContainerArgsContains(reconcilermanager.Reconciler, "-v=5"),
testpredicates.DeploymentContainerArgsContains(reconcilermanager.GitSync, "-v=7"),
testpredicates.DeploymentContainerArgsContains(metrics.OtelAgentName, "--set=service.telemetry.logs.level=debug"),
testpredicates.DeploymentContainerArgsContains(metrics.OtelAgentName, "--set=service.telemetry.logs.level=fatal"),
testpredicates.DeploymentContainerArgsContains(reconcilermanager.HydrationController, "-v=9"),
},
)
Expand Down Expand Up @@ -129,7 +129,7 @@ func TestOverrideRootSyncLogLevel(t *testing.T) {

// try invalid log level value
maxError := "logLevel in body should be less than or equal to 10"
minError := "logLevel in body should be greater than or equal to -1"
minError := "logLevel in body should be greater than or equal to 0"

err = nt.KubeClient.MergePatch(rootSyncV1, `{"spec": {"override": {"logLevels": [{"containerName": "reconciler", "logLevel": 13}]}}}`)
if !strings.Contains(err.Error(), maxError) {
Expand Down Expand Up @@ -216,7 +216,7 @@ func TestOverrideRepoSyncLogLevel(t *testing.T) {
},
{
ContainerName: "otel-agent",
LogLevel: 2,
LogLevel: 8,
},
{
ContainerName: "hydration-controller",
Expand All @@ -235,7 +235,7 @@ func TestOverrideRepoSyncLogLevel(t *testing.T) {
testpredicates.DeploymentContainerArgsContains(reconcilermanager.Reconciler, "-v=7"),
testpredicates.DeploymentContainerArgsContains(reconcilermanager.GitSync, "-v=9"),
testpredicates.DeploymentContainerArgsContains(reconcilermanager.HydrationController, "-v=5"),
testpredicates.DeploymentContainerArgsContains(metrics.OtelAgentName, "--set=service.telemetry.logs.level=error"),
testpredicates.DeploymentContainerArgsContains(metrics.OtelAgentName, "--set=service.telemetry.logs.level=debug"),
},
)
if err != nil {
Expand Down
24 changes: 14 additions & 10 deletions manifests/reposync-crd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -354,12 +354,14 @@ spec:
pattern: ^(reconciler|git-sync|hydration-controller|oci-sync|helm-sync|gcenode-askpass-sidecar|otel-agent)$
type: string
logLevel:
description: logLevel specifies the log level override value
for the container. The default value for git-sync container
is 5, while all other containers will default to 0. Allowed
values are from -1 to 10
description: logLevel specifies the verbosity level of the
logging for a specific container. The "git-sync" and "otel-agent"
containers default to 5, while all other containers default
to 0. Increasing the value of logLevel increases the verbosity
of the logs. Lower severity messages are logged at higher
verbosity. Allowed values are from 0 to 10.
maximum: 10
minimum: -1
minimum: 0
type: integer
required:
- containerName
Expand Down Expand Up @@ -1403,12 +1405,14 @@ spec:
pattern: ^(reconciler|git-sync|hydration-controller|oci-sync|helm-sync|gcenode-askpass-sidecar|otel-agent)$
type: string
logLevel:
description: logLevel specifies the log level override value
for the container. The default value for git-sync container
is 5, while all other containers will default to 0. Allowed
values are from -1 to 10
description: logLevel specifies the verbosity level of the
logging for a specific container. The "git-sync" and "otel-agent"
containers default to 5, while all other containers default
to 0. Increasing the value of logLevel increases the verbosity
of the logs. Lower severity messages are logged at higher
verbosity. Allowed values are from 0 to 10.
maximum: 10
minimum: -1
minimum: 0
type: integer
required:
- containerName
Expand Down
24 changes: 14 additions & 10 deletions manifests/rootsync-crd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -364,12 +364,14 @@ spec:
pattern: ^(reconciler|git-sync|hydration-controller|oci-sync|helm-sync|gcenode-askpass-sidecar|otel-agent)$
type: string
logLevel:
description: logLevel specifies the log level override value
for the container. The default value for git-sync container
is 5, while all other containers will default to 0. Allowed
values are from -1 to 10
description: logLevel specifies the verbosity level of the
logging for a specific container. The "git-sync" and "otel-agent"
containers default to 5, while all other containers default
to 0. Increasing the value of logLevel increases the verbosity
of the logs. Lower severity messages are logged at higher
verbosity. Allowed values are from 0 to 10.
maximum: 10
minimum: -1
minimum: 0
type: integer
required:
- containerName
Expand Down Expand Up @@ -1468,12 +1470,14 @@ spec:
pattern: ^(reconciler|git-sync|hydration-controller|oci-sync|helm-sync|gcenode-askpass-sidecar|otel-agent)$
type: string
logLevel:
description: logLevel specifies the log level override value
for the container. The default value for git-sync container
is 5, while all other containers will default to 0. Allowed
values are from -1 to 10
description: logLevel specifies the verbosity level of the
logging for a specific container. The "git-sync" and "otel-agent"
containers default to 5, while all other containers default
to 0. Increasing the value of logLevel increases the verbosity
of the logs. Lower severity messages are logged at higher
verbosity. Allowed values are from 0 to 10.
maximum: 10
minimum: -1
minimum: 0
type: integer
required:
- containerName
Expand Down
10 changes: 6 additions & 4 deletions pkg/api/configsync/v1alpha1/resource_override.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,10 +160,12 @@ type ContainerLogLevelOverride struct {
// +kubebuilder:validation:Pattern=^(reconciler|git-sync|hydration-controller|oci-sync|helm-sync|gcenode-askpass-sidecar|otel-agent)$
ContainerName string `json:"containerName"`

// logLevel specifies the log level override value for the container.
// The default value for git-sync container is 5, while all other containers will default to 0.
// Allowed values are from -1 to 10
// +kubebuilder:validation:Minimum=-1
// logLevel specifies the verbosity level of the logging for a specific container.
// The "git-sync" and "otel-agent" containers default to 5, while all other containers default to 0.
// Increasing the value of logLevel increases the verbosity of the logs.
// Lower severity messages are logged at higher verbosity.
// Allowed values are from 0 to 10.
// +kubebuilder:validation:Minimum=0
// +kubebuilder:validation:Maximum=10
// +kubebuilder:validation:Required
LogLevel int `json:"logLevel"`
Expand Down
10 changes: 6 additions & 4 deletions pkg/api/configsync/v1beta1/resource_override.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,10 +160,12 @@ type ContainerLogLevelOverride struct {
// +kubebuilder:validation:Pattern=^(reconciler|git-sync|hydration-controller|oci-sync|helm-sync|gcenode-askpass-sidecar|otel-agent)$
ContainerName string `json:"containerName"`

// logLevel specifies the log level override value for the container.
// The default value for git-sync container is 5, while all other containers will default to 0.
// Allowed values are from -1 to 10
// +kubebuilder:validation:Minimum=-1
// logLevel specifies the verbosity level of the logging for a specific container.
// The "git-sync" and "otel-agent" containers default to 5, while all other containers default to 0.
// Increasing the value of logLevel increases the verbosity of the logs.
// Lower severity messages are logged at higher verbosity.
// Allowed values are from 0 to 10.
// +kubebuilder:validation:Minimum=0
// +kubebuilder:validation:Maximum=10
// +kubebuilder:validation:Required
LogLevel int `json:"logLevel"`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (

// ReconcilerContainerLogLevelDefaults are the default log level to use for the
// reconciler deployment containers.
// All containers default value are 0 except git-sync which default value is 5
// All containers default value are 0 except git-sync/otel-agent which default value is 5
func ReconcilerContainerLogLevelDefaults() map[string]v1beta1.ContainerLogLevelOverride {
return map[string]v1beta1.ContainerLogLevelOverride{
reconcilermanager.Reconciler: {
Expand Down Expand Up @@ -56,7 +56,7 @@ func ReconcilerContainerLogLevelDefaults() map[string]v1beta1.ContainerLogLevelO
},
metrics.OtelAgentName: {
ContainerName: metrics.OtelAgentName,
LogLevel: 0,
LogLevel: 5, // otel-agent default is 5, this maps to zap level "INFO"
},
}
}
Expand Down Expand Up @@ -107,7 +107,9 @@ func mutateContainerLogLevel(c *corev1.Container, override []v1beta1.ContainerLo
switch c.Name {
case metrics.OtelAgentName:
// otel-agent surfaces the log level configuration differently.
zapLevel := zapcore.Level(logLevel.LogLevel)
// Our log levels range from 0-10, whereas zap ranges from -1 (debug) to 5 (fatal).
// We reverse the order for consistent behavior with our other log levels.
zapLevel := zapcore.Level(max(int(zapcore.FatalLevel)-logLevel.LogLevel, int(zapcore.DebugLevel)))
// unmarshal and marshal to validate that the zap level is valid
if _, err := zapcore.ParseLevel(zapLevel.String()); err != nil {
return err
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
// Copyright 2024 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package controllers

import (
"fmt"
"testing"

"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"
"kpt.dev/configsync/pkg/api/configsync/v1beta1"
"kpt.dev/configsync/pkg/metrics"
)

func TestMutateContainerLogLevelOtelAgent(t *testing.T) {
testCases := map[string]struct {
logLevel int
expectedLevel string
}{
"otel-agent with 0 log level": {
logLevel: 0,
expectedLevel: "fatal",
},
"otel-agent with 1 log level": {
logLevel: 1,
expectedLevel: "panic",
},
"otel-agent with 2 log level": {
logLevel: 2,
expectedLevel: "dpanic",
},
"otel-agent with 3 log level": {
logLevel: 3,
expectedLevel: "error",
},
"otel-agent with 4 log level": {
logLevel: 4,
expectedLevel: "warn",
},
"otel-agent with 5 log level": {
logLevel: 5,
expectedLevel: "info",
},
"otel-agent with 6 log level": {
logLevel: 6,
expectedLevel: "debug",
},
"otel-agent with 7 log level": {
logLevel: 7,
expectedLevel: "debug",
},
"otel-agent with 8 log level": {
logLevel: 8,
expectedLevel: "debug",
},
"otel-agent with 9 log level": {
logLevel: 9,
expectedLevel: "debug",
},
"otel-agent with 10 log level": {
logLevel: 10,
expectedLevel: "debug",
},
}
for name, tc := range testCases {
t.Run(name, func(t *testing.T) {
container := &corev1.Container{
Name: metrics.OtelAgentName,
}
override := []v1beta1.ContainerLogLevelOverride{
{
ContainerName: metrics.OtelAgentName,
LogLevel: tc.logLevel,
},
}
err := mutateContainerLogLevel(container, override)
assert.NoError(t, err)
expectedArgs := []string{fmt.Sprintf("--set=service.telemetry.logs.level=%s", tc.expectedLevel)}
assert.Equal(t, expectedArgs, container.Args)
})
}
}

0 comments on commit 83aa714

Please sign in to comment.