elastic / elastic-agent-shipper

Data shipper for the Elastic Agent - single, unified way to add monitoring for logs, metrics, and other types of data to a host.
Other
9 stars 19 forks source link

The shipper protocol should not convert all numeric values to Float64 #243

Closed cmacknz closed 1 year ago

cmacknz commented 1 year ago

The shipper protocol currently uses a fork of the google.protobuf.Struct , intended to be the protobuf representation of a JSON object. Since JSON does not have a distinct integer or float type, google.protobuf.Struct represents all numeric values as Float64:

https://github.com/elastic/elastic-agent-shipper-client/blob/7610e1852122c0e441800549ab1121aded6e3a06/pkg/helpers/struct.go#LL218-L233C44

    case int:
        return NewNumberValue(float64(newValueTyped)), nil
    case int32:
        return NewNumberValue(float64(newValueTyped)), nil
    case int64:
        return NewNumberValue(float64(newValueTyped)), nil
    case uint:
        return NewNumberValue(float64(newValueTyped)), nil
    case uint32:
        return NewNumberValue(float64(newValueTyped)), nil
    case uint64:
        return NewNumberValue(float64(newValueTyped)), nil
    case float32:
        return NewNumberValue(float64(newValueTyped)), nil
    case float64:
        return NewNumberValue(newValueTyped), nil

While this may be technically correct, it introduces a lack of determinism into the number representation from floating point precision errors. We should instead preserve the original numeric type in the shipper protocol rather than strictly attempting to follow the JSON specification. For example, an uint32 should be serialized as a protobuf uint32. See https://protobuf.dev/programming-guides/proto3/#scalar for the full list of scalar protobuf types.

Converting all numeric values to Floats in the shipper protocol creates the opportunity for unexpected behaviour when using processors, specifically conditions. Consider a user of the Equals condition:

equals:
  http.response.code: 200

The current shipper protocol would represent 200 as 200.0 internally turning what appears to be an integer or string constant comparison into a floating point comparison with different behaviour.