fix: otel sink json access logging without text field

[kraashen]

What type of PR is this?

This PR attempts to fix issue #5481 by passing only the JSON attributes to the otel formatter builder. It does this by leaving the text format empty when JSON type is specified but text is not.

What this PR does / why we need it:

As described in the linked issue, this leads to a situation where HTTPRoutes are not registering to listeners due to error returned in situations where JSON logging is used. This issue seemed to be rising when OpenTelemetry sink type is used without text field in the configuration.

Tested this in a local cluster calling the test API, and logs seemed to be produced in Loki when queried with exporter=OTEL label.

Which issue(s) this PR fixes:

Fixes #5481

Release Notes: Yes

Note

I have little to none prior experience of Envoy Gateway development. Went through the guidelines with best effort, but I'm a bit unsure if there are any possible other downstream consequences to this change. Any advice would be appreciated!

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 65.26%. Comparing base (580686a) to head (2153e2d).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/xds/translator/accesslog.go	66.66%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5498      +/-   ##
==========================================
+ Coverage   65.25%   65.26%   +0.01%     
==========================================
  Files         213      213              
  Lines       34063    34069       +6     
==========================================
+ Hits        22228    22236       +8     
+ Misses      10500    10499       -1     
+ Partials     1335     1334       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[zirain]

LGTM, defer to @guydc

[kraashen]

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)
}

[zirain]

sounds goo to me, cc @guydc

[kraashen]

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.

[guydc]

LGTM, thanks for fixing this and adding a test!

[arkodg]

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

[arkodg]

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

[kraashen]

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

[arkodg]

this looks good, sorry for the confusion

[arkodg]

LGTM thanks !

[arkodg]

@kraashen can you fix DCO and force push