Closed Bug 1683171 Opened 1 year ago Closed 6 months ago

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

Categories

(Toolkit :: Telemetry, task, P2)

task

Tracking

()

RESOLVED FIXED
91 Branch
Tracking Status
firefox91 --- fixed

People

(Reporter: chutten, Assigned: chutten)

References

(Blocks 1 open bug)

Details

(Whiteboard: [telemetry:fog:m?])

Attachments

(5 files)

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.

Blocks: 1670259

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

Assignee: nobody → chutten
Status: NEW → ASSIGNED
Priority: P3 → P2

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

Flags: needinfo?(rpierzina)
Flags: needinfo?(jrediger)
Flags: needinfo?(brizental)

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

Flags: needinfo?(brizental)

🚢

Flags: needinfo?(jrediger)

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

Flags: needinfo?(rpierzina)

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.
Pushed by chutten@mozilla.com:
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

Sorry about that. Fixed that up.

Flags: needinfo?(chutten)
Pushed by chutten@mozilla.com:
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

Backed out for causing bustage on Counter.cpp

[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 -    ^
Flags: needinfo?(chutten)

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

Flags: needinfo?(chutten)
Pushed by chutten@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch
You need to log in before you can comment on or make changes to this bug.