1766515 - Why does using an invalid label in a `labeled_boolean` not result in an `invalid_label` error?

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Chris H-C :chutten]

This assert should not work. Calling testGetValue should throw, meaning that the assert should instead look like either this one for labeled_counter or this one for labeled_string.

Why? The docs are clear: we're supposed to record invalid_label if the label contains invalid characters or exceeds the label length.

Filing this in Toolkit :: Telemetry to begin as it's possible this is FOG-only. But I don't know for sure: I haven't gone looking for proof in RLB or glean-core or any of the other SDKs.

(( This is just for invalid labels. For the case that the label is valid, but just not in the list of static labels, see bug 1761520. ))

Comment 1

[Chris H-C :chutten]

Interestingly, if the labeled_* has static labels, then the use of an invalid label (with e.g. capital letters) also doesn't record invalid_label (or at least doesn't throw when calling testGetValue() like it ought to.

Comment 2

[Jan-Erik Rediger [:janerik]]

This (In reply to Chris H-C :chutten from comment #1)

Interestingly, if the labeled_* has static labels, then the use of an invalid label (with e.g. capital letters) also doesn't record invalid_label (or at least doesn't throw when calling testGetValue() like it ought to.

This is essentially bug 1761520: If a metric has labels statically defined we don't do any label validity checks beyond the "is this label allowed by the list?" check.

Comment 3

[Jan-Erik Rediger [:janerik]]

(In reply to Chris H-C :chutten from comment #0)

This assert should not work. Calling testGetValue should throw, meaning that the assert should instead look like either this one for labeled_counter or this one for labeled_string.

Why?

Because FOG's Boolean implementation does not return NS_ERROR_LOSS_OF_SIGNIFICANT_DATA ever. See the code: https://searchfox.org/mozilla-central/rev/dc09246dfbfd8dafeb6d55ebee18a6294d525443/toolkit/components/glean/bindings/private/Boolean.cpp#57-61

Compare that to: https://searchfox.org/mozilla-central/rev/dc09246dfbfd8dafeb6d55ebee18a6294d525443/toolkit/components/glean/bindings/private/Counter.cpp#65-70

Comment 4

[Chris H-C :chutten]

Ahhhh, because whoever wrote this (me) didn't clue in that labeled_* submetric types could inherit the errors of their labeled overmetric.

At least my instincts were correct in filing this in FOG and not in Glean SDK : )

Comment 5

[Chris H-C :chutten]

Something that came up when chatting with Perry about this: FOG's not alone in thinking that boolean is error-free. This is repeated all the way down the stack. So though there is an error that can be retrieved if we do call glean_core::test_get_num_recorded_errors with the boolean's information, there is no API to do so on the glean_core boolean trait nor on the RLB's impl.

Heck, it's not even in the docs. Meaning that either FOG needs to treat the submetric as a different type (which we kinda-sorta do for IPC purposes for labeled_counter, though that information doesn't leak out to C++ land where this would have to) or we need to expose test_get_num_recorded_errors on a metric type that doesn't itself return errors because it's a submetric of a parent/overmetric type that can record errors.

This is gonna be a fun one.

Comment 6

[GitHub Bugzilla PR Linker]

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

Comment 7

[Perry McManis [:perry.mcmanis]]

Attachment: Bug 1766515 - Implemented new glean Boolean error handling behavior on Labeled Booleans collected in Glean, and updated tests r?chutten

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37a07f2d420d
Implemented new glean Boolean error handling behavior on Labeled Booleans collected in Glean, and updated tests r=chutten

Comment 9

[Noemi Erli[:noemi_erli]]

Backed out changeset 37a07f2d420d (Bug 1766515) for causing failures in test_GIFFT.js CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=E5uBzrn8SwiJdF_PGTxXZA.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=37a07f2d420d7fec5af65264889261e14b6dbea0

Log: https://treeherder.mozilla.org/logviewer?job_id=382166655&repo=autoland&lineNumber=1877

Backout: https://hg.mozilla.org/integration/autoland/rev/0f44f95874c33f33e5d77961a8fa801493cf224b

Comment 10

[Perry McManis [:perry.mcmanis]]

Don't believe this has landed yet but rectified in: https://phabricator.services.mozilla.com/D149569

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4dd17a60cad0
Implemented new glean Boolean error handling behavior on Labeled Booleans collected in Glean, and updated tests r=chutten

Comment 12

[bszekely]

Backed out causing xpcshell failures on test_JOG.js

Failure line: TEST-UNEXPECTED-FAIL | toolkit/components/glean/tests/xpcshell/test_JOG.js | xpcshell return code: 0

Push with failures

Failure log

Backout link

[task 2022-06-28T09:45:28.105Z] 09:45:28     INFO -  TEST-START | toolkit/components/glean/tests/xpcshell/test_JOG.js
[task 2022-06-28T09:45:28.187Z] 09:45:28     INFO -  adb launch_application: am startservice -W -n 'org.mozilla.geckoview.test_runner/org.mozilla.geckoview.test_runner.XpcshellTestRunnerService$i0' -a android.intent.action.MAIN --es env0 XPCOM_DEBUG_BREAK=stack-and-abort --es env1 MOZ_CRASHREPORTER=1 --es env2 MOZ_CRASHREPORTER_NO_REPORT=1 --es env3 MOZ_DISABLE_NONLOCAL_CONNECTIONS=1 --es env4 MOZ_DEVELOPER_REPO_DIR=/builds/worker/checkouts/gecko --es env5 MOZ_DISABLE_CONTENT_SANDBOX=1 --es env6 MOZ_FETCHES_DIR=/builds/worker/fetches --es env7 MOZ_DISABLE_SOCKET_PROCESS=1 --es env8 LD_LIBRARY_PATH=/data/local/tmp/test_root/xpcb --es env9 MOZ_LINKER_CACHE=/data/local/tmp/test_root/xpcb --es env10 GRE_HOME=/data/local/tmp/test_root/xpcb --es env11 XPCSHELL_TEST_PROFILE_DIR=/data/local/tmp/test_root/xpc/p/d3c8fa6d-c39f-4c3a-819e-82ae4b6553de --es env12 HOME=/data/local/tmp/test_root/xpc/p --es env13 XPCSHELL_TEST_TEMP_DIR=/data/local/tmp/test_root/xpc/tmp/4be9f79a-819b-42d3-bb71-04087a27d02c --es env14 MOZ_ANDROID_DATA_DIR=/data/local/tmp/test_root/xpcb --es env15 MOZ_IN_AUTOMATION=1 --es env16 MOZ_ANDROID_CPU_ABI=x86_64 --es env17 MOZHTTP2_PORT=38089 --es env18 MOZNODE_EXEC_PORT=44597 --es env19 TMPDIR=/data/local/tmp/test_root/xpc/p/d3c8fa6d-c39f-4c3a-819e-82ae4b6553de --es env20 XPCSHELL_MINIDUMP_DIR=/data/local/tmp/test_root/xpc/minidumps/d3c8fa6d-c39f-4c3a-819e-82ae4b6553de --es arg0 -g --es arg1 /data/local/tmp/test_root/xpcb --es arg2 --greomni --es arg3 /data/local/tmp/test_root/xpcb/geckoview-test_runner.apk --es arg4 -m --es arg5 -e --es arg6 'const _HEAD_JS_PATH = "/data/local/tmp/test_root/xpc/head.js";' --es arg7 -e --es arg8 'const _MOZINFO_JS_PATH = "/data/local/tmp/test_root/xpc/p/d3c8fa6d-c39f-4c3a-819e-82ae4b6553de/mozinfo.json";' --es arg9 -e --es arg10 'const _PREFS_FILE = "/data/local/tmp/test_root/xpc/user.js";' --es arg11 -e --es arg12 'const _TESTING_MODULES_DIR = "/data/local/tmp/test_root/xpc/m";' --es arg13 -f --es arg14 /data/local/tmp/test_root/xpc/head.js --es arg15 -e --es arg16 'const _HEAD_FILES = ["/data/local/tmp/test_root/xpc/toolkit/components/glean/tests/xpcshell/head.js"];' --es arg17 -e --es arg18 'const _JSDEBUGGER_PORT = 0;' --es arg19 -e --es arg20 'const _TEST_CWD = "/data/local/tmp/test_root/xpc/toolkit/components/glean/tests/xpcshell";' --es arg21 -e --es arg22 'const _TEST_FILE = ["test_JOG.js"];' --es arg23 -e --es arg24 'const _TEST_NAME = "toolkit/components/glean/tests/xpcshell/test_JOG.js";' --es arg25 -e --es arg26 '_execute_test(); quit(0);' --ez use_multiprocess True --es out_file /data/local/tmp/test_root/xpc/logs/xpcshell-38cfb1ee-92f3-4c8f-b337-c1ee54a5c6ed.log
[task 2022-06-28T09:45:28.474Z] 09:45:28     INFO -  remotexpcshelltests.py | toolkit/components/glean/tests/xpcshell/test_JOG.js | 1095 | Launched Test App
[task 2022-06-28T09:45:29.223Z] 09:45:29     INFO -  remotexpcshelltests.py | toolkit/components/glean/tests/xpcshell/test_JOG.js | 1095 | Application ran for: 0:00:01.116855
[task 2022-06-28T09:45:29.298Z] 09:45:29  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/components/glean/tests/xpcshell/test_JOG.js | xpcshell return code: 0
[task 2022-06-28T09:45:29.298Z] 09:45:29     INFO -  TEST-INFO took 1193ms
[task 2022-06-28T09:45:29.298Z] 09:45:29     INFO -  >>>>>>>
[task 2022-06-28T09:45:29.298Z] 09:45:29     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2022-06-28T09:45:29.298Z] 09:45:29     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2022-06-28T09:45:29.298Z] 09:45:29     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2022-06-28T09:45:29.298Z] 09:45:29     INFO -  running event loop
[task 2022-06-28T09:45:29.298Z] 09:45:29     INFO -  toolkit/components/glean/tests/xpcshell/test_JOG.js | Starting test_setup
[task 2022-06-28T09:45:29.298Z] 09:45:29     INFO -  (xpcshell/head.js) | test test_setup pending (2)
[task 2022-06-28T09:45:29.299Z] 09:45:29     INFO -  TEST-SKIP | toolkit/components/glean/tests/xpcshell/test_JOG.js | test_setup - test_setup skipped because the following conditions were met: (AppConstants.platform == "android")
[task 2022-06-28T09:45:29.299Z] 09:45:29     INFO -  (xpcshell/head.js) | test run_next_test pending (3)
[task 2022-06-28T09:45:29.299Z] 09:45:29     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (3)
[task 2022-06-28T09:45:29.299Z] 09:45:29     INFO -  (xpcshell/head.js) | test run_next_test 1 pending (3)
[task 2022-06-28T09:45:29.299Z] 09:45:29     INFO -  (xpcshell/head.js) | test test_setup finished (3)
[task 2022-06-28T09:45:29.299Z] 09:45:29     INFO -  (xpcshell/head.js) | test run_next_test finished (2)
[task 2022-06-28T09:45:29.299Z] 09:45:29     INFO -  toolkit/components/glean/tests/xpcshell/test_JOG.js | Starting test_jog_counter_works
[task 2022-06-28T09:45:29.299Z] 09:45:29     INFO -  (xpcshell/head.js) | test test_jog_counter_works pending (2)
[task 2022-06-28T09:45:29.300Z] 09:45:29     INFO -  TEST-PASS | toolkit/components/glean/tests/xpcshell/test_JOG.js | test_jog_counter_works - [test_jog_counter_works : 38] 53 == 53

Comment 13

[Perry McManis [:perry.mcmanis]]

Attachment: Bug 1766515 - Implemented new glean Boolean error handling behavior on Labeled Booleans collected in Glean, and updated tests r=chutten

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79628afca6fd
Implemented new glean Boolean error handling behavior on Labeled Booleans collected in Glean, and updated tests r=chutten

Comment 15

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/79628afca6fd