Skip to content

Commit 83aa714

Browse files
sdowellgoogle-oss-prow[bot]
sdowell
authored and
google-oss-prow[bot]
committed
fix: reverse log level ordering for zap/otel-agent (#1102)
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.
1 parent bd28cd9 commit 83aa714

File tree

7 files changed

+144
-36
lines changed

7 files changed

+144
-36
lines changed

‎e2e/testcases/override_log_level_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -93,14 +93,14 @@ func TestOverrideRootSyncLogLevel(t *testing.T) {
9393
}
9494

9595
// apply override to all containers and validate
96-
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}]}}}`)
96+
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}]}}}`)
9797

9898
err = nt.Watcher.WatchObject(kinds.Deployment(),
9999
rootReconcilerName.Name, rootReconcilerName.Namespace,
100100
[]testpredicates.Predicate{
101101
testpredicates.DeploymentContainerArgsContains(reconcilermanager.Reconciler, "-v=5"),
102102
testpredicates.DeploymentContainerArgsContains(reconcilermanager.GitSync, "-v=7"),
103-
testpredicates.DeploymentContainerArgsContains(metrics.OtelAgentName, "--set=service.telemetry.logs.level=debug"),
103+
testpredicates.DeploymentContainerArgsContains(metrics.OtelAgentName, "--set=service.telemetry.logs.level=fatal"),
104104
testpredicates.DeploymentContainerArgsContains(reconcilermanager.HydrationController, "-v=9"),
105105
},
106106
)
@@ -129,7 +129,7 @@ func TestOverrideRootSyncLogLevel(t *testing.T) {
129129

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

134134
err = nt.KubeClient.MergePatch(rootSyncV1, `{"spec": {"override": {"logLevels": [{"containerName": "reconciler", "logLevel": 13}]}}}`)
135135
if !strings.Contains(err.Error(), maxError) {
@@ -216,7 +216,7 @@ func TestOverrideRepoSyncLogLevel(t *testing.T) {
216216
},
217217
{
218218
ContainerName: "otel-agent",
219-
LogLevel: 2,
219+
LogLevel: 8,
220220
},
221221
{
222222
ContainerName: "hydration-controller",
@@ -235,7 +235,7 @@ func TestOverrideRepoSyncLogLevel(t *testing.T) {
235235
testpredicates.DeploymentContainerArgsContains(reconcilermanager.Reconciler, "-v=7"),
236236
testpredicates.DeploymentContainerArgsContains(reconcilermanager.GitSync, "-v=9"),
237237
testpredicates.DeploymentContainerArgsContains(reconcilermanager.HydrationController, "-v=5"),
238-
testpredicates.DeploymentContainerArgsContains(metrics.OtelAgentName, "--set=service.telemetry.logs.level=error"),
238+
testpredicates.DeploymentContainerArgsContains(metrics.OtelAgentName, "--set=service.telemetry.logs.level=debug"),
239239
},
240240
)
241241
if err != nil {

‎manifests/reposync-crd.yaml

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -354,12 +354,14 @@ spec:
354354
pattern: ^(reconciler|git-sync|hydration-controller|oci-sync|helm-sync|gcenode-askpass-sidecar|otel-agent)$
355355
type: string
356356
logLevel:
357-
description: logLevel specifies the log level override value
358-
for the container. The default value for git-sync container
359-
is 5, while all other containers will default to 0. Allowed
360-
values are from -1 to 10
357+
description: logLevel specifies the verbosity level of the
358+
logging for a specific container. The "git-sync" and "otel-agent"
359+
containers default to 5, while all other containers default
360+
to 0. Increasing the value of logLevel increases the verbosity
361+
of the logs. Lower severity messages are logged at higher
362+
verbosity. Allowed values are from 0 to 10.
361363
maximum: 10
362-
minimum: -1
364+
minimum: 0
363365
type: integer
364366
required:
365367
- containerName
@@ -1403,12 +1405,14 @@ spec:
14031405
pattern: ^(reconciler|git-sync|hydration-controller|oci-sync|helm-sync|gcenode-askpass-sidecar|otel-agent)$
14041406
type: string
14051407
logLevel:
1406-
description: logLevel specifies the log level override value
1407-
for the container. The default value for git-sync container
1408-
is 5, while all other containers will default to 0. Allowed
1409-
values are from -1 to 10
1408+
description: logLevel specifies the verbosity level of the
1409+
logging for a specific container. The "git-sync" and "otel-agent"
1410+
containers default to 5, while all other containers default
1411+
to 0. Increasing the value of logLevel increases the verbosity
1412+
of the logs. Lower severity messages are logged at higher
1413+
verbosity. Allowed values are from 0 to 10.
14101414
maximum: 10
1411-
minimum: -1
1415+
minimum: 0
14121416
type: integer
14131417
required:
14141418
- containerName

‎manifests/rootsync-crd.yaml

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -364,12 +364,14 @@ spec:
364364
pattern: ^(reconciler|git-sync|hydration-controller|oci-sync|helm-sync|gcenode-askpass-sidecar|otel-agent)$
365365
type: string
366366
logLevel:
367-
description: logLevel specifies the log level override value
368-
for the container. The default value for git-sync container
369-
is 5, while all other containers will default to 0. Allowed
370-
values are from -1 to 10
367+
description: logLevel specifies the verbosity level of the
368+
logging for a specific container. The "git-sync" and "otel-agent"
369+
containers default to 5, while all other containers default
370+
to 0. Increasing the value of logLevel increases the verbosity
371+
of the logs. Lower severity messages are logged at higher
372+
verbosity. Allowed values are from 0 to 10.
371373
maximum: 10
372-
minimum: -1
374+
minimum: 0
373375
type: integer
374376
required:
375377
- containerName
@@ -1468,12 +1470,14 @@ spec:
14681470
pattern: ^(reconciler|git-sync|hydration-controller|oci-sync|helm-sync|gcenode-askpass-sidecar|otel-agent)$
14691471
type: string
14701472
logLevel:
1471-
description: logLevel specifies the log level override value
1472-
for the container. The default value for git-sync container
1473-
is 5, while all other containers will default to 0. Allowed
1474-
values are from -1 to 10
1473+
description: logLevel specifies the verbosity level of the
1474+
logging for a specific container. The "git-sync" and "otel-agent"
1475+
containers default to 5, while all other containers default
1476+
to 0. Increasing the value of logLevel increases the verbosity
1477+
of the logs. Lower severity messages are logged at higher
1478+
verbosity. Allowed values are from 0 to 10.
14751479
maximum: 10
1476-
minimum: -1
1480+
minimum: 0
14771481
type: integer
14781482
required:
14791483
- containerName

‎pkg/api/configsync/v1alpha1/resource_override.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -160,10 +160,12 @@ type ContainerLogLevelOverride struct {
160160
// +kubebuilder:validation:Pattern=^(reconciler|git-sync|hydration-controller|oci-sync|helm-sync|gcenode-askpass-sidecar|otel-agent)$
161161
ContainerName string `json:"containerName"`
162162

163-
// logLevel specifies the log level override value for the container.
164-
// The default value for git-sync container is 5, while all other containers will default to 0.
165-
// Allowed values are from -1 to 10
166-
// +kubebuilder:validation:Minimum=-1
163+
// logLevel specifies the verbosity level of the logging for a specific container.
164+
// The "git-sync" and "otel-agent" containers default to 5, while all other containers default to 0.
165+
// Increasing the value of logLevel increases the verbosity of the logs.
166+
// Lower severity messages are logged at higher verbosity.
167+
// Allowed values are from 0 to 10.
168+
// +kubebuilder:validation:Minimum=0
167169
// +kubebuilder:validation:Maximum=10
168170
// +kubebuilder:validation:Required
169171
LogLevel int `json:"logLevel"`

‎pkg/api/configsync/v1beta1/resource_override.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -160,10 +160,12 @@ type ContainerLogLevelOverride struct {
160160
// +kubebuilder:validation:Pattern=^(reconciler|git-sync|hydration-controller|oci-sync|helm-sync|gcenode-askpass-sidecar|otel-agent)$
161161
ContainerName string `json:"containerName"`
162162

163-
// logLevel specifies the log level override value for the container.
164-
// The default value for git-sync container is 5, while all other containers will default to 0.
165-
// Allowed values are from -1 to 10
166-
// +kubebuilder:validation:Minimum=-1
163+
// logLevel specifies the verbosity level of the logging for a specific container.
164+
// The "git-sync" and "otel-agent" containers default to 5, while all other containers default to 0.
165+
// Increasing the value of logLevel increases the verbosity of the logs.
166+
// Lower severity messages are logged at higher verbosity.
167+
// Allowed values are from 0 to 10.
168+
// +kubebuilder:validation:Minimum=0
167169
// +kubebuilder:validation:Maximum=10
168170
// +kubebuilder:validation:Required
169171
LogLevel int `json:"logLevel"`

‎pkg/reconcilermanager/controllers/reconciler_container_log_level.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ import (
2727

2828
// ReconcilerContainerLogLevelDefaults are the default log level to use for the
2929
// reconciler deployment containers.
30-
// All containers default value are 0 except git-sync which default value is 5
30+
// All containers default value are 0 except git-sync/otel-agent which default value is 5
3131
func ReconcilerContainerLogLevelDefaults() map[string]v1beta1.ContainerLogLevelOverride {
3232
return map[string]v1beta1.ContainerLogLevelOverride{
3333
reconcilermanager.Reconciler: {
@@ -56,7 +56,7 @@ func ReconcilerContainerLogLevelDefaults() map[string]v1beta1.ContainerLogLevelO
5656
},
5757
metrics.OtelAgentName: {
5858
ContainerName: metrics.OtelAgentName,
59-
LogLevel: 0,
59+
LogLevel: 5, // otel-agent default is 5, this maps to zap level "INFO"
6060
},
6161
}
6262
}
@@ -107,7 +107,9 @@ func mutateContainerLogLevel(c *corev1.Container, override []v1beta1.ContainerLo
107107
switch c.Name {
108108
case metrics.OtelAgentName:
109109
// otel-agent surfaces the log level configuration differently.
110-
zapLevel := zapcore.Level(logLevel.LogLevel)
110+
// Our log levels range from 0-10, whereas zap ranges from -1 (debug) to 5 (fatal).
111+
// We reverse the order for consistent behavior with our other log levels.
112+
zapLevel := zapcore.Level(max(int(zapcore.FatalLevel)-logLevel.LogLevel, int(zapcore.DebugLevel)))
111113
// unmarshal and marshal to validate that the zap level is valid
112114
if _, err := zapcore.ParseLevel(zapLevel.String()); err != nil {
113115
return err

‎pkg/reconcilermanager/controllers/reconciler_container_log_level_test.go

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
// Copyright 2024 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package controllers
16+
17+
import (
18+
"fmt"
19+
"testing"
20+
21+
"github.com/stretchr/testify/assert"
22+
corev1 "k8s.io/api/core/v1"
23+
"kpt.dev/configsync/pkg/api/configsync/v1beta1"
24+
"kpt.dev/configsync/pkg/metrics"
25+
)
26+
27+
func TestMutateContainerLogLevelOtelAgent(t *testing.T) {
28+
testCases := map[string]struct {
29+
logLevel int
30+
expectedLevel string
31+
}{
32+
"otel-agent with 0 log level": {
33+
logLevel: 0,
34+
expectedLevel: "fatal",
35+
},
36+
"otel-agent with 1 log level": {
37+
logLevel: 1,
38+
expectedLevel: "panic",
39+
},
40+
"otel-agent with 2 log level": {
41+
logLevel: 2,
42+
expectedLevel: "dpanic",
43+
},
44+
"otel-agent with 3 log level": {
45+
logLevel: 3,
46+
expectedLevel: "error",
47+
},
48+
"otel-agent with 4 log level": {
49+
logLevel: 4,
50+
expectedLevel: "warn",
51+
},
52+
"otel-agent with 5 log level": {
53+
logLevel: 5,
54+
expectedLevel: "info",
55+
},
56+
"otel-agent with 6 log level": {
57+
logLevel: 6,
58+
expectedLevel: "debug",
59+
},
60+
"otel-agent with 7 log level": {
61+
logLevel: 7,
62+
expectedLevel: "debug",
63+
},
64+
"otel-agent with 8 log level": {
65+
logLevel: 8,
66+
expectedLevel: "debug",
67+
},
68+
"otel-agent with 9 log level": {
69+
logLevel: 9,
70+
expectedLevel: "debug",
71+
},
72+
"otel-agent with 10 log level": {
73+
logLevel: 10,
74+
expectedLevel: "debug",
75+
},
76+
}
77+
for name, tc := range testCases {
78+
t.Run(name, func(t *testing.T) {
79+
container := &corev1.Container{
80+
Name: metrics.OtelAgentName,
81+
}
82+
override := []v1beta1.ContainerLogLevelOverride{
83+
{
84+
ContainerName: metrics.OtelAgentName,
85+
LogLevel: tc.logLevel,
86+
},
87+
}
88+
err := mutateContainerLogLevel(container, override)
89+
assert.NoError(t, err)
90+
expectedArgs := []string{fmt.Sprintf("--set=service.telemetry.logs.level=%s", tc.expectedLevel)}
91+
assert.Equal(t, expectedArgs, container.Args)
92+
})
93+
}
94+
}

0 commit comments

Comments
 (0)