fix: log format doesn't uphold opentelemetry field naming (MAPCO-8460) #17

shimoncohen · 2025-08-03T10:44:45Z

Additional read:
https://nginx.org/en/docs/http/ngx_http_core_module.html
https://www.dash0.com/documentation/dash0/logging/nginx-logs#extracted-semantic-attributes

syncush · 2025-08-03T10:47:36Z

helm/config/log_format.conf

        {{ if .Values.authorization.enabled }}
-        '"mapcolonies.http.auth.token.client_name":"$jwt_payload_sub",'
+        '"mapcolonies.http.auth.token.client_name": "$jwt_payload_sub",'


@CptSchnitz , back in the days we tried to put mapcolonies custom fields under "mapcolonies" key, do you think we should put it in the body or message key ?

syncush · 2025-08-03T10:48:19Z

helm/config/nginx.conf

+    map $server_protocol $otel_network_protocol_name {
+        ~^(.+)/.*$  $1;
+        default     "http";
+    }
+
+    map $server_protocol $otel_network_protocol_version {
+        ~^.+/(.+)$  $1;
+        default     "1.1";
+    }
+


can you explain please?

Opentelemetry doesn't have a field for the nginx server_protocol variable and we cannot get the network.protocol.name or network.protocol.version (https://opentelemetry.io/docs/specs/semconv/registry/attributes/network/) from any nginx variable.
What we are doing here is "splitting" the server_protocol variable by the / to get the name and version.

yes i didnt understand the dark magic of ~^.+/(.+)$

I simplified the regex.

syncush · 2025-08-03T10:48:48Z

BTW we have hard coded INFO value, do we custom logs for errors? should we ?

shimoncohen · 2025-08-03T11:11:12Z

BTW we have hard coded INFO value, do we custom logs for errors? should we ?

If I understood you correctly, we can map logs based on the status code to log levels (warn for 4xx and error for 5xx) with the map directive as with the server_protocol.

syncush · 2025-08-03T12:13:54Z

BTW we have hard coded INFO value, do we custom logs for errors? should we ?

If I understood you correctly, we can map logs based on the status code to log levels (warn for 4xx and error for 5xx) with the map directive as with the server_protocol.

for 5xx we still use INFO ?

shimoncohen · 2025-08-03T12:18:39Z

BTW we have hard coded INFO value, do we custom logs for errors? should we ?

If I understood you correctly, we can map logs based on the status code to log levels (warn for 4xx and error for 5xx) with the map directive as with the server_protocol.

for 5xx we still use INFO ?

Yes, I just made sure and when we get for example 502 the value of SeverityText is INFO.

CptSchnitz · 2025-08-03T12:48:36Z

helm/config/log_format.conf

+        '"http.request.header.content-length": "$content_length",'
+        '"http.request.body.size": $request_length,'


the usage of this nginx variable does not conform to OTEL SPEC

Do you think I should change it to use "$content_length" as well?

CptSchnitz · 2025-08-03T13:32:08Z

helm/config/log_format.conf

+        '"mapcolonies.http.upstream_bytes_sent": $upstream_bytes_sent,'
+        '"mapcolonies.http.upstream_bytes_received": $upstream_bytes_received,'
+        '"mapcolonies.http.upstream_cache_status": "$upstream_cache_status",'
+        '"host.name": "$hostname",'


$hostname in nginx is for the server name (in the SERVER block)
host.name in otel is for the machine host name (not under attributes)

From the documentation:

Name of the host. On Unix systems, it may contain what the hostname command returns, or the fully qualified hostname, or another name specified by the user.

It seems that this is the recieved value from $hostname.

CptSchnitz · 2025-08-03T13:35:44Z

helm/config/log_format.conf

+        '"mapcolonies.http.upstream_status_code": "$upstream_status",'
+        '"mapcolonies.http.upstream_connect_time": "$upstream_connect_time",'
+        '"mapcolonies.http.upstream_response_time": "$upstream_response_time",'
+        '"mapcolonies.http.upstream_response_length": $upstream_response_length,'


what happens if the upstream returns 204? i remember it doesnt return a value and then the json is broken

I'll put it in quotes.

CptSchnitz · 2025-08-03T13:39:28Z

helm/config/log_format.conf

+        '"http.response.header.x_forwarded_for": "$proxy_add_x_forwarded_for",'
+        '"http.response.status_code": "$status",'
+        '"user_agent.original": "$http_user_agent",'
+        '"network.protocol.name": "$otel_network_protocol_name",'


this is not required according to the docs if its http (pretty sure its always http in our case)

Probably, but it won't hurt.

CptSchnitz · 2025-08-03T13:41:02Z

helm/config/log_format.conf

-        '"url.path":"$uri",'
-        '"url.full":"$request_uri"'
+        '"http.request.method": "$request_method",'
+        '"http.request.header.host": "$host",'


according to the docs it should be in server address
https://opentelemetry.io/docs/specs/semconv/http/http-spans/#setting-serveraddress-and-serverport-attributes
we need to make sure if theres a value for both

It seems like "$hostname" fits the server.address.

The host variable (from nginx):

in this order of precedence: host name from the request line, or host name from the “Host” request header field, or the server name matching a request

syncush · 2025-08-31T07:34:31Z

@shimoncohen @netanelC status?

Copilot

Pull Request Overview

This PR updates the nginx log format configuration to align with OpenTelemetry field naming conventions. The changes ensure that logged HTTP metrics and network protocol information follow the standardized OpenTelemetry semantic conventions for better observability and compatibility.

Adds network protocol extraction maps to parse protocol name and version from $server_protocol
Updates log field names to match OpenTelemetry semantic conventions (e.g., network.protocol.name instead of network.protocol)
Enhances request/response logging with additional OpenTelemetry-compliant fields

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
helm/config/nginx.conf	Adds map directives to extract protocol name and version from server protocol
helm/config/log_format.conf	Updates field names and values to comply with OpenTelemetry semantic conventions

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

helm/config/log_format.conf

Copilot · 2025-09-08T16:54:14Z

helm/config/log_format.conf

+        '"mapcolonies.http.upstream_bytes_received": $upstream_bytes_received,'
+        '"mapcolonies.http.upstream_cache_status": "$upstream_cache_status",'
+        '"host.name": "$hostname",'
+        '"server.address": "$server_addr",'


According to OpenTelemetry semantic conventions, server.address should represent the logical server hostname/address that the client is connecting to, not the physical server IP. This should likely use $host or $server_name instead of $server_addr.

Suggested change

'"server.address": "$server_addr",'

'"server.address": "$host",'

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

helm/config/log_format.conf

fix: log format doesn't uphold opentelemetry field naming

2bc017e

shimoncohen requested review from alex131290, CptSchnitz, syncush and netanelC August 3, 2025 10:44

shimoncohen self-assigned this Aug 3, 2025

syncush reviewed Aug 3, 2025

View reviewed changes

chore: simplify nginx.conf network_protocol regex

a46320f

CptSchnitz requested changes Aug 3, 2025

View reviewed changes

chore: quote upstream_response_length in log_format.conf

0b756e5

shimoncohen requested a review from Copilot September 8, 2025 16:53

Copilot AI reviewed Sep 8, 2025

View reviewed changes

Update helm/config/log_format.conf

538358b

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

shimoncohen commented Sep 8, 2025

View reviewed changes

helm/config/log_format.conf Outdated Show resolved Hide resolved

Update helm/config/log_format.conf

0aaf0b3

		'"http.request.header.content-length": "$content_length",'
		'"http.request.body.size": $request_length,'

	'"server.address": "$server_addr",'
	'"server.address": "$host",'

fix: log format doesn't uphold opentelemetry field naming (MAPCO-8460) #17

Are you sure you want to change the base?

fix: log format doesn't uphold opentelemetry field naming (MAPCO-8460) #17

Uh oh!

Conversation

shimoncohen commented Aug 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syncush commented Aug 3, 2025

Uh oh!

shimoncohen commented Aug 3, 2025

Uh oh!

syncush commented Aug 3, 2025

Uh oh!

shimoncohen commented Aug 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CptSchnitz Aug 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syncush commented Aug 31, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shimoncohen commented Aug 3, 2025 •

edited

Loading

CptSchnitz Aug 3, 2025 •

edited

Loading