1450098 - Rewrite TelemetryScalars' internal_LogScalarError to use nsPrintfCString

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Chris H-C :chutten]

internal_LogScalarError does some boring string construction that might be easier to maintain with if we use the built-in nsPrintfCString

Comment 1

[Alessio Placitelli [:Dexter]]

Make it mentored!

Comment 2

[Alessio Placitelli [:Dexter]]

In [1] we convert error enums to string messages that are printed to the browser console. However, some constants are hardcoded in the text messages adding friction to future changes.

We need to change these locations [2] to use |nsPrintfCString| to print the relevant constants instead.

- |kMaximumStringValueLength| for the maximum string length;
- |kMaximumKeyStringLength| for the key length;
- |kMaximumNumberOfKeys| for the maximum number of keys;

Please note that |nsPrintfCString| returns a narrow string, while |errorMessage| is a wide string. We might need to use |AppendUTF8toUTF16| to concatenate the output of nsPrintfCstring.

To test that this works, we should:

> ./mach test toolkit/components/telemetry/test/unit

And then also manually validate the output by:

- ./mach run, to open Firefox
- go to about:telemetry page
- open the web developer tools
- open the browser console
- issue the |Services.telemetry.scalarSet("telemetry.test.string_kind", "aaaaaaaaaaaaaaaaaaaaaaaaaaasssdasdasdasdadasdasdaasdasd")| and check that the error message is correctly printed to the browser console

A similar process can be used to test the other cases.

[1] - https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/toolkit/components/telemetry/TelemetryScalar.cpp#888-941
[2] - https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/toolkit/components/telemetry/TelemetryScalar.cpp#919,925,928

Comment 3

[Michael Calabrese]

Hello Alessio,

I'm looking at making this my first open source commit, but I'm having some trouble with the unit tests. 34 out of 35 pass, with the exception being test_TelemetryEnvironment.js. I receive a lot of information related to the test, but I'm not quite sure what to post to receive help.

The tests started with a warning that the MOZ_NODE_PATH environment variable is not set, so tests requiring http/2 will fail. I don't know if that's the cause, but I also don't know how to fix it to find out.

Services.telemetry.scalarSet("telemetry.test.string_kind", "aaaaaaaaaaaaaaaaaaaaaaaaaaasssdasdasdasdadasdasdaasdasd") comes back as undefined on the developer console, which means I didn't edit the code right I assume. What message should I expect to receive from that test so I may know if I did it right on my next attempt?

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to mcalabrese85 from comment #3)
> Hello Alessio,
> 
> I'm looking at making this my first open source commit, but I'm having some
> trouble with the unit tests. 34 out of 35 pass, with the exception being
> test_TelemetryEnvironment.js. I receive a lot of information related to the
> test, but I'm not quite sure what to post to receive help.

Thank you for your contribution... and welcome! That's the right place to ask for help. If you want some real-time help, you're also encouraged to join the #introduction IRC channel on irc.mozilla.org!

> The tests started with a warning that the MOZ_NODE_PATH environment variable
> is not set, so tests requiring http/2 will fail. I don't know if that's the
> cause, but I also don't know how to fix it to find out.

That looks unrelated. You should look for "unexpected" or "exceptions" keywords. Usually, if you're running the test on Linux, the error line will be highlighted in red.

That's probably close to the end of the test log. I would start looking there for the offending log line. When you find it, feel free to attach the relevant log section to the bug. If you have troubles finding the problem, please attach both your patch and the full test log.

> Services.telemetry.scalarSet("telemetry.test.string_kind",
> "aaaaaaaaaaaaaaaaaaaaaaaaaaasssdasdasdasdadasdasdaasdasd") comes back as
> undefined on the developer console, which means I didn't edit the code right
> I assume. What message should I expect to receive from that test so I may
> know if I did it right on my next attempt?

You're expected to see "Truncating scalar value to 50 characters" :)

Please let me know if you need further help!

Comment 5

[Michael Calabrese]

> Thank you for your contribution... and welcome! That's the right place to
> ask for help. If you want some real-time help, you're also encouraged to
> join the #introduction IRC channel on irc.mozilla.org!

Thank you for pointing me towards them, they were able to help with the issue. All automated tests now pass. It turned out that my DNS provider was returning a valid address when the test was expecting it to come back as invalid. I got to file my first (hand-held and hopefully good) bug report because of that. :)

> You're expected to see "Truncating scalar value to 50 characters" :)

I expected that to be the case. Unfortunately I was also unable to produce the expected results with a downloaded version of Firefox, nor with a clean build and downloaded version on a separate computer. I still receive "undefined" as a response.

I'm basically copying the command you wrote out above. I tried to use the string from test_TelemetryScalars.js also ("browser.qaxfiuosnzmhlg.rpvxicawolhtvmbkpnludhedobxvkjwqyeyvmv"), but the same result occurred. I know that the command is recognized because it will the console will mention that I do not have the correct number of arguments. I tried to see if there was anything else that may need to be done before or after entering that command, but as best I can tell it should work as a stand-alone one.

Any help with this would be appreciated. I plan on playing around with it some more, but mach suddenly decided to stop working so I may spend the rest of the night fixing that.

Comment 6

[Michael Calabrese]

I also forgot to mention that I tried to test the 100 key limit using the test_keyed_max_keys function to guide me. The following lines returned undefined, both when sent at the same time and sent individually:

Services.telemetry.keyedScalarAdd(KEYED_UINT_SCALAR, LAST_KEY_NAME, 10);
Services.telemetry.keyedScalarSet(KEYED_UINT_SCALAR, LAST_KEY_NAME, 1);
Services.telemetry.keyedScalarSetMaximum(KEYED_UINT_SCALAR, LAST_KEY_NAME, 10);

I can see that keyNamesSet has 100 keys in it by using a foreach loop to log them to the console. I cannot tell if each scalar got added correctly from keyNamesSet, though, as I do not see a way to check what is there.

Comment 7

[Alessio Placitelli [:Dexter]]

(In reply to mcalabrese85 from comment #5)
> > Thank you for your contribution... and welcome! That's the right place to
> > ask for help. If you want some real-time help, you're also encouraged to
> > join the #introduction IRC channel on irc.mozilla.org!
> 
> Thank you for pointing me towards them, they were able to help with the
> issue. All automated tests now pass. It turned out that my DNS provider was
> returning a valid address when the test was expecting it to come back as
> invalid. I got to file my first (hand-held and hopefully good) bug report
> because of that. :)

Nice! Do you have a bug number for that?

> > You're expected to see "Truncating scalar value to 50 characters" :)
> 
> I expected that to be the case. Unfortunately I was also unable to produce
> the expected results with a downloaded version of Firefox, nor with a clean
> build and downloaded version on a separate computer. I still receive
> "undefined" as a response.

I think I understand where the confusion comes from! You should look for the message in the browser console, you're probably looking at the console from the devtools, which is different :) See https://developer.mozilla.org/en-US/docs/Tools/Browser_Console#Opening_the_Browser_Console

I double checked and the message is printed correctly with the vanilla Firefox Release/Nightly/Beta.

Comment 8

[Michael Calabrese]

Awesome, thank you again for your help. I was indeed using the web console instead of the browser one. 

I can confirm that manually doing the tests results in the expected error messages. I did notice a discrepancy, though. In the original hard-coded message the error message is "The key length must be limited to 70 characters". However, kMaximumKeyStringLength is set to 72, which changes the value seen in the message.

The bug number for the one I filed is bug 1453832.

What would be the next step now that I have a solution?

Comment 9

[Michael Calabrese]

I think I did the commit correctly, and hopefully referenced you in it right, too. Please let me know either way.

Comment 10

[Michael Calabrese]

Attachment: Bug 1450098 - Rewrote part of TelemetryScalars' internal_LogScalarError to use nsPrintfCString and defined variables instead of constants;

Review commit: https://reviewboard.mozilla.org/r/236784/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236784/

Comment 11

[Alessio Placitelli [:Dexter]]

Comment on attachment 8968108 [details]
Bug 1450098 - Rewrote part of TelemetryScalars' internal_LogScalarError to use nsPrintfCString and defined variables instead of constants;

https://reviewboard.mozilla.org/r/236784/#review242520

Hi! Thanks for your patch, the commit is right. It's not an r+ yet as there are a few style problems. You're right about the discrepancy :) Things tend to get out of sync when using hardcoded stuff instead of constants ;) 72 is correct!

::: toolkit/components/telemetry/TelemetryScalar.cpp:931
(Diff revision 1)
>        break;
>      case ScalarResult::InvalidValue:
>        errorMessage.AppendLiteral(u" - Attempted to set the scalar to an incompatible value.");
>        break;
>      case ScalarResult::StringTooLong:
> -      errorMessage.AppendLiteral(u" - Truncating scalar value to 50 characters.");
> +			AppendUTF8toUTF16(nsPrintfCString(" - Truncating scalar value to %i characters.", kMaximumStringValueLength), errorMessage);

Please use spaces instead of tabs, as per our [Mozilla Code Style guide](https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Naming_and_Formatting_code).

::: toolkit/components/telemetry/TelemetryScalar.cpp:937
(Diff revision 1)
>        break;
>      case ScalarResult::KeyIsEmpty:
>        errorMessage.AppendLiteral(u" - The key must not be empty.");
>        break;
>      case ScalarResult::KeyTooLong:
> -      errorMessage.AppendLiteral(u" - The key length must be limited to 72 characters.");
> +			AppendUTF8toUTF16(nsPrintfCString(" - The key length must be limited to %i characters.", kMaximumKeyStringLength), errorMessage);

Please use spaces.

::: toolkit/components/telemetry/TelemetryScalar.cpp:940
(Diff revision 1)
>        break;
>      case ScalarResult::KeyTooLong:
> -      errorMessage.AppendLiteral(u" - The key length must be limited to 72 characters.");
> +			AppendUTF8toUTF16(nsPrintfCString(" - The key length must be limited to %i characters.", kMaximumKeyStringLength), errorMessage);
>        break;
>      case ScalarResult::TooManyKeys:
> -      errorMessage.AppendLiteral(u" - Keyed scalars cannot have more than 100 keys.");
> +			AppendUTF8toUTF16(nsPrintfCString(" - Keyed scalars cannot have more than %i keys.", kMaximumNumberOfKeys), errorMessage);

Please use spaces

Comment 12

[Michael Calabrese]

Attachment: Bug 1450098 - Rewrote part of TelemetryScalars' internal_LogScalarError to use nsPrintfCString and defined variables instead of constants;

Review commit: https://reviewboard.mozilla.org/r/236996/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236996/

Comment 13

[Michael Calabrese]

Whoops, sorry about that. Forgot to change the settings in my IDE. It should be good now and has been resubmitted.

Comment 14

[Alessio Placitelli [:Dexter]]

Comment on attachment 8968330 [details]
Bug 1450098 - Rewrote part of TelemetryScalars' internal_LogScalarError to use nsPrintfCString and defined variables instead of constants;

https://reviewboard.mozilla.org/r/236996/#review242930

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b0b1c38b8ba
Rewrote part of TelemetryScalars' internal_LogScalarError to use nsPrintfCString and defined variables instead of constants; r=Dexter

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9b0b1c38b8ba

Comment 17

[Michael Calabrese]

Hey Alessio,

Thank you for taking the time to mentor me throughout this process. It gave me a lot more confidence in trying to contribute to this and other projects.

Comment 18

[Alessio Placitelli [:Dexter]]

(In reply to Michael Calabrese from comment #17)
> Hey Alessio,
> 
> Thank you for taking the time to mentor me throughout this process. It gave
> me a lot more confidence in trying to contribute to this and other projects.

You're welcome! Would you be interested in working on another bug? If so, do you have a language preference?

Comment 19

[Michael Calabrese]

I would like to. I prefer C++, but can work with Java, too. If the other languages you use involve bugs that aren't too complex I can work on those, though I would need extra time to go through the language some first.