envoy icon indicating copy to clipboard operation
envoy copied to clipboard

json log with sort_properties prints decimals for all numbers

Open larhauga opened this issue 1 year ago • 2 comments

Title: json log with sort_properties prints decimals for all numbers

Description:

The json logs when using FileAccessLog are serialized as floating numbers with decimals rather than integers. An example is response codes which are logged as "HTTP" 200.0. We see this when json_format_options.sort_properties: true is set (introduced in https://github.com/envoyproxy/envoy/commit/fc33e5d94f6919f6748e468b75df607bdd667e24).

{"bytes_received":0.0,"bytes_sent":11576.0,"duration":6.0,"method":"GET","response_code":200.0,"upstream_service_time":"6"}...

Repro steps: To reproduce, we use istio 1.22 (envoy b2dd15dc498b84431c063042e7821e179d5a3c41/1.30.2-dev) with the following configuration. The change in the format is json_format_options.sort_properties: true.

          - name: envoy.filters.network.http_connection_manager                                                                                                                                                                                                                                                                
            typed_config:                                                                                                                                                                                                                                                                                                      
              '@type': type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager                                                                                                                                                                                                   
              access_log:                                                                                                                                                                                                                                                                                                      
              - name: envoy.access_loggers.file                                                                                                                                                                                                                                                                                
                typed_config:                                                                                                                                                                                                                                                                                                  
                  '@type': type.googleapis.com/envoy.extensions.access_loggers.file.v3.FileAccessLog                                                                                                                                                                                                                           
                  log_format:                                                                                                                                                                                                                                                                                                  
                    json_format:                                                                                                                                                                                                                                                                                               
                      authority: '%REQ(:AUTHORITY)%'                                                                                                                                                                                                                                                                           
                      bytes_received: '%BYTES_RECEIVED%'                                                                                                                                                                                                                                                                       
                      bytes_sent: '%BYTES_SENT%'                                                                                                                                                                                                                                                                               
                      connection_termination_details: '%CONNECTION_TERMINATION_DETAILS%'                                                                                                                                                                                                                                       
                      downstream_local_address: '%DOWNSTREAM_LOCAL_ADDRESS%'                                                                                                                                                                                                                                                   
                      downstream_remote_address: '%DOWNSTREAM_REMOTE_ADDRESS%'                                                                                                                                                                                                                                                 
                      duration: '%DURATION%'                                                                                                                                                                                                                                                                                   
                      method: '%REQ(:METHOD)%'                                                                                                                                                                                                                                                                                 
                      path: '%REQ(X-ENVOY-ORIGINAL-PATH?:PATH)%'                                                                                                                                                                                                                                                               
                      protocol: '%PROTOCOL%'                                                                                                                                                                                                                                                                                   
                      request_id: '%REQ(X-REQUEST-ID)%'                                                                                                                                                                                                                                                                        
                      requested_server_name: '%REQUESTED_SERVER_NAME%'                                                                                                                                                                                                                                                         
                      response_code: '%RESPONSE_CODE%'                                                                                                                                                                                                                                                                         
                      response_code_details: '%RESPONSE_CODE_DETAILS%'                                                                                                                                                                                                                                                         
                      response_flags: '%RESPONSE_FLAGS%'                                                                                                                                                                                                                                                                       
                      route_name: '%ROUTE_NAME%'                                                                                                                                                                                                                                                                               
                      start_time: '%START_TIME%'                                                                                                                                                                                                                                                                               
                      upstream_cluster: '%UPSTREAM_CLUSTER%'                                                                                                                                                                                                                                                                   
                      upstream_host: '%UPSTREAM_HOST%'                                                                                                                                                                                                                                                                         
                      upstream_local_address: '%UPSTREAM_LOCAL_ADDRESS%'                                                                                                                                                                                                                                                       
                      upstream_service_time: '%RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)%'                                                                                                                                                                                                                                           
                      upstream_transport_failure_reason: '%UPSTREAM_TRANSPORT_FAILURE_REASON%'                                                                                                                                                                                                                                 
                      user_agent: '%REQ(USER-AGENT)%'                                                                                                                                                                                                                                                                          
                      x_forwarded_for: '%REQ(X-FORWARDED-FOR)%'                                                                                                                                                                                                                                                                
                    json_format_options:                                                                                                                                                                                                                                                                                       
                      sort_properties: true                                                                                                                                                                                                                                                                                    
                  path: /dev/stdout                                                                                                                                                                                                                                                                                            

Note: The Envoy_collect tool gathers a tarball with debug logs, config and the following admin endpoints: /stats, /clusters and /server_info. Please note if there are privacy concerns, sanitize the data prior to sharing the tarball/pasting.

Admin and Stats Output:

Include the admin output for the following endpoints: /stats, /clusters, /routes, /server_info. For more information, refer to the admin endpoint documentation.

 "version": "b2dd15dc498b84431c063042e7821e179d5a3c41/1.30.2-dev/Clean/RELEASE/BoringSSL",
  "user_agent_name": "envoy",
  "user_agent_build_version": {
   "version": {
    "major_number": 1,
    "minor_number": 30,
    "patch": 2
   },
   "metadata": {
    "revision.sha": "b2dd15dc498b84431c063042e7821e179d5a3c41",
    "ssl.version": "BoringSSL",
    "build.label": "dev",
    "revision.status": "Clean",
    "build.type": "RELEASE"
   }
  },

Note: If there are privacy concerns, sanitize the data prior to sharing.

Config:

Include the config used to configure Envoy.

Logs:

Include the access logs and the Envoy logs.

Note: If there are privacy concerns, sanitize the data prior to sharing.

Call Stack:

If the Envoy binary is crashing, a call stack is required. Please refer to the Bazel Stack trace documentation.

larhauga avatar May 30 '24 05:05 larhauga

This might be interesting for a beginner to take on. For json stats generation I wrote a fast json serializer that I think will not have this problem (and possibly be faster for logs): https://github.com/envoyproxy/envoy/blob/main/source/common/json/json_streamer.h

Not sure how easy it would be to incorporate into the access logger. But I'll call this a beginner project for any aspiring Envoy devs.

jmarantz avatar May 31 '24 20:05 jmarantz

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

github-actions[bot] avatar Jul 01 '24 00:07 github-actions[bot]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

github-actions[bot] avatar Jul 08 '24 04:07 github-actions[bot]

Can we reopen this?

howardjohn avatar Jul 08 '24 12:07 howardjohn

The issue seems to be this JSON values can be one of 5 types namely, string, double, uint64_t, int64_t and bool as described over here

But we have defined formatter bases only for number(double) and string over here

Johan511 avatar Sep 05 '24 07:09 Johan511

I would like to work on it, please assign this to me.

shreemaan-abhishek avatar Sep 25 '24 14:09 shreemaan-abhishek

Oh this might be solved by https://github.com/envoyproxy/envoy/pull/35545 -- @wbpcode WDYT?

jmarantz avatar Sep 25 '24 14:09 jmarantz

All numbers is float now in the log formatter (and in the Protobuf::Value)

The key problem is the Nlohmann json lib. In the previous implemention of json formatter, if sort is required, the Nlohmann json will be used to generate the output. That lib will serialize the number according it's type strictly. So, the response code will be 200.0.

wbpcode avatar Sep 25 '24 14:09 wbpcode

Oh this might be solved by https://github.com/envoyproxy/envoy/pull/35545 -- @wbpcode WDYT?

Yeah, I think so.

wbpcode avatar Sep 25 '24 14:09 wbpcode

Just gave this a shot, and I still see the decimals using the JSON logger. Can I get this assigned to me?

grnmeira avatar Oct 02 '24 15:10 grnmeira

sure but are you certain you are trying with #35545 in your build? Or are you using a release binary that might not include it?

jmarantz avatar Oct 02 '24 16:10 jmarantz

I think maybe an earlier version of #35545 would have fixed this by inheriting the behavior from JsonStreamer:

https://github.com/envoyproxy/envoy/blob/520442a7e2b676ee7c710fcea26ae904be6abc65/source/common/json/json_streamer.h#L389

See the overrides for addNumber with integer args.

However I think we wound up diverging the json access logger from the json streamer in the final version, so you might need to duplicate the behavior of that overload.

jmarantz avatar Oct 02 '24 16:10 jmarantz

It should be there, it was a checkout from this morning. I'll double check anyways.

grnmeira avatar Oct 02 '24 16:10 grnmeira

Remember you still need to set a runtime flag for now. Our unit tests tell me it should be fixed.

wbpcode avatar Oct 03 '24 09:10 wbpcode

Oh OK, I totally missed that. Will try with the flag when I have a chance.

grnmeira avatar Oct 03 '24 21:10 grnmeira

I just verified with the flag (envoy.reloadable_fatures.logging_with_fast_json_formatter) and yes, it does fix the issue. From the comments in the code if I understand correctly this is supposed to launch on 1.32. Meanwhile, can we remove the "beginner" and "help wanted" tags maybe? Not sure if there's a more appropriate issue state (other than "open") for this. Thanks for all the help everyone.

grnmeira avatar Oct 04 '24 11:10 grnmeira

1.32 on the way, shall we close this @wbpcode?

grnmeira avatar Oct 16 '24 13:10 grnmeira

should be closed by #36629

wbpcode avatar Oct 16 '24 14:10 wbpcode