Support client-side per-record sampling

[tgr]

Currently StatsdService is the only way to actually perform sampling (short of doing it manually), and it does not allow setting different sampling rates for different keys. Something like

$statsdService->increment('foo')->setSampleRate(0.01);

looks valid but actually will only account for the sampling on the server side, not the client side, since the client-side sampling check happens inside increment. IMO StatsdClient::send() would be a more reasonable place to perform the client-side sampling. (Also, setting the sample rate both in StatsdData and StatsdService will result in the latter overriding the former, which is counterintuitive; and setting the sample rate in StatsdData and also using the $sampleRate parameter of StatsdClient::send() will result in a malformed message with two sampling rates. The nice thing to do would be to rename $sampleRate to $defaultSampleRate and the StatsdService field similarly, and have the StatsdData value always override the others.)

[liuggio]

this is a good idea ... but how to implement this with no breaking things?

[tgr]

One attempt is at https://gerrit.wikimedia.org/r/#/c/226639/1/includes/libs/SamplingStatsdClient.php. It does break things insofar as it refuses string messages, but the handling of string messages is already somewhat broken. (In general IMO it is too liberal to accept already stringified messages in $data - serialization should only be done at the edges.)

[dav-m85]

@tgr This is one good point, I've felt while implementing it that it'll become a limitation indeed.

$statsdService->increment('foo')->setSampleRate(0.01);

I'm not a big fan of this interface, because it would suggest that increment is returning an object "StatsdVariable" on which I can define a specific sample rate. However, I agree what you imply here: sample rate shall be closely located to the variable it applies to.

Thus I would suggest this:

$statsdService->increment('foo', 0.01)->flush();

Where obviously the sample rate is the second optional argument.

This breaks compatibility because we need to change StatsdDataFactoryInterface. I would suggest coupling this with #51 and release a new major.

Does it sound yummy ?

[liuggio]

👍

[tgr]

It does, thanks! I would hope that the same gets applied to StatsdDataFactoryInterface, though. StatsdClient is not always the best way to do the data collection.