Closed Bug 1463198 Opened 6 years ago Closed 5 years ago

Gather telemetry on the usage of the downgrade UI

Categories

(Toolkit :: Startup and Profile System, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: mossop, Assigned: mossop)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

In bug 1455707 we're adding UI to block users from downgrading with their existing profile. We'd like to gather telemetry on the usage of this UI.
Priority: -- → P3
Attached file request.txt
Attachment #8979633 - Flags: review?(chutten)
Comment on attachment 8979633 [details]
request.txt

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, provided :Mossop adjusts the Environment docs and adds a doc for the new "downgrade" ping type. (docs live in toolkit/components/telemetry/docs and are built using ./mach doc - current live docs are somewhere near here: https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/data/environment.html)

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, the data submission preference.

    If the request is for permanent data collection, is there someone who will monitor the data over time?**

Yes, Romain said he'll monitor it.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 2.

    Is the data collection request for default-on or default-off?

Default-on.

    Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

No.

    Is the data collection covered by the existing Firefox privacy notice? 

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

No.

---
Result: datareview+ with documentation issues addressed.

Also, budgetary analysis should be performed to ensure that your new ping is useful enough to take up the space and bandwidth that it will require (it almost certainly will be, but we should have it documented). Calculate the likely and worst-case ping size and frequency and ni?mreid to ensure all's well.
Attachment #8979633 - Flags: review?(chutten) → review+
Assignee: nobody → dtownsend
Comment on attachment 8979640 [details]
Bug 1463198: Submit telemetry whenever the downgrade UI is shown.

https://reviewboard.mozilla.org/r/245780/#review253720

Sorry for the delay in reviewing this; I think I can review the detection code in the other bug, but that may take me a little longer.

The code looks generally reasonable; some comments below based on reading through.  Canceling review because some of the below should be addressed first and there will be other reviews required, I think.

All of the Telemetry stuff should be reviewed by somebody involved with the server side of Telemetry, and probably somebody on the client side, too.  It will also need some sort of data-review, I think, unless you have gotten that already?

::: toolkit/xre/nsAppRunner.cpp:2622
(Diff revision 2)
> +// This must match PING_FORMAT_VERSION from TelemetrySend.jsm
> +#define PING_FORMAT_VERSION 4

Um.  Do we have plans for ensuring these don't get out of sync?

::: toolkit/xre/nsAppRunner.cpp:2659
(Diff revision 2)
> +  rv = prefBranch->GetCharPref("app.update.channel", channel);
> +  NS_ENSURE_SUCCESS_VOID(rv);
> +
> +  nsID uuid;
> +  nsCOMPtr<nsIUUIDGenerator> uuidGen = do_GetService("@mozilla.org/uuid-generator;1");
> +  NS_ENSURE_TRUE_VOID(prefBranch);

Presumably this should be testing `uuidGen`?

::: toolkit/xre/nsAppRunner.cpp:2676
(Diff revision 2)
> +  time_t now;
> +  time(&now);
> +  char date[sizeof "YYYY-MM-DDThh:mm:ss.000Z"];
> +  strftime(date, sizeof date, "%FT%T.000Z", gmtime(&now));
> +
> +  nsDependentCSubstring pingId(strid + 1, NSID_LENGTH - 3);

A comment on why `- 3` would be nice; `- 1` I would kind of expect for symmetry with `+ 1`, but 3?

::: toolkit/xre/nsAppRunner.cpp:2677
(Diff revision 2)
> +  time(&now);
> +  char date[sizeof "YYYY-MM-DDThh:mm:ss.000Z"];
> +  strftime(date, sizeof date, "%FT%T.000Z", gmtime(&now));
> +
> +  nsDependentCSubstring pingId(strid + 1, NSID_LENGTH - 3);
> +  nsCString pingType("downgrade");

`NS_NAMED_LITERAL_CSTRING`?

::: toolkit/xre/nsAppRunner.cpp:2687
(Diff revision 2)
> +  nsPrintfCString ping(
> +    "{\n"
> +    "  \"type\": \"%s\",\n"
> +    "  \"id\": \"%s\",\n"
> +    "  \"creationDate\": \"%s\",\n"
> +    "  \"version\": %d,\n"
> +    "  \"application\": {\n"
> +    "    \"architecture\": \"%s\",\n"
> +    "    \"buildId\": \"%s\",\n"
> +    "    \"name\": \"%s\",\n"
> +    "    \"version\": \"%s\",\n"
> +    "    \"displayVersion\": \"%s\",\n"
> +    "    \"vendor\": \"%s\",\n"
> +    "    \"platformVersion\": \"%s\",\n"
> +    "    \"xpcomAbi\": \"%s\",\n"
> +    "    \"channel\": \"%s\"\n"
> +    "  },\n"
> +    "  \"clientId\": \"%s\",\n"
> +    "  \"payload\": {\n"
> +    "    \"lastVersion\": \"%s\",\n"
> +    "    \"lastBuildId\": \"%s\",\n"
> +    "    \"hasSync\": %s,\n"
> +    "    \"hasBinary\": %s,\n"
> +    "    \"button\": %d\n"
> +    "  }\n"
> +    "}",
> +    pingType.get(), PromiseFlatCString(pingId).get(), date, PING_FORMAT_VERSION,
> +    arch.get(), (const char*)gAppData->buildID, (const char*)gAppData->name,
> +    (const char*)gAppData->version, NS_STRINGIFY(MOZ_APP_VERSION_DISPLAY),
> +    (const char*)gAppData->vendor, gToolkitVersion,
> +#ifdef TARGET_XPCOM_ABI
> +    TARGET_XPCOM_ABI,
> +#else
> +    "unknown";
> +#endif
> +    channel.get(), clientId.get(), PromiseFlatCString(lastVersion).get(),
> +    PromiseFlatCString(lastBuildId).get(),
> +    aHasSync ? "true" : "false",
> +    aHasBinary ? "true" : "false",
> +    aButton);

I would feel slightly better if this used `JSONWriter` (mfbt/JSONWriter.h), but I don't think it's a blocker; WDYT?

It would help make it more visually obvious what arguments correspond to what field of the JSON packet.

::: toolkit/xre/nsAppRunner.cpp:2728
(Diff revision 2)
> +  nsCString url(server);
> +  url.Append("/submit/telemetry/");
> +  url.Append(pingId);
> +  url.AppendLiteral("/");
> +  url.Append(pingType);
> +  url.AppendLiteral("/");
> +  url.Append(gAppData->name);
> +  url.AppendLiteral("/");
> +  url.Append(gAppData->version);
> +  url.AppendLiteral("/");
> +  url.Append(channel);
> +  url.AppendLiteral("/");
> +  url.Append(gAppData->buildID);
> +  url.AppendLiteral("?v=");
> +  url.AppendInt(PING_FORMAT_VERSION);

We are willing to use a giant format string for the payload, but we're going to build up the URL with repeated appends? :)

::: toolkit/xre/nsAppRunner.cpp:2745
(Diff revision 2)
> +  url.Append(gAppData->buildID);
> +  url.AppendLiteral("?v=");
> +  url.AppendInt(PING_FORMAT_VERSION);
> +
> +  nsCOMPtr<nsIFile> pingFile;
> +  rv = NS_GetSpecialDirectory("UAppData", getter_AddRefs(pingFile));

`XRE_USER_APP_DATA_DIR` instead of `"UAppData"`, please.

::: toolkit/xre/nsAppRunner.cpp:2778
(Diff revision 2)
> +  rv = pingFile->OpenANSIFileDesc("w", &file);
> +  NS_ENSURE_SUCCESS_VOID(rv);
> +  fprintf(file, "%s", ping.get());
> +  fclose(file);
> +
> +  nsCString filePath = pingFile->NativePath();

I think this fails (at compile time or runtime, I can't say which) on Windows, because paths on Windows are not UTF-8.

::: toolkit/xre/nsAppRunner.cpp:4781
(Diff revision 2)
>                                       nsPrintfCString("%.16" PRIu64, uint64_t(gMozillaPoisonBase)));
>    CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("FramePoisonSize"),
>                                       nsPrintfCString("%" PRIu32, uint32_t(gMozillaPoisonSize)));
>  
> +  if (aIsDowngrade) {
> +    Preferences::SetBool("profile.downgraded", true);

Are we planning for this to be permanently set in somebody's profile, or is there code for erasing this at some point?
Attachment #8979640 - Flags: review?(nfroyd)
Comment on attachment 8979640 [details]
Bug 1463198: Submit telemetry whenever the downgrade UI is shown.

https://reviewboard.mozilla.org/r/245780/#review253720

> Are we planning for this to be permanently set in somebody's profile, or is there code for erasing this at some point?

I was planning on permanently set. Maybe it should be some kind of marker for when it happened, either the version they downgraded to or the date it happened. I'll see what the telemetry folks think.
Comment on attachment 8979640 [details]
Bug 1463198: Submit telemetry whenever the downgrade UI is shown.

https://reviewboard.mozilla.org/r/245780/#review253720

> I was planning on permanently set. Maybe it should be some kind of marker for when it happened, either the version they downgraded to or the date it happened. I'll see what the telemetry folks think.

Actually we're just going to drop this.
Comment on attachment 8979640 [details]
Bug 1463198: Submit telemetry whenever the downgrade UI is shown.

https://reviewboard.mozilla.org/r/245780/#review254530

Thank you!

::: toolkit/xre/nsAppRunner.cpp:2777
(Diff revisions 2 - 3)
> -  const char* args[2];
> +  fclose(file);
> +
> +  PathString filePath = pingFile->NativePath();
> +  const filesystem::Path::value_type* args[2];
> +#ifdef XP_WIN
> +  nsString urlw = NS_ConvertUTF8toUTF16(url);

This can just be:

```
NS_ConvertUTF8toUTF16 urlw(url);
```

which saves some work.

::: toolkit/components/telemetry/telemetry-constants.mozbuild:7
(Diff revision 3)
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DEFINES['TELEMETRY_PING_FORMAT_VERSION'] = 4

Nice.
Attachment #8979640 - Flags: review?(nfroyd) → review+
Comment on attachment 8979640 [details]
Bug 1463198: Submit telemetry whenever the downgrade UI is shown.

https://reviewboard.mozilla.org/r/245780/#review254790

Pulling the telemetry version out into AppConstants is weird, not gonna lie. But I have nothing against it.

Also: wow, is it ever really painful sending a custom ping from C++. I'm glad this doesn't come up terribly often, and I'm sorry it did for you.
Attachment #8979640 - Flags: review?(chutten) → review+
Looking forward to reviewing the tests!
(In reply to Chris H-C :chutten from comment #11)
> Looking forward to reviewing the tests!

How could I go about writing tests for this? This is in code that cannot be easily triggered at runtime.
Flags: needinfo?(chutten)
I'm not sure. I'm not familiar with nsAppRunner. 

I mention tests because permanent data collection is the sort of thing that benefits from tests (to make sure later code changes don't mess with your data collection's timing or contents). If it's not possible, then it's not ideal but I guess we push on.
Flags: needinfo?(chutten)
MozReview-Commit-ID: 8LFNJCCG8cD
Attachment #9011140 - Attachment description: Bug 1463198: Submit telemetry whenever the downgrade UI is shown. → Bug 1463198: Submit telemetry whenever the downgrade UI is shown. r=froydnj
Attachment #8979640 - Attachment is obsolete: true
I think the only change with this new patch is that we no longer submit the hasBinary flag.
(In reply to Dave Townsend [:mossop] (he/him) from comment #15)
> I think the only change with this new patch is that we no longer submit the
> hasBinary flag.

It looks like the hasBinary flag is still there?  Is this an oversight, or confusion?
Flags: needinfo?(dtownsend)
(In reply to Nathan Froyd [:froydnj] from comment #16)
> (In reply to Dave Townsend [:mossop] (he/him) from comment #15)
> > I think the only change with this new patch is that we no longer submit the
> > hasBinary flag.
> 
> It looks like the hasBinary flag is still there?  Is this an oversight, or
> confusion?

So it's still passed to telemetry but just set to false all the time. Removing it entirely from the ping would involve updating the schema on the server-side. Chutten, would you rather I did that?
Flags: needinfo?(dtownsend) → needinfo?(chutten)
Ideally, yes :)

The volume of downgrades is so low that a couple of extra bytes per ping for pings that are sent never for most, and almost never for those who send it at all won't break users' disks or bandwidth (or ours to receive and store it).

I'm fine with that being filed as a follow-up and prioritized appropriate to its low severity. (Though as a part of verifying downgrade pings you should verify that the volume actually -is- low)
Flags: needinfo?(chutten)
Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b965981c9ce0
Submit telemetry whenever the downgrade UI is shown. r=froydnj, datareview=chutten
Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86b7743d7a65
Submit telemetry whenever the downgrade UI is shown. r=froydnj, datareview=chutten
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Depends on: 1532760
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: