Fix #6

[Serphentas]

This fixes the issue when one gives a timestamp but it gets truncated, hence leading to Graphite silently ignoring the metrics that are sent.

[felixge]

Thanks for submitting this.

However, even if a timestamp is provided, it gets transformed. This effectively create a wrong timestamp and as such Graphite silently ignores the metrics that are sent.

• How does truncating the provided timestamp to 1000ms precision create a "wrong" timestamp? IIRC graphite only supports 1 data point / second?
• Why would graphite silently ignore the incoming timestamp when it's truncated?

[Serphentas]

@felixge

To answer your questions:

Q1:

Truncating up to 1000ms=1s precision is indeed correct, since Graphite only supports sending metrics as fast as one per second. However, the "divide by 1000 and floor it" process is also executed if one gives a timestamp that is already correct.

Indeed, when no timestamp is given, a new one is created (timestamp = timestamp || Date.now()) and then truncated to obtain 1s precision (timestamp = Math.floor(timestamp / 1000)).

If I do provide one (i.e. Math.floor(Date.now() / 1000)), then its value is kept (timestamp = timestamp || Date.now()) but it still gets truncated right after (timestamp = Math.floor(timestamp / 1000)). This create a totally different timestamp.

I've added some debugging lines to GraphiteClient.js so now it looks like this:

GraphiteClient.prototype.write = function(metrics, timestamp, cb) {
  console.log('given timestamp: ' + (typeof timestamp === 'number' ? timestamp : 'none'));

  if (typeof timestamp === 'function') {
    cb        = timestamp;
    timestamp = null;
  }

  timestamp = timestamp || Date.now();
  timestamp = Math.floor(timestamp / 1000);

  console.log('new timestamp: ' + timestamp);

  this._carbon.write(GraphiteClient.flatten(metrics), timestamp, cb);
};

I then run specific tests in Node:

> graphite = require('./GraphiteClient.js');
{ [Function: GraphiteClient] createClient: [Function], flatten: [Function] }
> client = graphite.createClient('plaintext://10.233.69.160:2003/');
GraphiteClient {
  _carbon: CarbonClient { _dsn: 'plaintext://10.233.69.160:2003/', _socket: null } }
> metrics1 = {'foo.one': 100}
{ 'foo.one': 100 }
> metrics2 = {'foo.two': 200}
{ 'foo.two': 200 }
> client.write(metrics1, function(err){ if(err != null){ console.log(err); }});
given timestamp: none
new timestamp: 1515236738
undefined
> client.write(metrics2, Math.floor(Date.now() / 1000), function(err){ if(err != null){ console.log(err); }});
given timestamp: 1515236753
new timestamp: 1515236
undefined
> 

As you can see, the first write works just fine as the current time is sent as timestamp. In the second case though, despite the timestamp being correct, it gets transformed to 1515236 which is Sun Jan 18 12:53:56 1970 UTC.

EDIT: I forgot to add these results from Graphite:

asd@asd ~ $ curl "http://10.233.69.160/render?target=foo.one&format=raw&from=-5min"
foo.one,1515236520,1515236820,60|None,None,None,100.0,None
asd@asd ~ $ curl "http://10.233.69.160/render?target=foo.two&format=raw&from=-5min"
foo.two,1515236520,1515236820,60|None,None,None,None,None

Perhaps the problem here is that it is not stated what type of timestamp one must provide. That is, should one send Date.now() or Math.floor(Date.now() / 1000) ? Once we settle upon a basis, we can define how to properly handle timestamp in the write function.

Q2:

I think it will ignore metrics if it goes outside its storage scope.

Say you have the following in storage-schemas.conf:

[test]
pattern = ^test\.
retention = 5s:5m

You can store up to 5 minutes worth of data, with step of 5 seconds between each sample point.

If you happen to give a metric that matches this configuration and whose timestamp is 15 minutes in the past, then Graphite will not be able to store it. Hence, if the write function "fails you" during timestamp handling, then your code will work just fine (the plaintext socket will properly write it with no errors) but your metrics won't be saved at all.

[felixge]

Perhaps the problem here is that it is not stated what type of timestamp one must provide. That is, should one send Date.now() or Math.floor(Date.now() / 1000) ? Once we settle upon a basis, we can define how to properly handle timestamp in the write function.

Yeah, I think this problem is caused by a lack of documentation. write() currently expects to be given a timestamp expressed as millseconds since unix time started, since that's the idiomatic time format in JavaScript.

So your example should be:

client.write(metrics2, Math.floor(Date.now() / 1000) * 1000, ...)

Of course in the case where you use Date.now() it'd be simpler to simply leave out the timestamp argument.

Anyway, let me know what you think.

[Serphentas]

As you say, since the default is to use Date.now(), which has millisecond precision, it would be wiser to ask users to provide such a timestamp. It also make calls shorter (no need to write Math.floor(Date.now() / 1000).

So in the end, I think we should settle on this as the default timestamp format. It should also be made clear in the docs.

Do you agree with this way of doing things ? If yes I will update this PR with new commits.

[felixge]

@Serphentas yes, I agree. I'd love it if you send the commits for that.

[Serphentas]

I think it's good now. I've also resolved conflicts coming from tagging support.

[felixge]

LGTM, thanks!

[Serphentas]

I've created a base release 0.1.0 for the last PR (tags) and made a new patch release for this one, since it doesn't add new features but rather fixes some ambiguity regarding usage and changes the way timestamp are processed.

As timestamps had to be Date.now() before, this shouldn't break compatibility with existing codebases.

[felixge]

SGTM