1683171 - Expose test_get_num_recorded_errors in FOG metrics' JS and C++ APIs

Status: RESOLVED FIXED

(Toolkit :: Telemetry, task, P2)

Description

[Chris H-C :chutten]

The current iteration of test APIs from the SDK include test_get_num_recorded_errors.

We haven't exposed these APIs in JS and C++ and don't yet have a clever way to do so in a way that keeps them in sync with glean::ErrorType.

We should figure one out.

Comment 1

[Chris H-C :chutten]

Pending the result of proposal test_get_value should assert num_errors == 0, which is collecting feedback until May 19.

Comment 2

[Chris H-C :chutten]

Requesting feedback on the proposal from Bea for Glean.js, Jan-Erik for Glean SDK, and Raphael for QA.

Comment 3

[Beatriz Rizental [:brizental]]

Just gave the proposal a read, left my comments in the "guestbook" section :)

Comment 4

[Chris H-C :chutten]

Attachment: Proposal: test_get_value should assert num_errors == 0

Comment 5

[Jan-Erik Rediger [:janerik]]

🚢

Comment 6

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/glean/pull/1651

Comment 7

[Raphael Aurich [:raphael] UTC+02:00]

I apologize for the delay in responding to the ni request. I added comments and general feedback to the Google Doc just now.

Comment 8

[Chris H-C :chutten]

Thank you everyone for your feedback. The proposal is SUSPENDED pending uniffi integration. The callback approach for getting language- or test-suite-specific asserts/panics/etc was technically infeasible (asserts don't necessarily cross two layers of FFI intact, notably in Python but expected in other languages too) which left us with handling this individually for each metric type in each language binding which... seems unpleasant.

Without the proposal, this is now back to "Handle test_get_num_recorded_errors in FOG" which needs a micro-design. Let's try:

Rust:

• test_get_num_recorded_errors is already present and happy on the Metric traits as implemented by FOG, so let's not mess with anything there (for now)

C++:

• We already mux test_has_value and test_get_value together into a mozilla::Maybe optional return type for TestGetValue. Let's cram more into these return types and mux in some test_get_num_recorded_errors into this by making it a mozilla::Result<mozilla::Maybe<T>, E>.

JS:

• In the event we have errors we can return a non-NS_OK value (I'm looking at NS_ERROR_LOSS_OF_SIGNIFICANT_DATA) from testGetValue which will raise an exception in JS. I don't think we can add a custom string message, but it's something I can look into. All else fails, LogToBrowserConsole is right over there.

Comment 9

[Chris H-C :chutten]

Attachment: Bug 1683171 - Tidy up some header includes r?janerik!

Comment 10

[Chris H-C :chutten]

Attachment: Bug 1683171 - JS/C++ FOG test APIs will now fail if there are errors r?janerik!

Depends on D117922

Comment 11

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/glean/pull/1698

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2782440566b7
Tidy up some header includes r=janerik
https://hg.mozilla.org/integration/autoland/rev/401623bf09ce
JS/C++ FOG test APIs will now fail if there are errors r=janerik

Comment 13

[Atila Butkovits]

Backed out for causing Python3 failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/1b890e7f8483eef9261c7069f6fee459a86d7411

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=L1fRuyg4TE-ay80JZL_QjQ.0&revision=401623bf09ce3d1fead1b1214eb36f9ea1e219dd&searchStr=linux%2C18.04%2Cx64%2Copt%2Cpython%2C3%2Cunit%2Ctests%2Csource-test-python-fog-linux1804-64%2Fopt-py3%2Cfp

Failure log: https://treeherder.mozilla.org/logviewer?job_id=343410799&repo=autoland&lineNumber=85
https://treeherder.mozilla.org/logviewer?job_id=343410749&repo=autoland&lineNumber=30271
https://treeherder.mozilla.org/logviewer?job_id=343410752&repo=autoland&lineNumber=7139

Comment 14

[Chris H-C :chutten]

Sorry about that. Fixed that up.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e14f3d135e1
Tidy up some header includes r=janerik
https://hg.mozilla.org/integration/autoland/rev/50d55b80e7a1
JS/C++ FOG test APIs will now fail if there are errors r=janerik

Comment 16

[Natalia Csoregi [:nataliaCs]]

Backed out for causing bustage on Counter.cpp

• backout: https://hg.mozilla.org/integration/autoland/rev/eb6840adae8ca3eb79ff03a9ad92a87698e87ea3

• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=50d55b80e7a18bf1da959286744d0b9ed18c3963

• failure log: https://treeherder.mozilla.org/logviewer?job_id=343495486&repo=autoland&lineNumber=30649

[task 2021-06-22T16:51:27.906Z] 16:51:27     INFO -  In file included from Unified_cpp_components_glean0.cpp:47:
[task 2021-06-22T16:51:27.906Z] 16:51:27    ERROR -  /builds/worker/checkouts/gecko/toolkit/components/glean/bindings/private/Counter.cpp:41:10: error: no viable conversion from returned value of type 'mozilla::Nothing' to function return type 'Result<Maybe<int32_t>, nsCString>' (aka 'Result<Maybe<int>, nsTString<char>>')
[task 2021-06-22T16:51:27.906Z] 16:51:27     INFO -    return Nothing();
[task 2021-06-22T16:51:27.906Z] 16:51:27     INFO -           ^~~~~~~~~
[task 2021-06-22T16:51:27.906Z] 16:51:27     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Result.h:458:26: note: candidate constructor not viable: no known conversion from 'mozilla::Nothing' to 'mozilla::Maybe<int> &&' for 1st argument
[task 2021-06-22T16:51:27.906Z] 16:51:27     INFO -    MOZ_IMPLICIT constexpr Result(V&& aValue) : mImpl(std::forward<V>(aValue)) {
[task 2021-06-22T16:51:27.906Z] 16:51:27     INFO -                           ^
[task 2021-06-22T16:51:27.906Z] 16:51:27     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Result.h:463:26: note: candidate constructor not viable: no known conversion from 'mozilla::Nothing' to 'const mozilla::Maybe<int> &' for 1st argument
[task 2021-06-22T16:51:27.906Z] 16:51:27     INFO -    MOZ_IMPLICIT constexpr Result(const V& aValue) : mImpl(aValue) {
[task 2021-06-22T16:51:27.906Z] 16:51:27     INFO -                           ^
[task 2021-06-22T16:51:27.906Z] 16:51:27     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Result.h:510:3: note: candidate constructor not viable: no known conversion from 'mozilla::Nothing' to 'const mozilla::Result<mozilla::Maybe<int>, nsTString<char>> &' for 1st argument
[task 2021-06-22T16:51:27.906Z] 16:51:27     INFO -    Result(const Result&) = delete;
[task 2021-06-22T16:51:27.906Z] 16:51:27     INFO -    ^
[task 2021-06-22T16:51:27.906Z] 16:51:27     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Result.h:511:3: note: candidate constructor not viable: no known conversion from 'mozilla::Nothing' to 'mozilla::Result<mozilla::Maybe<int>, nsTString<char>> &&' for 1st argument
[task 2021-06-22T16:51:27.907Z] 16:51:27     INFO -    Result(Result&&) = default;
[task 2021-06-22T16:51:27.907Z] 16:51:27     INFO -    ^

Comment 17

[Chris H-C :chutten]

Oh, bother. I see what went wrong. Forgot to ask try for an Android build.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9038557845d
Tidy up some header includes r=janerik
https://hg.mozilla.org/integration/autoland/rev/e54466d24305
JS/C++ FOG test APIs will now fail if there are errors r=janerik

Comment 19

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/e9038557845d
https://hg.mozilla.org/mozilla-central/rev/e54466d24305

Comment 20

[Jan-Erik Rediger [:janerik]]

badboy merged PR #1698: "bug 1683171 - FOG uses testGetValue for errors [doc only]" in c392a5f.