Status

()

Firefox for iOS
Telemetry
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

(Blocks: 1 bug)

unspecified
All
iOS
Dependency tree / graph

Firefox Tracking Flags

(fxios-v4.0 fixed, fxios-v5.0 fixed, fxios5.0+)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
This bug is for implementing the core ping described at [1], which will allow us to track essential usage data.

[1] https://gecko.readthedocs.org/en/latest/toolkit/components/telemetry/telemetry/core-ping.html
(Assignee)

Comment 1

2 years ago
Created attachment 8744015 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1734
Attachment #8744015 - Flags: review?(sarentz)
Couple of questions about the JSON that you posted:

{
  "arch": "arm",
  "locale": "en",
  "device": "Apple-iPod5,1",
  "clientId": "E33FA481-33DF-4A30-9DBB-858E8BD2C5CC",
  "osversion": "9.2.0",
  "os": "iOS",
  "profileDate": 16892,
  "v": 3,
  "defaultSearch": "Yahoo",
  "seq": 3
}

I think "arch" can be more specific. I see it is hardcoded. We should really ask the OS for the right value. Because it could be armv7, armv7s or arm64. Or whatever Apple decides to use for the iPhone 7.

The "locale" looks too short. Shouldn't that be at least a full locale code like "en_CA"?

Do we need the "Apple-" prefix for "device"? Maybe we can have a "vendor" field instead?


In the PR you wrote "We don't send the pings on developer builds". What do we do for TestFlight and Aurora builds? I'd like to get data for those too.

Can we include a "channel" field in the Core ping so that we know what build this is?
Flags: needinfo?(mark.finkle)
Flags: needinfo?(bnicholson)
(Assignee)

Comment 3

2 years ago
(In reply to Stefan Arentz [:st3fan] from comment #2)
> I think "arch" can be more specific. I see it is hardcoded. We should really
> ask the OS for the right value. Because it could be armv7, armv7s or arm64.
> Or whatever Apple decides to use for the iPhone 7.

It was a pain to get that when I tried, and we could figure it out if needed based on the model. It didn't seem as important as it would be on Android since we don't have the fragmentation issues, but I can try again if we think it matters.

> The "locale" looks too short. Shouldn't that be at least a full locale code
> like "en_CA"?

The docs say "app locale", so it's whatever locales are available to the app. Since I'm running a developer build, "en" is all that's available, so that's what we get.

> Do we need the "Apple-" prefix for "device"? Maybe we can have a "vendor"
> field instead?

I think mfinkle can answer this one...I'm also curious why this was spec'd as a single field instead of two.

> In the PR you wrote "We don't send the pings on developer builds". What do
> we do for TestFlight and Aurora builds? I'd like to get data for those too.

That condition is guarded on AppConstants.BuildChannel == AppBuildChannel.Developer, so as long as TestFlight and Aurora builds use a different channel, we'll send the ping.

> Can we include a "channel" field in the Core ping so that we know what build
> this is?

We do :) Check the URL! It includes the version, build number, and release channel.
Flags: needinfo?(bnicholson)
(In reply to Brian Nicholson (:bnicholson) from comment #3)
> (In reply to Stefan Arentz [:st3fan] from comment #2)
> > I think "arch" can be more specific. I see it is hardcoded. We should really
> > ask the OS for the right value. Because it could be armv7, armv7s or arm64.
> > Or whatever Apple decides to use for the iPhone 7.

This field was originally in the spec to handle "arm or x86", not to track the fragmentation of the either processor type. If iOS will also be "arm", so be it. We'll have other pings, like the crash ping, which can give more details when looking for a particular use-case of processor type.

At the core ping level, the device will likely provide enough detail for dimensional analysis.

> > The "locale" looks too short. Shouldn't that be at least a full locale code
> > like "en_CA"?
> 
> The docs say "app locale", so it's whatever locales are available to the
> app. Since I'm running a developer build, "en" is all that's available, so
> that's what we get.

Gecko, and the Android core ping, provide locales in AA-BB format. If that's not simple to do in iOS, then let's just send the AA_BB format.

We do intend to have a single DB table with Android and iOS data intermixed (think Desktop and Windows, Linux and Darwin). If we ever try to do WHERE clauses on locale, we'll just need to be aware of the differences.

> > Do we need the "Apple-" prefix for "device"? Maybe we can have a "vendor"
> > field instead?
> 
> I think mfinkle can answer this one...I'm also curious why this was spec'd
> as a single field instead of two.

We want to keep the core ping as small as possible, so we can send it under any network situation, Wifi or cell data. So we didn't want too many separate fields. We also considered the "how would we use this?" aspect and settled on a unique device field.

If we had to do it again, I might consider using an Array, ["Apple", "iPod5,1"], so we could easily separate the two components. Let's see how we end up using it.

> > Can we include a "channel" field in the Core ping so that we know what build
> > this is?
> 
> We do :) Check the URL! It includes the version, build number, and release
> channel.

As I mentioned above, we'll be mixing Android and iOS data together. What are the possible values of "channel"? Obviously, on Desktop and Android they are "nightly", "aurora", "beta" and "release"
Flags: needinfo?(mark.finkle)
(In reply to Mark Finkle (:mfinkle) from comment #4)

> 
> > > The "locale" looks too short. Shouldn't that be at least a full locale code
> > > like "en_CA"?
> > 
> > The docs say "app locale", so it's whatever locales are available to the
> > app. Since I'm running a developer build, "en" is all that's available, so
> > that's what we get.
> 
> Gecko, and the Android core ping, provide locales in AA-BB format. If that's
> not simple to do in iOS, then let's just send the AA_BB format.

Well this is one of my pet peeves .. :-)

aa-BB is a language identifier
aa_BB is a locale identifier

Which are very different things and may not even have the same values.

It sounds like that field should have been named "language" is that is what we want. If we accidentally called it locale but expect a language identifier then we should note that in the docs.


Also, they can have shorter and longer forms:

For language identifiers: language,  language-region, language-script

For locale identifiers: language, language_region, language-script, language-script_region

None of this is in our control as it depends on user settings.


But what should this be? System language or application language? The first is more interesting to me, nbecause that also tells me if people use a language that we do not support. (In that case the current method would default to 'en' - which is the fallback language)
Flags: needinfo?(mark.finkle)
Flags: needinfo?(bnicholson)
(In reply to Mark Finkle (:mfinkle) from comment #4)

> 
> > > The "locale" looks too short. Shouldn't that be at least a full locale code
> > > like "en_CA"?
> > 
> > The docs say "app locale", so it's whatever locales are available to the
> > app. Since I'm running a developer build, "en" is all that's available, so
> > that's what we get.
> 
> Gecko, and the Android core ping, provide locales in AA-BB format. If that's
> not simple to do in iOS, then let's just send the AA_BB format.

Well this is one of my pet peeves .. :-)

aa-BB is a language identifier
aa_BB is a locale identifier

Which are very different things and may not even have the same values.

It sounds like that field should have been named "language" is that is what we want. If we accidentally called it locale but expect a language identifier then we should note that in the docs.


Also, they can have shorter and longer forms:

For language identifiers: language,  language-region, language-script

For locale identifiers: language, language_region, language-script, language-script_region

None of this is in our control as it depends on user settings.


But what should this be? System language or application language? The first is more interesting to me, nbecause that also tells me if people use a language that we do not support. (In that case the current method would default to 'en' - which is the fallback language)
(Assignee)

Comment 7

2 years ago
Comment on attachment 8744015 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1734

Updated. More notes:

* After IRC discussions, we decided we do want to report the app locale. For format consistency, I did a s/_/-/ replace on the code. That means we'll report locales of the format AA, AA-BB, or AA-BB-CC.
* This guards the ping on the existing "settings.sendUsageData" setting. As discussed earlier, we'll want to update the Sumo article since that's currently Adjust-specific.
* Arch is still hardcoded as "arm".
* No delay before sending the ping.
* We only have two release channels now, "release" and "beta", that send pings. The "aurora" channel (used by l10n), the "developer" channel for local builds, and the unused (?) "fennec" channel all suppress the ping.
Flags: needinfo?(bnicholson)
Attachment #8744015 - Flags: review?(mark.finkle)
Comment on attachment 8744015 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1734

Look sane to me. We can file a followup for adding the Date HTTP Header, along with the next pieces of data.
Flags: needinfo?(mark.finkle)
Attachment #8744015 - Flags: review?(mark.finkle) → review+
Comment on attachment 8744015 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1734

Looks good, but I have one remaining question.

The ping is currently triggered via application:willFinishLaunching:. That means that the ping is only sent the very first time the app is cold started.

I just want to verify if that is correct because under the right conditions, an app can stay resident for days or even weeks. That means no pings all that time. (Days is very realistic, weeks could certainly happen if you are on a device with a good amount of memory and you never reboot it.)

Do we instead need to send the ping when the app goes back into the foreground? What does Android do?
Attachment #8744015 - Flags: review?(sarentz) → review+
(Assignee)

Comment 10

2 years ago
Android uses onCreate, which is the equivalent of our willFinishLaunchingWithOptions. Like iOS, Android's lifecycle rules make no guarantee about when an app will be killed, so we have parity there.

Mark mentioned possibly using onResume on Android to dispatch pings more frequently in the future; we can do the same with applicationDidBecomeActive.
(Assignee)

Comment 11

2 years ago
https://github.com/mozilla/firefox-ios/commit/69e7ef6ffaa27d5ab9c1a458d1f26b6497a8a139
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

2 years ago
v4.x: 30c535f
status-fxios-v4.0: --- → fixed
status-fxios-v5.0: --- → fixed
(Assignee)

Updated

2 years ago
Depends on: 1268554
(Assignee)

Updated

2 years ago
Depends on: 1268924
You need to log in before you can comment on or make changes to this bug.