Not sending float numbers that are less than 1 for Counters to datadog. about veneur

[cory-stripe]

Hey there @jingyuanswitchco. I am actually not sure of the behavior of non-integer counters. This likely undefined behavior for us because when we think of counters we increment them in whole numbers.

Thanks for opening the issue, we will write up a test case and check.

As for the change in behavior, we didn't explicitly change anything that I think would have affected this behavior, but since it's undefined perhaps we did!

[jingyuanswitchco]

Thanks for the super fast response @cory-stripe!. I will also try to check with the latest code and verify whether it fixed the issue and update you if I find something.

[cory-stripe]

No problem! If you find anything suspicious do let us know. Otherwise we'll hopefully find it by writing some test cases.

[jingyuanswitchco]

I saw the latest code is using golang 1.9. However on the official website: https://golang.org/dl/. It is listing go1.9 as unstable version. I am wondering why does Veneur need to upgrade to 1.9? Or is it possible I can still compile it using go1.8.3?

[cory-stripe]

It's possible to still use 1.8.3, but we use 1.9 for our internal stuff.

[jingyuanswitchco]

I am trying to compile the latest code with 1.8.3. But I run into the error when running command gofmt -w .:
vendor/golang.org/x/net/context/go19.go:15:14: expected type, found '='
vendor/golang.org/x/net/context/go19.go:20:17: expected type, found '='
vendor/golang.org/x/net/ipv4/batch.go:59:14: expected type, found '='
vendor/golang.org/x/net/ipv4/batch.go:78:2: expected declaration, found 'if'
vendor/golang.org/x/net/ipv6/batch.go:56:14: expected type, found '='
vendor/golang.org/x/net/ipv6/batch.go:69:2: expected declaration, found 'if'
I understand it is a formatting command. Just a FYI. I skipped the formatting and it compiled successfully now.

[cory-stripe]

Ah, yes. I believe we had to make some changes for go fmt in 1.9 now that you mention it. Sorry about that!

[jingyuanswitchco]

No worries:) I just tried to run the latest code. The same issue is still there. Do you plan to get this fixed?

[cory-stripe]

I plan to write some tests to exercise it, but I have no ETA on this. We've got some other stuff in progress at the moment.

[jingyuanswitchco]

Is it possible you can quickly point me to the code where we are handling this. And I can investigate it.

[cory-stripe]

I'm unsure. You could write a test in the sampler testing code to see if perhaps the counter doesn't work. Outside of that you'd have to test the JSON marshaling code. I'm also unsure if Datadog even works with a float counter, but your issue implies it does.

Othewise, I'm sorry but I can't tell you a specific time. This being OSS we have other obligations and these come as we can get to them!

[jingyuanswitchco]

Thanks Cory! Yes datadog agent worked fine with float counters.

[jingyuanswitchco]

Hi Cory,

I found:

veneur/samplers/samplers.go

Line 84 in 47f0a97

c.value += int64(sample) * int64(1/sampleRate)

this line is essentially change a float to an integer. Which would convert float number that is smaller than 1 to 0. Since the sample was passed in as a float64, I am wondering why do we want to force it to convert it to int64? If I want to submit a change and not converting sample to int64, is it an acceptable change?

Thanks,
Jingyuan

[jingyuanswitchco]

veneur/samplers/samplers.go

Line 79 in 47f0a97

value int64

It seems like it is because Counter is defined as an int64. Not sure if I changed it to float64 something will be broken.

[cory-stripe]

Gotya, in rereading this issue I now see that I misunderstood the original ask. You are pointing out that Veneur differs from the official DogStatsD client in that it does not work with counters. I misunderstood your original statements to mean that veneur's behavior had changed.

Ok, we'll work on this soon. I'm not sure how much work it entails. I'll report back to you!

[jingyuanswitchco]

Hi Cory,

I tried a fix today:
diff --git a/samplers/samplers.go b/samplers/samplers.go
index aa5dfa6..6076fae 100644
--- a/samplers/samplers.go
+++ b/samplers/samplers.go
@@ -76,12 +76,12 @@ type JSONMetric struct {
type Counter struct {
Name string
Tags []string

• value int64

• value float64
  }

// Sample adds a sample to the counter.
func (c *Counter) Sample(sample float64, sampleRate float32) {

• c.value += int64(sample) * int64(1/sampleRate)

• c.value += sample * (1/float64(sampleRate))
  }

// Flush generates a DDMetric from the current state of this Counter.
@@ -119,7 +119,7 @@ func (c *Counter) Export() (JSONMetric, error) {

// Combine merges the values seen with another set (marshalled as a byte slice)
func (c *Counter) Combine(other []byte) error {

• var otherCounts int64

• var otherCounts float64
  buf := bytes.NewReader(other)
  err := binary.Read(buf, binary.LittleEndian,

And after I changed the int64 to float64, it seems fixed the issue when I tested this on our infra. Not sure this may violate any design principle for Veneur. Please let me know if this helps.

Best,
Jingyuan

[cory-stripe]

Unfortunately I don't think this change is simple. Because of our naive serialization (the bytes above) trying to decode this would fail unless all veneurs were update at the same time. In other words, this would fail if someone sent a byte-encoded int64 to a box expecting a float64.

We likely need to redesign this to use floats for everything or something like that. Doing so will require a new import API. I don't think we'll have a quick solution here…

[ChimeraCoder]

There is a way to allow deserialization of either float or int types simultaneously, allowing for a gradual upgrade, but it'd be kind of cumbersome and a potentially problematic performance hit. We'd have to profile it and test it out to know whether or not it'd be problematic to update it in production.

[cory-stripe]

Neat!

[jingyuanswitchco]

Hi Cory,

Just saw you put this as an enhancement. Just want to touch base with you whether this will be fixed soon or it will need more time and there is no ETA to put in the fix.

Best,
Jingyuan

[cory-stripe]

No ETA, there's a speed bump here that we don't have an easy way to made this backward compatible, so I'm thinking will look at this for 1.7 or 2.0.