Skip to content

fix: otel sink json access logging without text field #5498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
arkodg merged 4 commits into from
Mar 22, 2025
Merged

fix: otel sink json access logging without text field #5498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4bb15af
fix otel sink json access logging without text field
kraashen Mar 14, 2025
5ea3f3b
use json format as default when format or type is not set
kraashen Mar 18, 2025
fac129c
set formatters only if the slice of formatters is not empty
kraashen Mar 20, 2025
2153e2d
Merge branch 'main' into fix/otel-json-accesslog
arkodg Mar 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions internal/xds/translator/accesslog.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,10 +336,10 @@ func buildXdsAccessLog(al *ir.AccessLog, accessLogType ir.ProxyAccessLogType) ([
ResourceAttributes: convertToKeyValueList(otel.Resources, false),
}

if otel.Text == nil {
return nil, errors.New("otel.Text is nil")
var format string
if otel.Text != nil && *otel.Text != "" {
format = *otel.Text
Comment on lines 340 to 341
Copy link
Copy Markdown
Contributor Author

@kraashen kraashen Mar 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also do a separate check for JSON logging? I.e. check if JSON attributes are defined or not, and in case not, default to EnvoyJSONLogFields?

Something along the lines

if len(otel.Attributes) != 0 {
	al.Attributes = convertToKeyValueList(otel.Attributes, true)
	formatters = accessLogOpenTelemetryFormatters(format, otel.Attributes)
} else {
	// if there are no attributes, use the default EnvoyJSONLogFields
	al.Attributes = convertToKeyValueList(EnvoyJSONLogFields, true)
	formatters = accessLogOpenTelemetryFormatters(format, EnvoyJSONLogFields)
}

Copy link
Copy Markdown
Member

@zirain zirain Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds goo to me, cc @guydc

Copy link
Copy Markdown
Contributor Author

@kraashen kraashen Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this took a bit of time. Went through the OpenTelemetry specs and documentation on the field. Implemented following:

  • When no text or JSON type is set, default to EnvoyJSONLogFields, similar to previous implementation with the respective text format prior to v1.3.1/v1.2.7

The JSON fields are populated to the accessLog Attributes field as before with the JSON fields. Went through the OpenTelemetry specification discussion on this and relevant docs and to me it seems that this should be an OK way to go.

}
format := *otel.Text

if format != "" {
al.Body = &otlpcommonv1.AnyValue{
Expand All @@ -349,9 +349,17 @@ func buildXdsAccessLog(al *ir.AccessLog, accessLogType ir.ProxyAccessLogType) ([
}
}

al.Attributes = convertToKeyValueList(otel.Attributes, true)
var attrs map[string]string
Copy link
Copy Markdown
Contributor

@arkodg arkodg Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesnt this feel weird, if we have a format its part of Body, else Attributes

Copy link
Copy Markdown
Contributor

@arkodg arkodg Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move this to another GH issue and only have the fix around

		if otel.Text != nil && *otel.Text != "" {
			format = *otel.Text

in this PR so we can add it into v1.3.2 , releasing soon

Copy link
Copy Markdown
Contributor Author

@kraashen kraashen Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be done! Only concern is, how does this behave in case no output type or format/attributes are defined? As JSON logging is now default, does it render logs empty for users who have neither defined? Previously in v1.3.0, the EnvoyTextLogFormat was set as default and then it was checked if log format was defined. https://github.com/envoyproxy/gateway/blob/v1.3.0/internal/xds/translator/accesslog.go#L325-L336

Copy link
Copy Markdown
Contributor

@arkodg arkodg Mar 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good, sorry for the confusion

if len(otel.Attributes) != 0 {
attrs = otel.Attributes
} else if len(otel.Attributes) == 0 && format == "" {
// if there are no attributes and text format is unset, use the default EnvoyJSONLogFields
attrs = EnvoyJSONLogFields
}

al.Attributes = convertToKeyValueList(attrs, true)
formatters := accessLogOpenTelemetryFormatters(format, attrs)

formatters := accessLogOpenTelemetryFormatters(format, otel.Attributes)
if len(formatters) != 0 {
al.Formatters = formatters
}
Expand Down
1 change: 1 addition & 0 deletions release-notes/current.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ bug fixes: |
Added support for BackendTLSPolicy and EnvoyExtensionPolicy parsing in Standalone mode.
Retrigger reconciliation when backendRef of type ServiceImport is updated or when EndpointSlice(s) for a ServiceImport are updated.
Fix not logging an error and returning it in the K8s Reconcile method when a GatewayClass is not accepted.
Fix allowing empty text field for opentelemetry sink when using JSON format.

# Enhancements that improve performance.
performance improvements: |
Expand Down
61 changes: 61 additions & 0 deletions test/e2e/testdata/accesslog-otel-default.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
name: accesslog-gtw
namespace: gateway-conformance-infra
spec:
gatewayClassName: {GATEWAY_CLASS_NAME}
listeners:
- name: http
port: 80
protocol: HTTP
allowedRoutes:
namespaces:
from: Same
infrastructure:
parametersRef:
group: gateway.envoyproxy.io
kind: EnvoyProxy
name: accesslog-otel
---
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: EnvoyProxy
metadata:
name: accesslog-otel
namespace: gateway-conformance-infra
spec:
ipFamily: IPv4
telemetry:
accessLog:
settings:
- matches:
- "'x-envoy-logged' in request.headers"
sinks:
- type: OpenTelemetry
openTelemetry:
backendRefs:
- name: otel-collector
namespace: monitoring
port: 4317
resources:
k8s.cluster.name: "envoy-gateway"
shutdown:
drainTimeout: 5s
minDrainDuration: 1s
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
name: accesslog-otel
namespace: gateway-conformance-infra
spec:
parentRefs:
- name: accesslog-gtw
rules:
- matches:
- path:
type: PathPrefix
value: /otel
backendRefs:
- name: infra-backend-v2
port: 8080
79 changes: 79 additions & 0 deletions test/e2e/testdata/accesslog-otel-json.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
name: accesslog-gtw
namespace: gateway-conformance-infra
spec:
gatewayClassName: {GATEWAY_CLASS_NAME}
listeners:
- name: http
port: 80
protocol: HTTP
allowedRoutes:
namespaces:
from: Same
infrastructure:
parametersRef:
group: gateway.envoyproxy.io
kind: EnvoyProxy
name: accesslog-otel
---
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: EnvoyProxy
metadata:
name: accesslog-otel
namespace: gateway-conformance-infra
spec:
ipFamily: IPv4
telemetry:
accessLog:
settings:
- format:
type: JSON
json:
time: "%START_TIME%"
method: "%REQ(:METHOD)%"
metadata: "%METADATA(ROUTE:envoy-gateway:resources)%"
x-envoy-original-path: "%REQ(X-ENVOY-ORIGINAL-PATH?:PATH)%"
protocol: "%PROTOCOL%"
responseCode: "%RESPONSE_CODE%"
responseFlags: "%RESPONSE_FLAGS%"
bytesReceived: "%BYTES_RECEIVED%"
bytesSent: "%BYTES_SENT%"
duration: "%DURATION%"
x-forwarded-for: "%REQ(X-FORWARDED-FOR)%"
user-agent: "%REQ(USER-AGENT)%"
x-request-id: "%REQ(X-REQUEST-ID)%"
authority: "%REQ(:AUTHORITY)%"
upstreamHost: "%UPSTREAM_HOST%"
matches:
- "'x-envoy-logged' in request.headers"
sinks:
- type: OpenTelemetry
openTelemetry:
backendRefs:
- name: otel-collector
namespace: monitoring
port: 4317
resources:
k8s.cluster.name: "envoy-gateway"
shutdown:
drainTimeout: 5s
minDrainDuration: 1s
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
name: accesslog-otel
namespace: gateway-conformance-infra
spec:
parentRefs:
- name: accesslog-gtw
rules:
- matches:
- path:
type: PathPrefix
value: /otel
backendRefs:
- name: infra-backend-v2
port: 8080
114 changes: 110 additions & 4 deletions test/e2e/tests/accesslog.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
)

func init() {
ConformanceTests = append(ConformanceTests, FileAccessLogTest, OpenTelemetryTest, ALSTest)
ConformanceTests = append(ConformanceTests, FileAccessLogTest, OpenTelemetryTestText, OpenTelemetryTestJSON, ALSTest, OpenTelemetryTestJSONAsDefault)
}

var FileAccessLogTest = suite.ConformanceTest{
Expand Down Expand Up @@ -116,9 +116,9 @@ var FileAccessLogTest = suite.ConformanceTest{
},
}

var OpenTelemetryTest = suite.ConformanceTest{
ShortName: "OpenTelemetryAccessLog",
Description: "Make sure OpenTelemetry access log is working",
var OpenTelemetryTestText = suite.ConformanceTest{
ShortName: "OpenTelemetryTextAccessLog",
Description: "Make sure OpenTelemetry text access log is working",
Manifests: []string{"testdata/accesslog-otel.yaml"},
Test: func(t *testing.T, suite *suite.ConformanceTestSuite) {
labels := map[string]string{
Expand Down Expand Up @@ -169,6 +169,112 @@ var OpenTelemetryTest = suite.ConformanceTest{
},
}

var OpenTelemetryTestJSONAsDefault = suite.ConformanceTest{
ShortName: "OpenTelemetryAccessLogJSONAsDefault",
Description: "Make sure OpenTelemetry JSON access log is working as default when no format or type is specified",
Manifests: []string{"testdata/accesslog-otel-default.yaml"},
Test: func(t *testing.T, suite *suite.ConformanceTestSuite) {
labels := map[string]string{
"k8s_namespace_name": "envoy-gateway-system",
"exporter": "OTLP",
}

ns := "gateway-conformance-infra"
routeNN := types.NamespacedName{Name: "accesslog-otel", Namespace: ns}
gwNN := types.NamespacedName{Name: "accesslog-gtw", Namespace: ns}
gwAddr := kubernetes.GatewayAndHTTPRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), routeNN)

t.Run("Positive", func(t *testing.T) {
expectedResponse := httputils.ExpectedResponse{
Request: httputils.Request{
Path: "/otel",
Headers: map[string]string{
"x-envoy-logged": "1",
},
},
Response: httputils.Response{
StatusCode: 200,
},
Namespace: ns,
}
// make sure listener is ready
httputils.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, expectedResponse)

runLogTest(t, suite, gwAddr, expectedResponse, labels, "", 1)
})

t.Run("Negative", func(t *testing.T) {
expectedResponse := httputils.ExpectedResponse{
Request: httputils.Request{
Path: "/otel",
// envoy will not log this request without the header x-envoy-logged
},
Response: httputils.Response{
StatusCode: 200,
},
Namespace: ns,
}
// make sure listener is ready
httputils.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, expectedResponse)

runLogTest(t, suite, gwAddr, expectedResponse, labels, "", 0)
})
},
}

var OpenTelemetryTestJSON = suite.ConformanceTest{
ShortName: "OpenTelemetryAccessLogJSON",
Description: "Make sure OpenTelemetry JSON access log is working with custom JSON attributes",
Manifests: []string{"testdata/accesslog-otel-json.yaml"},
Test: func(t *testing.T, suite *suite.ConformanceTestSuite) {
labels := map[string]string{
"k8s_namespace_name": "envoy-gateway-system",
"exporter": "OTLP",
}

ns := "gateway-conformance-infra"
routeNN := types.NamespacedName{Name: "accesslog-otel", Namespace: ns}
gwNN := types.NamespacedName{Name: "accesslog-gtw", Namespace: ns}
gwAddr := kubernetes.GatewayAndHTTPRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), routeNN)

t.Run("Positive", func(t *testing.T) {
expectedResponse := httputils.ExpectedResponse{
Request: httputils.Request{
Path: "/otel",
Headers: map[string]string{
"x-envoy-logged": "1",
},
},
Response: httputils.Response{
StatusCode: 200,
},
Namespace: ns,
}
// make sure listener is ready
httputils.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, expectedResponse)

runLogTest(t, suite, gwAddr, expectedResponse, labels, "", 1)
})

t.Run("Negative", func(t *testing.T) {
expectedResponse := httputils.ExpectedResponse{
Request: httputils.Request{
Path: "/otel",
// envoy will not log this request without the header x-envoy-logged
},
Response: httputils.Response{
StatusCode: 200,
},
Namespace: ns,
}
// make sure listener is ready
httputils.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, expectedResponse)

runLogTest(t, suite, gwAddr, expectedResponse, labels, "", 0)
})
},
}

var ALSTest = suite.ConformanceTest{
ShortName: "ALS",
Description: "Make sure ALS access log is working",
Expand Down