AutoTimer should assert on empty histogram key names

RESOLVED FIXED in Firefox 55

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: billm, Assigned: paavininanda, Mentored)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [measurement:client] [lang=c++])

Attachments

(3 attachments, 1 obsolete attachment)

As far as I know, we don't have any rule that prohibits empty histogram keys. However, if you pass an empty key to AutoTimer, it will act as if you want to accumulate to a non-keyed histogram. This just causes the Accumulate call to be ignored, along with an NS_WARNING.

I think we should either allow empty keys and make them work or assert against them. This patch allows them.
Attachment #8830102 - Flags: review?(chutten)
Comment on attachment 8830102 [details] [diff] [review]
make empty keys work

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

Is this a problem that actually comes up, or something that you just encountered during a code dive?

Regardless, yes, we do currently support empty keys for keyed histograms (It might be a mistake, but eh) so this is perfectly in keeping with existing behaviour.

AutoTimers aren't used so much that the addition of a sizeof(bool) will break the bank, will it? Compared to the size of the keystring, I should hope not.
Attachment #8830102 - Flags: review?(chutten) → review+
Empty histogram keys are not supported intentionally; if they work now they might not in the future.
If you have specific use-cases for them we should talk about what they are.
(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> Empty histogram keys are not supported intentionally; if they work now they
> might not in the future.
> If you have specific use-cases for them we should talk about what they are.

Well, it's not uncommon that we record a value where the key might be empty. In this case it's the name of a runnable. We can always convert it to something else ("<empty>" or something). I don't really see the point, but it's not a big deal.

Is it okay if I instead assert in AutoTimer that the key is non-empty?
Flags: needinfo?(gfritzsche)
I filed this because bug 1331804, which was passing an empty key to AutoTimer, resulted in bug 1333597.
(In reply to Bill McCloskey (:billm) from comment #3)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> > Empty histogram keys are not supported intentionally; if they work now they
> > might not in the future.
> > If you have specific use-cases for them we should talk about what they are.
> 
> Well, it's not uncommon that we record a value where the key might be empty.
> In this case it's the name of a runnable. We can always convert it to
> something else ("<empty>" or something). I don't really see the point, but
> it's not a big deal.

There is an internal artifact of the Histogram storage that makes this a bit harder to deal with.
More importantly, i don't think any of the upstream tooling (e.g. telemetry.mozilla.org dashboard) expect & handle this.
In any place where we should show a set of keys/labels for selection/consumption/..., empty keys will need special treatment.

> Is it okay if I instead assert in AutoTimer that the key is non-empty?

Sure, that sounds good, thanks.
Flags: needinfo?(gfritzsche)
Priority: -- → P2
Whiteboard: [measurement:client]
Bill, were you working on this?
Flags: needinfo?(wmccloskey)
Priority: P2 → P3
See Also: → 1334469
I never had time to write a patch for this. It should be a one-liner though.
Assignee: wmccloskey → nobody
Flags: needinfo?(wmccloskey)
- Instead of what attachment 8830102 [details] [diff] [review] does, assert in the constructor [1] that the key is not empty using MOZ_ASSERT().
- Firefox should still build successfully after.
- Tests should still run successfully: mach test toolkit/components/telemetry
- Try adding a use of AutoTimer somewhere with an empty key (e.g. at [2]).
- Running Firefox or the tests should now trigger the assert.

1: https://dxr.mozilla.org/mozilla-central/rev/6d38ad302429c98115c354d643e81987ecec5d3c/toolkit/components/telemetry/Telemetry.h#190
2: https://dxr.mozilla.org/mozilla-central/rev/6d38ad302429c98115c354d643e81987ecec5d3c/toolkit/components/telemetry/Telemetry.cpp#2359
Mentor: gfritzsche, alessio.placitelli
Whiteboard: [measurement:client] → [measurement:client] [lang=c++]
(Assignee)

Comment 9

2 years ago
(In reply to Georg Fritzsche [:gfritzsche] [at workweek] from comment #8)
> - Instead of what attachment 8830102 [details] [diff] [review] does, assert
> in the constructor [1] that the key is not empty using MOZ_ASSERT().

Should it not be 'The key is empty' in case its empty?


> - Try adding a use of AutoTimer somewhere with an empty key (e.g. at [2]).
> - Running Firefox or the tests should now trigger the assert.

And can you elaborate on how this can be done?
Flags: needinfo?(gfritzsche)
(In reply to Paavini Nanda from comment #9)
> (In reply to Georg Fritzsche [:gfritzsche] [at workweek] from comment #8)
> > - Instead of what attachment 8830102 [details] [diff] [review] does, assert
> > in the constructor [1] that the key is not empty using MOZ_ASSERT().
> 
> Should it not be 'The key is empty' in case its empty?

The key should not be empty, that was the result of the discussion here.
I'm updating the bug title to reflect that.

> > - Try adding a use of AutoTimer somewhere with an empty key (e.g. at [2]).
> > - Running Firefox or the tests should now trigger the assert.
> 
> And can you elaborate on how this can be done?

You can search through DXR to find usage examples in our code:
https://dxr.mozilla.org/mozilla-central/search?q=AutoTimer&redirect=false

You could just copy an existing use and use an empty key with it.
Assignee: nobody → paavininanda
Flags: needinfo?(gfritzsche)
Summary: AutoTimer should allow empty histogram key names → AutoTimer should assert on empty histogram key names
I have done that few hours ago and I have a patch for which I tested that:
1) Firefox builds and run successfully locally when the assertion is added
2) Tested that if I give an empty string to AutoTimer:
   - Basically all the tests fail since the assertion is triggered;
   - running the browser fail;

It was tested in a debug build.

But I see this bug was recently assigned...
Let me know if you want me to send the patch to review at some point.
(Assignee)

Comment 12

2 years ago
Posted patch bug1333624.patch (obsolete) — Splinter Review
(Assignee)

Updated

2 years ago
Attachment #8848475 - Flags: review?(gfritzsche)
(In reply to Federico Padua (fedepad) from comment #11)
> I have done that few hours ago and I have a patch for which I tested that:

Ah, bad timing!
In the future you can comment on the bug that you are working on it (or want to).
Comment on attachment 8848475 [details] [diff] [review]
bug1333624.patch

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

Thanks, this looks close but still needs some changes.

::: toolkit/components/telemetry/Telemetry.h
@@ +190,5 @@
>    explicit AutoTimer(const nsCString& aKey, TimeStamp aStart = TimeStamp::Now() MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
>      : start(aStart)
>      , key(aKey)
>    {
> +    if(aKey.IsEmpty()) {

Minor issue: Please add space between "if" and "(".

@@ +191,5 @@
>      : start(aStart)
>      , key(aKey)
>    {
> +    if(aKey.IsEmpty()) {
> +     MOZ_ASSERT(false,"The key is not empty");

Please add a space after the ",".
Also, the message doesn't seem correct.
How about: "The key should not be empty."
Attachment #8848475 - Flags: review?(gfritzsche) → feedback+
(Assignee)

Comment 15

2 years ago
Attachment #8848475 - Attachment is obsolete: true
Attachment #8849370 - Flags: review?(chutten)
Comment on attachment 8849370 [details] [diff] [review]
bug1333624.patch

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

::: toolkit/components/telemetry/Telemetry.h
@@ +191,5 @@
>      : start(aStart)
>      , key(aKey)
>    {
> +  	if (aKey.IsEmpty()) {
> +	     MOZ_ASSERT(false, "The key should not be empty");

This would be more concisely put as

MOZ_ASSERT(!aKey.IsEmpty(), "The key must not be empty");
Attachment #8849370 - Flags: review?(chutten) → review-
Sorry for the delay in review. In future, please set the review flag to ? and chose a reviewer like me or :gfritzsche so that we know to take a look promptly.
Flags: needinfo?(paavininanda)
Hi Paavini, are you still working on this?

Comment 20

2 years ago
Comment on attachment 8871250 [details] [diff] [review]
AutoTimer now asserts on empty histogram key names.

Fixes Paavini's patch, but left his authorship (no steal).
Best regards,
Paul
Attachment #8871250 - Flags: review?(gfritzsche)
Comment on attachment 8871250 [details] [diff] [review]
AutoTimer now asserts on empty histogram key names.

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

Thanks, this looks good!

I'll push this to try.
Attachment #8871250 - Flags: review?(gfritzsche) → review+
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ab12892409e030029d9a09ed082c11fd007855d

Setting needinfo for me to check back after the weekend on the results and land it.
Flags: needinfo?(gfritzsche)
This looks good on try, requesting check-in for the patch.
Flags: needinfo?(gfritzsche)
Keywords: checkin-needed

Comment 24

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a37df0fa9c2
AutoTimer now asserts on empty histogram key names. r=billm
Keywords: checkin-needed

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2a37df0fa9c2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(paavininanda)
You need to log in before you can comment on or make changes to this bug.