add Telemetry::AccumulateTimeDelta and use it

RESOLVED FIXED in mozilla10

Status

()

Toolkit
Telemetry
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 556583 [details] [diff] [review]
patch to add AccumulateTimeDelta and update callsites

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.
(Assignee)

Comment 2

7 years ago
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 3

6 years ago
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-
(Assignee)

Comment 4

6 years ago
Created attachment 565216 [details] [diff] [review]
introduce AccumulateTimeDelta
Attachment #565216 - Flags: review?(tglek)
(Assignee)

Comment 5

6 years ago
Created attachment 565217 [details] [diff] [review]
update places to use AccumulateTimeDelta
(Assignee)

Updated

6 years ago
Attachment #556583 - Attachment is obsolete: true

Comment 6

6 years ago
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+
(Assignee)

Comment 7

6 years ago
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)

Updated

6 years ago
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+
(Assignee)

Comment 10

6 years ago
Created attachment 565966 [details] [diff] [review]
introduce Telemetry::AccumulateTimeDelta

Updated with taras's requested spaces and real commit info.  Carrying over r+.
Attachment #565216 - Attachment is obsolete: true
Attachment #565966 - Flags: review+
(Assignee)

Comment 11

6 years ago
Created attachment 565967 [details] [diff] [review]
update places to use AccumulateTimeDelta

Updated patch with r= info and such.  Carrying over r+.
Attachment #565217 - Attachment is obsolete: true
Attachment #565967 - Flags: review+
(Assignee)

Comment 12

6 years ago
Think everything's GTG for checkin-needed.  Flagging.
Keywords: checkin-needed

Updated

6 years ago
Assignee: nobody → nfroyd
https://hg.mozilla.org/mozilla-central/rev/d6b9a54956c0
https://hg.mozilla.org/mozilla-central/rev/e4edd20737fb
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.