1513496 - Stop logging warning when scalar is expired.

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Mark Banner (:standard8)]

STR:

1) Run a test in debug mode that includes getting keyed scalars, e.g. browser/modules/test/browser/browser_BrowserErrorReporter.js or browser/components/search/test/browser/browser_searchTelemetry.js
2) Look through the output

Actual Results

Lots of messages like:

[Parent 13597, Main Thread] WARNING: NS_FAILED internal_GetScalarByEnum for CHILD: file /Users/mark/dev/gecko/toolkit/components/telemetry/core/TelemetryScalar.cpp, line 2161

Expected Results

No messages

I found this originally with browser_searchTelemetry.js but seeing as this happens in other tests, I'm assuming it isn't something I'm doing wrong.

Comment 1

[Chris H-C :chutten]

(( First off, the keyed scalar failure message is wrong and ambiguous. I've filed bug 1513546 to fix that ))

This comes from the parent side of the IPC channel where we try to take all the accumulations that happened in the child and apply them.

Given that we're not asserting, the ID is valid[1], we aren't asking for a keyed string scalar (unsupported)[2]... we're unlikely to have failed in the KeyedScalar ctor[3], so that's not likely.

That leaves the case of accumulations in the child-process to expired scalars [4]. Which isn't really that big of a deal. Maybe we shouldn't warn on that?

[1]: https://searchfox.org/mozilla-central/rev/adcc169dcf58c2e45ba65c4ed5661d666fc3ac74/toolkit/components/telemetry/core/TelemetryScalar.cpp#1712
[2]: https://searchfox.org/mozilla-central/rev/adcc169dcf58c2e45ba65c4ed5661d666fc3ac74/toolkit/components/telemetry/core/TelemetryScalar.cpp#1756
[3]: https://searchfox.org/mozilla-central/rev/adcc169dcf58c2e45ba65c4ed5661d666fc3ac74/toolkit/components/telemetry/core/TelemetryScalar.cpp#1762
[4]: https://searchfox.org/mozilla-central/rev/adcc169dcf58c2e45ba65c4ed5661d666fc3ac74/toolkit/components/telemetry/core/TelemetryScalar.cpp#1751

Comment 2

[Alessio Placitelli [:Dexter]]

Hey Chris, do you think we should remove the waning there? If so, what do you think about making this mentored?

Comment 3

[Chris H-C :chutten]

Even better would be to both do that _and_ remove the expired scalars. But we can tackle the first part first:

To help Mozilla out with this bug, here's the steps:

0) Comment here on the bug that you want to volunteer to help. I (or someone else) will assign it to you.
1) Download and build the Firefox source code: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
- If you have any problems, please ask on IRC (https://wiki.mozilla.org/Irc) in the #introduction channel. They're there to help you get started.
- You can also read the Developer Guide, which has answers to most development questions: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
2) Start working on this bug. You'll be working in the TelemetrScalar.cpp file, making sure the warning here [1] doesn't fire when the nsresult is NS_ERROR_NOT_AVAILABLE (due to the scalar being expired).
- If you have any problems with this bug, please comment on this bug and set the needinfo flag for me. Also, you can find me and my teammates on the #telemetry channel on IRC (https://wiki.mozilla.org/Irc) most hours of most days. Be sure to mention this bug number so we can see the context.
3) Build your change with `mach build` and test your change with `mach test toolkit/components/telemetry/tests/`. Also check your changes for adherence to our style guidelines by using `mach lint`
4) Submit the patch for review. Mark me as a reviewer so I'll get an email to come look at your code.
- Here's the guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
5) After a series of reviews and changes to your patch, I'll ask Phabricator to push it to autoland. Your code will soon be shipping to Firefox users worldwide!
6) ...now you get to think about what kind of bug you'd like to work on next. Let me know what you're interested in and I can help you find your next contribution.

[1]: https://searchfox.org/mozilla-central/rev/adcc169dcf58c2e45ba65c4ed5661d666fc3ac74/toolkit/components/telemetry/core/TelemetryScalar.cpp#2274

Comment 4

[Kaio Augusto de Camargo]

Hi! I'd love to help with this bug if that's alright!

Comment 5

[Chris H-C :chutten]

That's perfectly alright, let me assign it to you. Let me know if you have any questions.

Comment 7

[Kaio Augusto de Camargo]

Attachment: Bug 1513496 - Stop logging failure warnings if the scalar is expired. r=chutten

Currently, when operating with scalars, if a call to internal_GetScalarByEnum
(or its keyed variant) return an error, then a warning will be logged. If one of
the requested scalars is expired, this could lead to an unwated flood of logs.
With this change, the return of the function is checked, and if it is
NS_ERROR_NOT_AVAILABLE (i.e. expired scalar), then no warning is issued.

Comment 8

[Chris H-C :chutten]

I have queued this to land. Thank you for your contribution! Do you have an idea for what you'd like your next contribution to be?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d352b4fe9d68
Stop logging failure warnings if the scalar is expired. r=chutten

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/d352b4fe9d68

Comment 11

[Kaio Augusto de Camargo]

I'm glad my little change has been approved, even if it took me a while to get it right... :)

As for my next contribution, I honestly have no idea. I feel a little more confident in myself now that I can at least build things, attach a debugger and use some (really) basic commands on Mach (e.g. run a test), but I'm still a bit unsure where I can be of any help.

Comment 12

[Chris H-C :chutten]

Well, what sort of things might you be interested in? We're a little low on C++ mentored bugs in our component, but I'm betting other components would have some. (here's a list of all unassigned "good second bugs" and "good next bugs")

From our component a good next bug might be bug 1168728 about file operations in JS, or bug 1515009 about tidying up the code that powers about:telemetry.

Comment 13

[Kaio Augusto de Camargo]

As much as I love C++, I think I'd prefer to stick with the Telemetry component for now even if it means writing JS or something else. Particularly, bug 1168728 sounds like a fun candidate for a second bug (although seeing it has been open for 4 years is a bit scary). Should I just comment on it and offer my help?

Comment 14

[Chris H-C :chutten]

Definitely comment. Alessio's on my team and he's a great mentor.