search

vthiery / cpp-statsd-client

A header-only StatsD client implemented in C++
MIT License
51 stars 19 forks source link

Add Sets and Histograms (Alias of Counters) and Guard the Metric Type Interface #26

Closed kevinkreiser closed 3 years ago

kevinkreiser commented 3 years ago

This issue is based on the review comment here: https://github.com/vthiery/cpp-statsd-client/pull/19#discussion_r647179020

The idea here is that we'd like to make it impossible for the client to send messages with bogus formatting but the current API allows users to send metrics manually by specifying the metric type as a string. This opens the door for users to pass a string that is not valid and would force us to validate it and set an error message if invalid. With a small (but breaking) API change we can side step the whole issue by:

  1. making the send method private
  2. add support for all the valid metric types (set and histogram are missing)

To follow with semver we will need to increment the major revision though.

@vthiery In general do you think this is a worthwhile thing to do?

vthiery commented 3 years ago
  • making the send method private
  • add support for all the valid metric types (set and histogram are missing)

This feels more robust to me and since we're pretty much reworking the whole API, this is the best time to move into this direction :+1:

kevinkreiser commented 3 years ago

In reviewing here: https://github.com/statsd/statsd/blob/9cf77d87855bcb69a8663135f59aa23825db9797/stats.js#L256-L324

It looks like tags are officially supported as well so I think we should add those too. I'd hate to have to make a new major version of the client twice in a row or do you think tags is a big enough feature we should add it in a separate PR?

vthiery commented 3 years ago

In reviewing here: https://github.com/statsd/statsd/blob/9cf77d87855bcb69a8663135f59aa23825db9797/stats.js#L256-L324

It looks like tags are officially supported as well so I think we should add those too. I'd hate to have to make a new major version of the client twice in a row or do you think tags is a big enough feature we should add it in a separate PR?

We can wait for tags to be supported before taking the next major release.