Netflix-Skunkworks / spectatord

A high performance metrics daemon
Apache License 2.0
24 stars 5 forks source link

Specatord Should Parse Monotonic Counters as uint64 #86

Closed copperlight closed 2 months ago

copperlight commented 4 months ago

Right now, all protocol lines are parsed into doubles, which could overflow if we accept uint64.

https://github.com/Netflix-Skunkworks/spectatord/blob/main/server/spectatord.cc#L252-L257

We should split the parsing based upon the type.

copperlight commented 4 months ago

Proposed Solution

Since we will now be storing either a double or a uint64 as a measurement value, and we do not want to introduce additional storage overhead, create a valueT union which is then referenced by the measurement struct. The access pattern for the value then becomes m->value.v or m->value.u, and this pattern needs to be percolated across the codebase, but it will incur no additional memory storage overhead, because it will only allocate what is needed for the type being used.

In spectatord.h:54:

union valueT {
  double d;
  uint64_t u;
};

struct measurement {
  spectator::Id id;
  valueT value;
};

In order to switch between storing uint64_t for monotonic counters, the get_measurement function needs to be updated to accept the meter type, which is parsed early.

In spectatord.cc:220, add the meter type to the parameter list:

auto get_measurement(std::string_view measurement_str, std::string* err_msg) {}

In spectatord.cc:506, pass the meter type:

auto measurement = get_measurement(type, p, &err_msg);

In spectatord.cc:253, conditionally parse a double or a uint:

auto value = std::strtod(value_str, &last_char);
copperlight commented 2 months ago

Fixed with https://github.com/Netflix-Skunkworks/spectatord/pull/90