Closed Bug 682869 Opened 9 years ago Closed 8 years ago

add Telemetry::AccumulateTimeDelta and use it

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(2 files, 3 obsolete files)

There are a number of places where telemetry probes are used to accumulate millisecond intervals.  We should have a separate function to make it obvious what's going on.
Here's a patch to add the function and update appropriate callers.  I was unsure of whether to do the two things as separate patches, or one big ball of mud.  I'm sure somebody will straighten me out.

I'm also not sure who to pester to look at tweaks like this.
Comment on attachment 556583 [details] [diff] [review]
patch to add AccumulateTimeDelta and update callsites

Taras, can you review the telemetry parts and/or point me at who else should be reviewing the rest?
Attachment #556583 - Flags: review?(tglek)
Comment on attachment 556583 [details] [diff] [review]
patch to add AccumulateTimeDelta and update callsites

Next time please submit api change + code updates in 2 separate patches. As we discussed on irc, instead of passing TimeStamp::Now() use default parameter value or method overloading that implies Now().
Attachment #556583 - Flags: review?(tglek) → review-
Attached patch introduce AccumulateTimeDelta (obsolete) — Splinter Review
Attachment #565216 - Flags: review?(tglek)
Attachment #556583 - Attachment is obsolete: true
Comment on attachment 565216 [details] [diff] [review]
introduce AccumulateTimeDelta

>+void AccumulateTimeDelta(ID id, TimeStamp start, TimeStamp end=TimeStamp::Now());

Add spaces around =, r+ with that
Attachment #565216 - Flags: review?(tglek) → review+
Comment on attachment 565217 [details] [diff] [review]
update places to use AccumulateTimeDelta

Flagging people for review on the use-it-everywhere patch.
Attachment #565217 - Flags: review?(tglek)
Attachment #565217 - Flags: review?(khuey)
Attachment #565217 - Flags: review?(jduell.mcbugs)
Attachment #565217 - Flags: review?(tglek) → review+
Comment on attachment 565217 [details] [diff] [review]
update places to use AccumulateTimeDelta

Review of attachment 565217 [details] [diff] [review]:
-----------------------------------------------------------------

Good stuff.  Thanks Nathan.
Attachment #565217 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 565217 [details] [diff] [review]
update places to use AccumulateTimeDelta

Review of attachment 565217 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable enough to me.
Attachment #565217 - Flags: review?(khuey) → review+
Updated with taras's requested spaces and real commit info.  Carrying over r+.
Attachment #565216 - Attachment is obsolete: true
Attachment #565966 - Flags: review+
Updated patch with r= info and such.  Carrying over r+.
Attachment #565217 - Attachment is obsolete: true
Attachment #565967 - Flags: review+
Think everything's GTG for checkin-needed.  Flagging.
Keywords: checkin-needed
Assignee: nobody → nfroyd
You need to log in before you can comment on or make changes to this bug.