Closed Bug 1536877 Opened 5 years ago Closed 5 years ago

Telemetry API: incorrect type for 'value' arg in 'recordEvent' method

Categories

(WebExtensions :: General, defect, P2)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(firefox67 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: jhirsch, Assigned: jhirsch)

Details

Attachments

(2 files)

Bug 1536877 - Correct the 'value' type in 'browser.telemetry.recordEvent'; r?rpl
47 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-beta+
Details | Review
Patch to correct the telemetry constant for beta
1.14 KB, patch
chutten
: review+
pascalc
: approval-mozilla-beta+
Details | Diff | Splinter Review

Discovered today that the type for the value argument to recordEvent is incorrectly specified as an integer in the telemetry webextension API[1], while in the telemetry service, it's expected to be a string[2]. This results in the telemetry service dropping any event pings with a non-null value argument[3].

Sadly, I tested value for the scalarAdd and scalarSet methods, but overlooked it for the recordEvent method[4] (and evidently assumed it would be an integer for events, since it was for scalars).

Should be a really straightforward fix. The temporary workaround, since the telemetry API is Mozilla-privileged, would be for consumers to instead roll their own embedded API for the recordEvent method with the correct type for value. I'll likely wind up doing this for lockbox, so I'll link that example into this bug for anyone affected.

I can probably just fix this directly, since it seems small.

[1] https://searchfox.org/mozilla-central/source/toolkit/components/extensions/schemas/telemetry.json#230

[2] https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/core/nsITelemetry.idl#490

[3] https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/core/TelemetryEvent.cpp#782

[4] https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js

Assignee: nobody → jhirsch
Priority: -- → P2
Component: Untriaged → General

One more place to fix: the docs I generated from the json definition of that API

Thanks for pointing that out. I included the documentation fix in my patch.

Status: NEW → ASSIGNED
Pushed by jhirsch@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb866ced3ffa
Correct the 'value' type in 'browser.telemetry.recordEvent'; r=rpl

Not sure which flags to set with a fixed bug

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

Whoops, missed the ni?. Trying again: not sure which flags I need to set here. Mind taking a look?

Flags: needinfo?(ddurst)

(I think it's changed to fixed by a sheriff when it goes to central.) Is this something we want ahead of 68?

Flags: needinfo?(ddurst) → needinfo?(lgreco)

(In reply to Jared Hirsch [:_6a68] [:jhirsch] (Needinfo please) from comment #7)

Whoops, missed the ni?. Trying again: not sure which flags I need to set here. Mind taking a look?

(In reply to David Durst [:ddurst] from comment #9)

(I think it's changed to fixed by a sheriff when it goes to central.) Is this something we want ahead of 68?

Hi Jared,
as David already mentioned, the bugzilla status and flags are usually updated by a sheriff when the patch reaches central (e.g. like in Bug 1541449 comment 5).

Besides setting the status to RESOLVE/FIXED, it looks that they usually also set the "Target Milestone" and the "status-firefoxNN" flag related to the firefox version on which the bug has been fixed, but if I recall correctly we can't set those bugzilla fields without the right "permissions".

You may set a needinfo assigned to :ccoroiu to double-check why they have not been set (and if :ccoroiu can set them for you on this issue).

Flags: needinfo?(lgreco)

(In reply to David Durst [:ddurst] from comment #9)

Is this something we want ahead of 68?

I missed to reply to this part. In my opinion yes: the patch to fix the issue is quite small, restricted to that telemetry API and tested in automation, and so I think it would be reasonable to request an uplift to 67 beta.

Jared,
if you also agree that we want to uplift this fix to 67 beta,
do you mind to provide QA an STR to verify this issue on Nightly and create the uplift request?

Flags: needinfo?(jhirsch)

Thanks for the explanation. Sure, I can request uplift.

Flags: needinfo?(jhirsch)

Comment on attachment 9058033 [details]
Bug 1536877 - Correct the 'value' type in 'browser.telemetry.recordEvent'; r?rpl

Beta/Release Uplift Approval Request

  • User impact if declined: Some event pings submitted by privileged webextensions will be dropped, specifically, those with a non-null 'value' field. I don't know what this list of webextensions would be, but don't see any in searchfox that are affected. (Possibly Shield studies could be impacted)
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It's a low risk bug because this patch is just a change to the schema type for one field in a privileged Mozilla-only webextension API. The bug doesn't throw a user-visible error, it just causes the event to be dropped.
  • String changes made/needed: None
Attachment #9058033 - Flags: approval-mozilla-beta?

Comment on attachment 9058033 [details]
Bug 1536877 - Correct the 'value' type in 'browser.telemetry.recordEvent'; r?rpl

Looks reasonably safe for uplift, accepted for beta 13 thanks.

Attachment #9058033 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Backed out changeset f350dc70a864 (bug 1536877) for XPCShell failures in toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js. a=backout

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=241466596&repo=mozilla-beta&lineNumber=2443

Push with failures:
https://hg.mozilla.org/releases/mozilla-beta/rev/f350dc70a86486f45ee7e2414332d9c058445617

Backout:
https://hg.mozilla.org/releases/mozilla-beta/rev/b1cb8bd44deeae0e8e0d94f7755b1abb04ef5653

Status: RESOLVED → REOPENED
status-firefox67: fixed → ---
Flags: needinfo?(jhirsch)
Resolution: FIXED → ---

That's odd, |Ci.nsITelemetry.DATASET_PRERELEASE_CHANNELS| seems to be used in a lot of tests.

:gfritzsche, any idea why this test would be failing on 67 beta?

Flags: needinfo?(jhirsch) → needinfo?(gfritzsche)

DATASET_PRERELEASE_CHANNELS is a new change (bug 1529696), so you'll probably need to use the old and oddly-named DATASET_RELEASE_CHANNEL_OPTIN.

Flags: needinfo?(gfritzsche)

Thanks for the explanation!

Is the right thing here to modify the tests in nightly to use either constant, replacing this line:

const snapshot = Services.telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, true);

with:

const DATASET_PRERELEASE_CHANNELS = Services.telemetry.DATASET_PRERELEASE_CHANNELS || 
                                    Services.telemetry.DATASET_RELEASE_CHANNEL_OPTIN;
const snapshot = Services.telemetry.snapshotEvents(DATASET_PRERELEASE_CHANNELS, true);

or, is it better/customary to change the test directly on the beta branch to use the old constant, with the understanding that it'll be automatically updated when current central becomes the next beta?

Flags: needinfo?(chutten)

I would think it would be better to change the test in your patch against beta to use the old constant directly.

We have no plans to reintroduce DATASET_RELEASE_CHANNEL_{OPTIN|OPTOUT} so it would be the cleaner option.

Flags: needinfo?(chutten)

Beta/Release Uplift Approval Request

  • User impact if declined: This patch should fix the test breakage that caused the backout.
  • Is this code covered by automated tests?: Yes - this part of the change is in the automated tests
  • Has the fix been verified in Nightly?: No - test change is Beta-only
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: The other patch attached to this bug
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Low risk. Just fixing a one-liner constant name that changed in 68.
  • String changes made/needed: None
Attachment #9060199 - Flags: review?(chutten)
Attachment #9060199 - Flags: approval-mozilla-beta?

Pushed both commits to Try on top of the beta branch, and re-ran the xpcshell tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e9283a8a4d7122dfe1f85022a66cdbeea89a3e0

Comment on attachment 9060199 [details] [diff] [review]
Patch to correct the telemetry constant for beta

Tests are green now, uplift approved for our next beta.
Attachment #9060199 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 9060199 [details] [diff] [review]
Patch to correct the telemetry constant for beta

Review of attachment 9060199 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me
Attachment #9060199 - Flags: review?(chutten) → review+
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: