Skip to content

Pushing computing delta into timing module#171

Merged
gwicke merged 1 commit intomasterfrom
delta
Feb 13, 2015
Merged

Pushing computing delta into timing module#171
gwicke merged 1 commit intomasterfrom
delta

Conversation

@arlolra
Copy link
Member

@arlolra arlolra commented Feb 11, 2015

Is that desirable?

@earldouglas
Copy link
Contributor

Makes sense to me.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 84.02% when pulling 1f24bdf on delta into 9bc8050 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 84.02% when pulling 1f24bdf on delta into 9bc8050 on master.

@gwicke
Copy link
Member

gwicke commented Feb 12, 2015

I left it out as passing in the timing seemed slightly more flexible / less magical (can add up several timings to one aggregate etc). Don't care too strongly though.

@arlolra
Copy link
Member Author

arlolra commented Feb 12, 2015

The common case is tedious though,
https://gerrit.wikimedia.org/r/#/c/186219/7/api/routes.js

You can always pass in (Date.now() - aggregate) :)

@gwicke
Copy link
Member

gwicke commented Feb 12, 2015

Maybe the start time could be an optional parameter for the best of both worlds?

@arlolra
Copy link
Member Author

arlolra commented Feb 12, 2015

Maybe the start time could be an optional parameter for the best of both worlds?

The signature would start to get ugly. I don't like passing in a null for delta in that case.

I guess just adding a wrapper would suffice.

StatsD.prototype.endTimer = function( name, suffix, start ) {
  return this.timing( name, suffix, Date.now() - start );
}

Or maybe this has been enough of a distraction already :)

@gwicke
Copy link
Member

gwicke commented Feb 13, 2015

@arlolra, that would work for me. Perhaps call it endTiming instead for some consistency?

@arlolra
Copy link
Member Author

arlolra commented Feb 13, 2015

You've s/-/,/ :)

endTiming sounds good. Or, captureTiming? (suggested just to keep this conversation going)

@gwicke
Copy link
Member

gwicke commented Feb 13, 2015

(suggested just to keep this conversation going)

Ha! We'll move this code to a separate package so that we can share it. Can add the method then.

 * And remove a redundant call to makeName.
@arlolra
Copy link
Member Author

arlolra commented Feb 13, 2015

Oh ... too late. And I merged in #175.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 84.7% when pulling 0f81eab on delta into bec5db2 on master.

gwicke added a commit that referenced this pull request Feb 13, 2015
Pushing computing delta into timing module
@gwicke gwicke merged commit da05209 into master Feb 13, 2015
@gwicke
Copy link
Member

gwicke commented Feb 13, 2015

@arlolra: alright, thank you!

@arlolra arlolra deleted the delta branch February 13, 2015 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments