1333624 - AutoTimer should assert on empty histogram key names

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: make empty keys work

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.

Comment 1

[Chris H-C :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.

Comment 2

[Georg Fritzsche [:gfritzsche]]

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.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I filed this because bug 1331804, which was passing an empty key to AutoTimer, resulted in bug 1333597.

Comment 5

[Georg Fritzsche [:gfritzsche]]

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

Comment 6

[Georg Fritzsche [:gfritzsche]]

Bill, were you working on this?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I never had time to write a patch for this. It should be a one-liner though.

Comment 8

[Georg Fritzsche [:gfritzsche]]

- 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

Comment 9

[Paavini Nanda]

(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?

Comment 10

[Georg Fritzsche [: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.

Comment 11

[Federico Padua [:fedepad]]

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.

Comment 12

[Paavini Nanda]

Attachment: bug1333624.patch

Comment 13

[Georg Fritzsche [: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 14

[Georg Fritzsche [:gfritzsche]]

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."

Comment 15

[Paavini Nanda]

Attachment: bug1333624.patch

Comment 16

[Chris H-C :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");

Comment 17

[Chris H-C :chutten]

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.

Comment 18

[Georg Fritzsche [:gfritzsche]]

Hi Paavini, are you still working on this?

Comment 19

[Paul Bignier]

Attachment: AutoTimer now asserts on empty histogram key names.

Comment 20

[Paul Bignier]

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

Comment 21

[Georg Fritzsche [: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.

Comment 22

[Georg Fritzsche [:gfritzsche]]

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.

Comment 23

[Georg Fritzsche [:gfritzsche]]

This looks good on try, requesting check-in for the patch.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a37df0fa9c2
AutoTimer now asserts on empty histogram key names. r=billm

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a37df0fa9c2