Support timestamp value for DogstatsD metrics

[Nevon]

Newer versions of the DogstatsD protocol support an optional timestamp value for count and gauge metrics: https://docs.datadoghq.com/developers/dogstatsd/datagram_shell/?tab=metrics#dogstatsd-protocol-v13

This is defined as a UNIX timestamp in the past, prefixed with T. For example:

page.views:15|c|#env:dev|T1656581400

Looking at the methods exposed on Client it doesn't seem to be possible to pass this along. Maybe if you construct it manually and call send, but then the timestamp will come before the tags, which I'm not sure if it's meaningful in the statsd protocol, but at least it doesn't match what's in the Datadog docs.

How do you think about these flavor-specific extensions? I might be open to implementing support for this, but I would like some guidance on how you would prefer the interface to work. They also have another extension for container id, which also isn't possible to pass.

[bdeitte]

Thanks for the nice writeup here. So we've had three ways so far that flavor-specific extensions have worked:

1. Specific params/methods, as shown in https://github.com/brightcove/hot-shots#dogstatsd-and-telegraf-functionality
2. A flag to enable them, as is currently only done with the "telegraf" option
3. Adding them in directly, in cases where it doesn't feel too breaking to other setups

This to me would be case #1 if I'm understanding correctly? So a new param passed into gauge and count methods which is specific to Datadog.

[Nevon]

Thanks for the reply! I originally considered the first option as well, as it seemed that was how some of the features were already implemented. The reason I didn't immediately go for that was due to the methods using positional arguments where multiple of the arguments are optional already. Taking gauge as an example the resulting type would be something like:

gauge(stat: string | string[], value: number, callback?: StatsCb): void;
gauge(stat: string | string[], value: number, sampleRate?: number, callback?: StatsCb): void;
gauge(stat: string | string[], value: number, tags?: Tags, callback?: StatsCb): void;
gauge(stat: string | string[], value: number, sampleRate?: number, tags?: Tags, callback?: StatsCb): void;
gauge(stat: string | string[], value: number, sampleRate?: number, tags?: Tags, timestamp?: Date, callback?: StatsCb): void;
gauge(stat: string | string[], value: number, tags?: Tags, timestamp?: Date, callback?: StatsCb): void;
gauge(stat: string | string[], value: number, sampleRate?: number, timestamp?: Date, callback?: StatsCb): void;
gauge(stat: string | string[], value: number, timestamp?: Date, callback?: StatsCb): void;

Since I made the timestamp argument be an instance of Date here, it is possible to differentiate between it and sampleRate, but if it was a number instead there would be no way of knowing what to do with an invocation like gauge('foo', 1, 1685427264789) since it could be either a sampleRate or a timestamp. Tacking on additional optional positional arguments seems like it will eventually be impossible or at least very complex to support.

If you still think it's the way to go about it, I can do it in this case (I think), but what do you think about introducing an options object (or some better name if you can think of one) where these optional arguments can be provided as named arguments instead:

interface GaugeOptions {
  tags?: Tags;
  sampleRate?: number;
  timestamp?: Date;
}

gauge(stat: string | string[], value: number, sampleRate?: number, tags?: Tags, callback?: StatsCb): void;
gauge(stat: string | string[], value: number, tags?: Tags, callback?: StatsCb): void;
gauge(stat: string | string[], value: number, callback?: StatsCb): void;
gauge(stat: string | string[], value: number, sampleRate?: number, callback?: StatsCb): void;
gauge(stat: string | string[], value: number, options?: GaugeOptions, callback?: StatsCb): void;

Since tags is also an object it'll be a bit of a nightmare to differentiate between tags and options in a case like: gauge('foo', 1, { sampleRate: 5 }, but I don't see any way to make this backwards compatible otherwise. The least bad way I see is to essentially make the keys of GaugeOptions "reserved" tags, and interpret the object as GaugeOptions if one of them is present and otherwise interpret it as a Tags object. Not great, not terrible.

Another option could be to replace all the positional arguments with an object. This would be a bigger change for users, although it could still be made backwards compatible.

interface StatsFunctionProps {
  stat: string | string[];
  value: number;
  callback?: StatsCb;
}

interface GaugeProps extends StatsFunctionProps {
  sampleRate?: number;
  tags?: Tags;
  timestamp?: Date;
}

gauge(props: GaugeProps): void;
// These overrides would be for backwards compatibility
gauge(stat: string | string[], value: number, sampleRate?: number, tags?: Tags, callback?: StatsCb): void;
gauge(stat: string | string[], value: number, tags?: Tags, callback?: StatsCb): void;
gauge(stat: string | string[], value: number, callback?: StatsCb): void;
gauge(stat: string | string[], value: number, sampleRate?: number, callback?: StatsCb): void;

// Usage

gauge({
  stat: 'foo',
  value: 1,
  timestamp: new Date()
})

Either option would make it trivial to add additional extensions in the future, such as the container id that I mentioned earlier, without needing to write complex code to figure out which property an optional argument actually refers to based on its type and which other arguments have been passed. The benefit of the last approach is that there's no conflict between the options object and the tags object, reducing complexity and side-stepping the problem where a tag key conflicts with an option name. Note that although my examples here are just for the gauge type, the same would obviously apply to all stat functions.

[bdeitte]

Apologies for delay here, have generally not been available much these past two weeks, and thanks for such a detailed and thoughtful reply. I see your point now on how the general arguments-adding solution isn't going to work here. I have had a philosophy in this project of tilting heavily towards backwards-compatibility, and it can show in some of the ugliness of that param growth! I like the first option myself, of adding a new GaugeOptions in there. This looks like a great idea in general to clean up some of the options overgrowth. Happy to review anything related to this if you are still looking to do this. Thanks.

[Nevon]

Apologies for delay here, have generally not been available much these past two weeks

No worries whatsoever. I'm a maintainer of a reasonably popular open-source project myself, and know full well what it's like to have an inbox full of issues and pending PRs.

The only issue I have with the first option is how on earth to differentiate between GaugeOptions and Tags. Breaking backwards compatibility and making the only interface be:

gauge(stat: string | string[], value: number, options?: GaugeOptions, callback?: StatsCb): void;

Would "solve" this, but obviously it comes at a cost :(

Otherwise we'd have to do something like:

const gaugeOptionKeys = new Set<keyof GaugeOptions>(['sampleRate', 'tags', 'timestamp']);

function isGaugeOptions(maybeGaugeOptions: number | Tags | StatsCb | GaugeOptions | undefined): maybeGaugeOptions is GaugeOptions {
    return maybeGaugeOptions != null && typeof maybeGaugeOptions === 'object' && !Array.isArray(maybeGaugeOptions) && Object.keys(maybeGaugeOptions).some(key => key in gaugeOptionKeys);
}

// Followed by one heck of an if-pyramid to figure out what we're dealing with

The obvious downside, other than leading to some real nasty code, is that you can't ever have a tag with the same name as an option in GaugeOptions, or the code will just silently misbehave compared to your intent.

Let me know if you have an ideas or thoughts. I'm about to head off to vacation for a few weeks, but I'll be back after.

[bdeitte]

Well let me say here that I obviously was waiting for your vacation for a few weeks and it wasn't that I was super behind on emails in general again! 😄 Thanks for the understanding- I like to make sure this project continues to get some love even though I haven't been able to give it much attention lately.

And thanks again for a great message on this. I understand there is some downside with the backward compatibility, but I would really like to keep it. There are definitely some other cases like this currently in the code, in places where we can do some interesting checks to keep that backward compatibility (as I'm sure you've already seen in https://github.com/brightcove/hot-shots/blob/master/lib/statsd.js#L158 and elsewhere). This will add to that ugliness, for sure, but I would want to consider a more full param rethink at once if this was to be considered.