Skip to content

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

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2bc017e
fix: log format doesn't uphold opentelemetry field naming
shimoncohen Aug 3, 2025
a46320f
chore: simplify nginx.conf network_protocol regex
shimoncohen Aug 3, 2025
0b756e5
chore: quote upstream_response_length in log_format.conf
shimoncohen Aug 3, 2025
538358b
Update helm/config/log_format.conf
shimoncohen Sep 8, 2025
0aaf0b3
Update helm/config/log_format.conf
shimoncohen Sep 8, 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
73 changes: 41 additions & 32 deletions helm/config/log_format.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,42 +6,51 @@ log_format main escape=json
'{'
'"Timestamp":$nanosec,'
'"Attributes":{'
'"mapcolonies.time_local":"$time_local",'
'"mapcolonies.time_local": "$time_local",'
{{ if .Values.authorization.enabled }}
'"mapcolonies.http.auth.token.client_name":"$jwt_payload_sub",'
'"mapcolonies.http.auth.token.client_name": "$jwt_payload_sub",'
Copy link

@syncush syncush Aug 3, 2025

Choose a reason for hiding this comment

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

@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 ?

{{ end }}
'"http.request.method":"$request_method",'
'"http.request.header.referer":"$http_referer",'
'"http.request.body.size":"$content_length",'
'"http.response.body.size":"$body_bytes_sent",'
'"http.response.header.x_forwarded_for":"$proxy_add_x_forwarded_for",'
'"http.response.status_code":"$status",'
'"user_agent.original":"$http_user_agent",'
'"network.protocol":"$server_protocol",'
'"mapcolonies.request_time":"$request_time",'
'"mapcolonies.http.upstream_connect_time":"$upstream_connect_time",'
'"mapcolonies.http.upstream_response_time":"$upstream_response_time",'
'"mapcolonies.http.upstream_addr":"$upstream_addr",'
'"mapcolonies.http.upstream_status_code":"$upstream_status",'
'"mapcolonies.http.upstream_cache_status":"$upstream_cache_status",'
'"mapcolonies.server":"$hostname",'
'"server.address":"$host",'
'"server.port":"$server_port",'
'"client.address":"$remote_addr",'
'"client.port":"$remote_port",'
'"url.scheme":"$scheme",'
'"url.path":"$uri",'
'"url.full":"$request_uri"'
'"http.request.method": "$request_method",'
'"http.request.header.host": "$host",'
Copy link
Contributor

@CptSchnitz CptSchnitz Aug 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@shimoncohen shimoncohen Aug 3, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@shimoncohen shimoncohen Aug 3, 2025

Choose a reason for hiding this comment

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

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

'"http.request.header.referer": "$http_referer",'
'"http.request.header.content-type": "$content_type",'
'"http.request.header.content-length": "$content_length",'
'"http.request.body.size": $request_length,'
Copy link
Contributor

@CptSchnitz CptSchnitz Aug 3, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@shimoncohen shimoncohen Aug 3, 2025

Choose a reason for hiding this comment

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

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

'"http.request.size": $bytes_sent,'
'"http.response.body.size": $body_bytes_sent,'
'"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",'
Copy link
Contributor

@CptSchnitz CptSchnitz Aug 3, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@shimoncohen shimoncohen Aug 3, 2025

Choose a reason for hiding this comment

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

Probably, but it won't hurt.

'"network.protocol.version": "$otel_network_protocol_version",'
'"mapcolonies.request_time": "$request_time",'
'"mapcolonies.http.upstream_addr": "$upstream_addr",'
'"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,'
Copy link
Contributor

@CptSchnitz CptSchnitz Aug 3, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@shimoncohen shimoncohen Aug 3, 2025

Choose a reason for hiding this comment

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

I'll put it in quotes.

'"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",'
Copy link
Contributor

@CptSchnitz CptSchnitz Aug 3, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@shimoncohen shimoncohen Aug 3, 2025

Choose a reason for hiding this comment

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

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.

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

Copilot AI Sep 8, 2025

Choose a reason for hiding this comment

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

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",'

Copilot uses AI. Check for mistakes.
'"server.port": "$server_port",'
'"client.address": "$remote_addr",'
'"client.port": "$remote_port",'
'"url.scheme": "$scheme",'
'"url.path": "$uri",'
'"url.full": "$request_uri",'
'"url.query": "$query_string"'
'},'
'"Resource":{'
# Additional important log details should be added here
'"service.name":"{{ .Values.image.repository }}",'
'"service.version":"{{ .Values.image.tag }}"'
'"service.name": "{{ .Values.image.repository }}",'
'"service.version": "{{ .Values.image.tag }}"'
'},'
'"TraceId":"$opentelemetry_trace_id",' ## this is a byte sequence (hex-encoded in JSON)
'"SpanId":"$opentelemetry_span_id",'
'"SeverityText":"INFO",'
'"SeverityNumber":9,'
'"InstrumentationScope":"access.log",'
'"Body":"$request"'
'"TraceId": "$opentelemetry_trace_id",' ## this is a byte sequence (hex-encoded in JSON)
'"SpanId": "$opentelemetry_span_id",'
'"SeverityText": "INFO",'
'"SeverityNumber": 9,'
'"InstrumentationScope": "access.log",'
'"Body": "$request"'
'}';
10 changes: 10 additions & 0 deletions helm/config/nginx.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,16 @@ http {
endpoint {{ .Values.opentelemetry.exporterHost }}:{{ .Values.opentelemetry.exporterPort }};
}

map $server_protocol $otel_network_protocol_name {
(.+)/.+$ $1;
default "http";
}

map $server_protocol $otel_network_protocol_version {
.+/(.+)$ $1;
default "1.1";
}

include /etc/nginx/mime.types;
default_type application/octet-stream;

Expand Down