yaorg / node-measured

A Node metrics library for measuring and reporting application-level metrics, inspired by Coda Hale, Yammer Inc's Dropwizard Metrics Libraries
https://yaorg.github.io/node-measured/
MIT License
517 stars 52 forks source link

High resolution timer in Stopwatch #5

Closed seriousManual closed 10 years ago

seriousManual commented 12 years ago

It would be a quite nice feature to use process.hrtime() to have a even more precise time.

Than it would be:

{  
  histogram:
  { min: 0.45434,
  ...
  }
}

instead of:

{  
  histogram:
  { min: 0,
  ...
  }
}

Unfortunately sinon does not support the mocking of process.hrtime, so all tests would fail...

felixge commented 12 years ago

Unfortunately sinon does not support the mocking of process.hrtime, so all tests would fail...

Well, then let's change that. Change the Stopwatch constructor like this:

function Stopwatch(options) {
  options = options || {};

  EventEmitter.call(this);

  this._getTime = options.getTime || process.hrtime || Date.now;
  this._start = this._getTime();
  this._ended = false;
}

This way you can easily pass in a fake getTime() function for testing.

Let me know if you can make the patch for this!

seriousManual commented 12 years ago

Ah, I did not think about that solution, I will take care about it!

seriousManual commented 12 years ago

hi, due to the nature of process.hrtime i had to solve the measurement of the elapsed time a bit differently... (process.hrtime does not provide an absolute time value but a relative one)

i made two different solutions: https://github.com/zaphod1984/node-measured/blob/highResolution/lib/util/Stopwatch.js https://github.com/zaphod1984/node-measured/blob/highResolutionAlternative/lib/util/Stopwatch.js

second version should be a little more efficient as getTime doesn't have to be assigned on every invocation.

to me both variations feel a little clumsy though they're working fine, i wouldn't mind if you don't like them. :) if you do I'll gladly will do a pull request.

felixge commented 12 years ago

Hm, seems overkill. I'm also not a fan of the way you use the ternary operator. What about my initial suggestion, but except for process.hrtime use hrtime() which is then defined as an inline function at the bottom of the module like this:

function hrtime() {
  var hrtime = process.hrtime();
  return hrtime[0] / 1000 + hrtime[1] / (1000 * 1000);
}

Something like that? Should be simpler.

felixge commented 12 years ago

Nevermind, this should be done:

Stopwatch.prototype._getTime = function() {
  if (!process.hrtime) return Date.now();

  var hrtime = process.hrtime();
  return hrtime[0] / 1000 + hrtime[1] / (1000 * 1000);
}

And in the constructor:

function Stopwatch(options) {
  options = options || {};

  EventEmitter.call(this);

  if (options.getTime) this._getTime = options.getTime;
  this._start = this._getTime();
  this._ended = false;
}
seriousManual commented 12 years ago

Uh, I obviously made it too complicated... I did a pull request, I hope it's fine now... :) https://github.com/felixge/node-measured/pull/6