search

56quarters / cadence

An extensible Statsd client for Rust
https://docs.rs/cadence/
Apache License 2.0
84 stars 27 forks source link

Support value packing #169

Closed Jason8Ni closed 2 years ago

Jason8Ni commented 2 years ago

This PR adds support for value packing in the Datadog Datagram which is available for Datadog agent versions >=v6.25.0 && <v7.0.0 or >=v7.25.0. All metric types except set support this, since : could be in the value of a set. This allows clients to buffer histogram and distribution values and send them in fewer payload to the agent.

Example:

<METRIC_NAME>:<VALUE1>:<VALUE2>:<VALUE3>|<TYPE>|@<SAMPLE_RATE>|#<TAG_KEY_1>:<TAG_VALUE_1>,<TAG_2>

This is mainly useful for HISTOGRAM, DISTRIBUTION, and TIMING but I've added the functionality for all types except for SET. I can scope it to just HISTOGRAM, DISTRIBUTION, and TIMING if that's preferred instead.

56quarters commented 2 years ago

Thanks for the PR! I'll try to take a first pass soon.

56quarters commented 2 years ago

This is mainly useful for HISTOGRAM, DISTRIBUTION, and TIMING but I've added the functionality for all types except for SET. I can scope it to just HISTOGRAM, DISTRIBUTION, and TIMING if that's preferred instead.

I was just thinking this through, I'm not sure I understand why it would be useful for counters or gauges since callers can just aggregate those themselves. I would prefer if you just limited this to histograms, distributions, and timing. Thanks!

Jason8Ni commented 2 years ago

I've taken the suggested approach of adding Packed variants to the MetricValue enum. I've also added some tests for these as well as simple examples in lib.rs, the README and under examples/! Let me know if i've missed anything and thanks for the review!

Also, I've run clippy and fmt and fixed all the issues.

56quarters commented 2 years ago

Your changes look great! Left a few last minor comments. I'll try to resurrect the benchmarking code to make sure single-value performance is roughly the same a bit later.

The benchmarks are pretty noisy (as they usually are) but single value performance looks to be within ~5% of what it is in master. Going to squash the commits down and merge. Thanks again for your work!

Jason8Ni commented 2 years ago

The benchmarks are pretty noisy (as they usually are) but single value performance looks to be within ~5% of what it is in master. Going to squash the commits down and merge. Thanks again for your work!

Thanks for reviewing! When you get a chance, would you be able to create a new release containing these changes?

56quarters commented 2 years ago

The benchmarks are pretty noisy (as they usually are) but single value performance looks to be within ~5% of what it is in master. Going to squash the commits down and merge. Thanks again for your work!

Thanks for reviewing! When you get a chance, would you be able to create a new release containing these changes?

Sure, I'll try to put something out later this week.