1196478 - Telemetries for Success and Failure of Fennec's collections sync.

Status: RESOLVED DUPLICATE of bug 1308337

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[Ahmed Khalil]

Adding telemetries for the success and failures (either database or network errors) of Fennec's sync collections (bookmarks, tabs, clients and history).

Comment 1

[Ahmed Khalil]

Reminder to self, Keyed Histograms[1] will be very useful for this task.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe#Keyed_Histograms

Comment 2

[Ahmed Khalil]

Attachment: telemetry_collection_sync.patch

Hi Nick,

This is the first draft of the patch, I feel that there are parts that will be hard to understand, so waiting your feedback about these parts and the patch in general.

Thank You in advance.

Comment 3

[Ahmed Khalil]

Attachment: telemetry_collection_sync.patch

Sorry, forgot to add the newly added file "TelemetrySyncStats.java" to the patch.

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8683902 [details] [diff] [review]
telemetry_collection_sync.patch

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

Right now, you've defined histograms like FENNEC_SYNC_COLLECTION_TABS[key], with key being an error count, or an upload count, or ...  I think this should be the other way around: you want FENNEC_SYNC_UPLOADED_RECORDS[tabs], FENNEC_SYNC_NETWORK_ERRORS[clients], etc.  I remembered we discussed this, but I can't recall what we concluded.  Can you remind me?

I'd like to suggest a slightly different approach for two parts of the code.

1) I'd rather not have the GlobalSession responsible for submission -- make it produce a summary report, which the FxAccountSyncAdapter submits.  That'll give you a sure-fire way to track "Sync success" and "Sync failure", since the logic exists.

2) Right now you have a map associating stages to client collection names.  There's some tricky handling around map entries being present, etc.  I'd like you to try having each stage *ask* the GlobalSession for a stats object for itself, so there's no pre-registration.  Each stage knows its collection name already, so you can just do something like session.statsForCollection(COLLECTION_NAME).numUploaded += 1.

Finally, worth making a note that sync stats objects are expected to be used single-threaded (no locking, no concurrency).

Great work!  Looking forward to discussing the information representation and seeing a second cut of this.  I can help track down this about:telemetry issue, too.

::: mobile/android/base/sync/TelemetrySyncStats.java
@@ +24,5 @@
> +    public boolean isDatabaseError;
> +
> +    public void clear() {
> +        currentStage = GlobalSyncStage.Stage.idle;
> +        numDownloaded = numUploaded = 0;

nit: separate lines please, and below.  House style: it makes future changes easier to read.

::: mobile/android/base/sync/stage/ServerSyncStage.java
@@ +589,5 @@
>      int inboundCount = synchronizerSession.getInboundCount();
>      int outboundCount = synchronizerSession.getOutboundCount();
> +
> +    // Gather stats of the completed sync.
> +    session.telemetrySyncStats.numDownloaded += inboundCount;

Nice!  I had forgotten that I did the work to count records...

::: mobile/android/base/sync/stage/SyncClientsEngineStage.java
@@ +261,5 @@
>        BaseResource.consumeEntity(response);
>        session.config.persistServerClientsTimestamp(responseTimestamp);
>  
> +      // Since uploading is always done after finishing downloading we know that this stage was a success.
> +      session.telemetrySyncStats.isSuccess = true;

The stage can fail during upload, right?  This seems premature -- surely we should wait for a 200 upload response?

::: mobile/android/base/sync/telemetry/TelemetryContract.java
@@ +65,5 @@
> +  public static final String SYNC_COLLECTION_FORMS = "FENNEC_SYNC_COLLECTION_FORMS";
> +
> +  public static final String SYNC_COLLECTION_PASSWORDS = "FENNEC_SYNC_COLLECTION_PASSWORDS";
> +
> +  public static final String HISTOGRAM_KEY_SYNC_COLLECTION_SUCCESS = "SYNC_COLLECTION_SUCCESS";

These make sense.

@@ +67,5 @@
> +  public static final String SYNC_COLLECTION_PASSWORDS = "FENNEC_SYNC_COLLECTION_PASSWORDS";
> +
> +  public static final String HISTOGRAM_KEY_SYNC_COLLECTION_SUCCESS = "SYNC_COLLECTION_SUCCESS";
> +
> +  public static final String HISTOGRAM_KEY_SYNC_COLLECTION_FAILED_NETWORK = "SYNC_COLLECTION_FAILED_NETWORK";

s/FAILED/FAILURE/, so that we have SUCCESS and FAILURE with the same tense.

@@ +74,5 @@
> +
> +  /**
> +   * A key that indicates no records are either uploaded or downloaded.
> +   */
> +  public static final String HISTOGRAM_KEY_SYNC_COLLECTION_NOTHING_NEW = "SYNC_COLLECTION_NOTHING_NEW";

Is there a reason to track this separately?  I guess it's interesting to know when we are doing nothing...

::: toolkit/components/telemetry/Histograms.json
@@ +8982,5 @@
>      "kind": "count",
>      "description": "Counts the number of times that a sync has failed because of trying to sync before server backoff interval has passed."
>    },
> +  "FENNEC_SYNC_COLLECTION_TABS": {
> +    "expires_in_version": "45",

There's a new bug_numbers field required, I think: see https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe.

@@ +9015,5 @@
> +  "FENNEC_SYNC_COLLECTION_PASSWORDS": {
> +    "expires_in_version": "45",
> +    "kind": "count",
> +    "keyed": true,
> +    "description": "Gather statistics for the success, network and database failures of the Forms collection."

nit: s/Forms/Passwords/.

Also, s/Word/'word'/.  We generally refer to these exactly as they are on the server, which is .../storage/forms, etc.

Comment 5

[Ahmed Khalil]

(In reply to Nick Alexander :nalexander from comment #4)
> Comment on attachment 8683902 [details] [diff] [review]
> telemetry_collection_sync.patch
> 
> Review of attachment 8683902 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Right now, you've defined histograms like FENNEC_SYNC_COLLECTION_TABS[key],
> with key being an error count, or an upload count, or ...  I think this
> should be the other way around: you want FENNEC_SYNC_UPLOADED_RECORDS[tabs],
> FENNEC_SYNC_NETWORK_ERRORS[clients], etc.  I remembered we discussed this,
> but I can't recall what we concluded.  Can you remind me?

That was out conclusion that made me take this approach.

<R4md4c>	nalexander: Hi Nick, I'm about to start on Bug 1196478, To sum up this issue I need to add 4 keyed histograms in telemetry.json file, For History, Clients, Tabs and Bookmarks collection, each keyed histogram has keys for success, network errors and database errors, is that everything?
<nalexander>	R4md4c: I think that sounds right.
<nalexander>	R4md4c: it would be a larger set though: clients, history, forms, tabs, bookmarks, passwords.  There are 6 IIRC.
<nalexander>	R4md4c: and yes, keys for sucess, etc.

Should I continue with that approach?

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Ahmed Khalil from comment #5)
> (In reply to Nick Alexander :nalexander from comment #4)
> > Comment on attachment 8683902 [details] [diff] [review]
> > telemetry_collection_sync.patch
> > 
> > Review of attachment 8683902 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Right now, you've defined histograms like FENNEC_SYNC_COLLECTION_TABS[key],
> > with key being an error count, or an upload count, or ...  I think this
> > should be the other way around: you want FENNEC_SYNC_UPLOADED_RECORDS[tabs],
> > FENNEC_SYNC_NETWORK_ERRORS[clients], etc.  I remembered we discussed this,
> > but I can't recall what we concluded.  Can you remind me?
> 
> That was out conclusion that made me take this approach.
> 
> <R4md4c>	nalexander: Hi Nick, I'm about to start on Bug 1196478, To sum up
> this issue I need to add 4 keyed histograms in telemetry.json file, For
> History, Clients, Tabs and Bookmarks collection, each keyed histogram has
> keys for success, network errors and database errors, is that everything?
> <nalexander>	R4md4c: I think that sounds right.
> <nalexander>	R4md4c: it would be a larger set though: clients, history,
> forms, tabs, bookmarks, passwords.  There are 6 IIRC.
> <nalexander>	R4md4c: and yes, keys for sucess, etc.
> 
> Should I continue with that approach?

Roll with it.  rnewman, are you cool with histograms == engines, keys == error/success/counts?

Comment 7

[Richard Newman [:rnewman]]

Yes.

Comment 8

[Ahmed Khalil]

Attachment: telemetry_collection_sync.patch

Hi Nick,

> Is there a reason to track this separately?  I guess it's interesting to know
> when we are doing nothing...

I've added a separate key because that numUploaded and numDownloaded can both be zeroes, and since our histograms are of counting type, we will have to count the number of times that these two fields are both zeroes (A.K.A no new records on both the client and the server).

And for the new patch, I've done your suggestions and moved the submission code to the FxAccountSyncAdapter.java file, but I'm not sure where should I have put this code, it is currently resides inside SessionCallback, perhaps you can think of a better place?

Also there might be a confusion in using statsForCollection, where the collectionName can be got from the Stage.getRepositoryName() or ServerSyncStage.getCollectionName(), but I'm not sure if these two methods returns the same string when both are being used in the same stage.

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8688169 [details] [diff] [review]
telemetry_collection_sync.patch

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

This is looking solid!  Nits, and one real request.  f+ because I want to see the final changes, I'll want to test it locally (is about:telemetry working yet?) and we'll land.  Great job, Ahmed!

::: mobile/android/base/android-services.mozbuild
@@ +1162,5 @@
>      'sync/synchronizer/UnbundleError.java',
>      'sync/synchronizer/UnexpectedSessionException.java',
>      'sync/SynchronizerConfiguration.java',
>      'sync/telemetry/TelemetryContract.java',
> +    'sync/TelemetrySyncStats.java',

Consider moving this into `telemetry/`.

::: mobile/android/base/sync/GlobalSession.java
@@ +69,5 @@
>    public static final long STORAGE_VERSION = 5;
>  
>    public SyncConfiguration config = null;
>  
> +  protected Map<String, TelemetrySyncStats> collectionsTelemetryStatsMap = new ArrayMap<>();

final.  And `HashMap` is traditional -- any reason to prefer `ArrayMap`?

@@ +77,5 @@
>    public final BaseGlobalSessionCallback callback;
>    protected final Context context;
>    protected final ClientsDataDelegate clientsDelegate;
>    protected final NodeAssignmentCallback nodeAssignmentCallback;
> +  

nit: fix this.

@@ +467,5 @@
>      }
>      this.callback.handleError(this, e);
>    }
>  
> +  public TelemetrySyncStats statsForCollection(String collectionName) {

I'd like to make this always supply stats, but sometimes just dummy stats.  Then we could get rid of all the `if (stats != null)`.

So always return a non-null instance; but only store it if the Stage is known by name.

::: mobile/android/base/sync/TelemetrySyncStats.java
@@ +11,5 @@
> +    /**
> +     * Number of uploaded records.
> +     */
> +    public int numUploaded;
> +    /**

nit: put downloaded first.  Syncs proceed with download first, then upload; might as well be consistent.

::: toolkit/components/telemetry/Histograms.json
@@ +9024,5 @@
>      "kind": "count",
>      "description": "Counts the number of times that a sync has failed because of trying to sync before server backoff interval has passed."
>    },
> +  "FENNEC_SYNC_COLLECTION_TABS": {
> +    "expires_in_version": "45",

Bump these to 52 or 59 or something larger.  (ESR releases are 38, 45, 52, ...).

Comment 10

[Ahmed Khalil]

Attachment: telemetry_collection_sync.patch

I've applied your suggestions, Nick.

> final.  And `HashMap` is traditional -- any reason to prefer `ArrayMap`?
I chose ArrayMap rather than HashMap as it is more memory friendly, it is similar to SparseArray, but with a Map<K, V> interface.
I can replace it with HashMap if you insist.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8689188 [details] [diff] [review]
telemetry_collection_sync.patch

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

This is a great patch.  Just a couple of tiny nits.  I'll push a try build (hard for me to test Telemetry locally with an artifact based build.)

ally: could you p? this, or re-direct?  The new probes are opt-in, and intended to give us an idea into hidden Sync failures.  (I expect we'll get some surprising insights into how often the logins database is completely corrupt and always failing!)  Thanks!

::: mobile/android/base/background/common/telemetry/TelemetryWrapper.java
@@ +70,5 @@
> +    }
> +
> +    if (mAddToKeyedHisogram != null) {
> +      try {
> +        mAddToKeyedHisogram.invoke(null, name, key, value);

What happens if `name` or `key` is null?  (Same question for the existing method.)

If we don't crash, roll on.  If we crash, let's throw `IllegalArgumentException` and save ourselves some pain.

::: toolkit/components/telemetry/Histograms.json
@@ +9028,5 @@
> +    "expires_in_version": "52",
> +    "kind": "count",
> +    "keyed": true,
> +    "bug_numbers": [1196478],
> +    "description": "Gather statistics for the success, network and database failures of the Tabs collection."

nit: 'tabs'.  'clients'.  'history'.

Comment 12

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e4e2271c110

Comment 13

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #12)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e4e2271c110

Pleased to report this worked well for me.  Not sure how it'll fare in the wild, but:

1) go to about:telemetry
2) go to Settings > Sync > Force Sync
3) return to about:telemetry
4) force refresh
5) observe populated Fennec sync histograms.

YAY!  Well done Ahmed!

Comment 14

[Ahmed Khalil]

Attachment: telemetry_collection_sync.patch

(In reply to Nick Alexander :nalexander from comment #13)
> (In reply to Nick Alexander :nalexander from comment #12)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e4e2271c110
> 
> Pleased to report this worked well for me.  Not sure how it'll fare in the
> wild, but:
> 
> 1) go to about:telemetry
> 2) go to Settings > Sync > Force Sync
> 3) return to about:telemetry
> 4) force refresh
> 5) observe populated Fennec sync histograms.
> 
> YAY!  Well done Ahmed!

That is good to hear!

I've tried supplying null values to the mAddToKeyedHisogram.invoke, it didn't crash and it just redirected the arguments to the real method.

I've also deleted the currentStage enum in TelemetrySyncStats, as it is not used anywhere in the code base, and also changed the description string in Histograms.json file.

Comment 15

[Allison Naaktgeboren :ally]

Comment on attachment 8691036 [details] [diff] [review]
telemetry_collection_sync.patch

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

Tell me a bit about what problems you are trying to solve by Firefox 52.

I don't see a compelling reason for using keyed histograms for these. It look like you could use an enumerated histogram just fine. 

for example

55   "APPLICATION_REPUTATION_SERVER_VERDICT": {
56     "expires_in_version": "never",
57     "kind": "enumerated",
58     "n_values": 8,
59     "description": "Application reputation remote response (0=SAFE, 1=DANGEROUS, 2=UNCOMMON, 3=POTENTIALLY_UNWANTED, 4=DANGEROUS_HOST)"
60   },

::: toolkit/components/telemetry/Histograms.json
@@ +9028,5 @@
> +    "expires_in_version": "52",
> +    "kind": "count",
> +    "keyed": true,
> +    "bug_numbers": [1196478],
> +    "description": "Gather statistics for the success, network and database failures of the tabs collection."

for keyed histograms, your description should include the range of keys.

@@ +9035,5 @@
> +    "expires_in_version": "52",
> +    "kind": "count",
> +    "keyed": true,
> +    "bug_numbers": [1196478],
> +    "description": "Gather statistics for the success, network and database failures of the clients collection."

'gather statistics for x failures' is a bit too vague for keyed histograms. What statistics exactly are you measure and collecting? In general another engineer/data scientist/manager should be able to read the description and be able to interpret the data. It is okay if the descriptions is more than one sentence.

Comment 16

[Nick Alexander :nalexander [he/him]]

(In reply to Allison Naaktgeboren :ally from comment #15)
> Comment on attachment 8691036 [details] [diff] [review]
> telemetry_collection_sync.patch
> 
> Review of attachment 8691036 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Tell me a bit about what problems you are trying to solve by Firefox 52.

This is general "we would like to know about Sync in the wild" telemetry.  We have essentially no error reporting; this tracks just a little:

* Sync success
* Sync failure (for various reasons);
* per-collection success/failure
* per-collection number of records uploaded/downloaded.

I have two specific questions that data will let me answer: how frequently are we seeing complete failure to Sync passwords (we speculate the store is frequently corrupt in the wild); and how many Syncs are no-op syncs, so we could turn down the frequency.  As we move to syncing collections independently, we can use that insight to tune the rate we impact your battery.

> I don't see a compelling reason for using keyed histograms for these. It
> look like you could use an enumerated histogram just fine. 

I don't see a compelling reason not to.  Why sign up for enumerating things now (with warnings about not making mistakes, 'cuz it's in stone) rather than just using the simple generic thing?

I'd be pleased to use a keyed-and-enumerated histogram, so we have "histogram[collection] = datapoint" and don't have per-collection histograms.

Educate us.

> for example
> 
> 55   "APPLICATION_REPUTATION_SERVER_VERDICT": {
> 56     "expires_in_version": "never",
> 57     "kind": "enumerated",
> 58     "n_values": 8,
> 59     "description": "Application reputation remote response (0=SAFE,
> 1=DANGEROUS, 2=UNCOMMON, 3=POTENTIALLY_UNWANTED, 4=DANGEROUS_HOST)"
> 60   },
> 
> ::: toolkit/components/telemetry/Histograms.json
> @@ +9028,5 @@
> > +    "expires_in_version": "52",
> > +    "kind": "count",
> > +    "keyed": true,
> > +    "bug_numbers": [1196478],
> > +    "description": "Gather statistics for the success, network and database failures of the tabs collection."
> 
> for keyed histograms, your description should include the range of keys.

OK.

> @@ +9035,5 @@
> > +    "expires_in_version": "52",
> > +    "kind": "count",
> > +    "keyed": true,
> > +    "bug_numbers": [1196478],
> > +    "description": "Gather statistics for the success, network and database failures of the clients collection."
> 
> 'gather statistics for x failures' is a bit too vague for keyed histograms.
> What statistics exactly are you measure and collecting? In general another
> engineer/data scientist/manager should be able to read the description and
> be able to interpret the data. It is okay if the descriptions is more than
> one sentence.

OK.  I think this is going to change if we move to enumerated histograms, so I'll wait on your suggestions there.

Comment 17

[Allison Naaktgeboren :ally]

(In reply to Nick Alexander :nalexander from comment #16)
> (In reply to Allison Naaktgeboren :ally from comment #15)
> > Comment on attachment 8691036 [details] [diff] [review]
> > telemetry_collection_sync.patch
> > 
> > Review of attachment 8691036 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Tell me a bit about what problems you are trying to solve by Firefox 52.
> 
> This is general "we would like to know about Sync in the wild" telemetry. 
> We have essentially no error reporting; this tracks just a little:
> 
> * Sync success
> * Sync failure (for various reasons);
> * per-collection success/failure
> * per-collection number of records uploaded/downloaded.
> 
> I have two specific questions that data will let me answer: how frequently
> are we seeing complete failure to Sync passwords (we speculate the store is
> frequently corrupt in the wild); and how many Syncs are no-op syncs, so we
> could turn down the frequency.  As we move to syncing collections
> independently, we can use that insight to tune the rate we impact your
> battery.

Excellent, Thank you.
> 
> > I don't see a compelling reason for using keyed histograms for these. It
> > look like you could use an enumerated histogram just fine. 
> 
> I don't see a compelling reason not to.  Why sign up for enumerating things
> now (with warnings about not making mistakes, 'cuz it's in stone) rather
> than just using the simple generic thing?
> 
> I'd be pleased to use a keyed-and-enumerated histogram, so we have
> "histogram[collection] = datapoint" and don't have per-collection histograms.
> 
> Educate us.

Keyed histograms are higher risk and should be avoided unless you have a good reason. They also incur more paperwork. Given the problems you would like to study by 52, an enum would do the job.

The lack of dashboard support for keyed histograms has previously been a dealbreaker for some groups. I don't think that's a dealbreaker you, but it's worth mentioning. 

It is true that the n_values of a enumerated histogram is fixed, but you are free to add values, and n_values can comfortably go up to 50-100 if you would like.

Comment 18

[Ahmed Khalil]

So Nick, Obviously keyed histograms works in our case, but ally recommended enumerated histograms as keyed histograms has its drawbacks.

So should I continue with keyed or begin researching on adding support of enumerated histograms on Android?

Comment 19

[Nick Alexander :nalexander [he/him]]

(In reply to Ahmed Khalil from comment #18)
> So Nick, Obviously keyed histograms works in our case, but ally recommended
> enumerated histograms as keyed histograms has its drawbacks.
> 
> So should I continue with keyed or begin researching on adding support of
> enumerated histograms on Android?

Sorry for the delayed reply.  I think we should use an enumerated histogram, as per ally's advice.  Sorry to make more work for this patch, but I was not aware that keyed histograms shouldn't be used.

Comment 20

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #19)
> (In reply to Ahmed Khalil from comment #18)
> > So Nick, Obviously keyed histograms works in our case, but ally recommended
> > enumerated histograms as keyed histograms has its drawbacks.
> > 
> > So should I continue with keyed or begin researching on adding support of
> > enumerated histograms on Android?
> 
> Sorry for the delayed reply.  I think we should use an enumerated histogram,

To be clear: a bunch of enumerated histograms, one histogram for each collection, one enumeration for each thing data point (success, failure, db_error).  I think that makes sense.

Comment 21

[Ahmed Khalil]

Hi Nick,

While working with enumerated histograms, a question popped in my mind, what if we wanted to count the records that are uploaded/download, then I don't think that enumerated histogram would work on that case, right?
As when dealing with enumerated histogram, you will have to supply a histogram name, and an enum that will have a counter that will be incremented with every call to TelemetryWrapper.addToEnumeratedHistogram(histName, enumEntry).

Comment 22

[Nick Alexander :nalexander [he/him]]

(In reply to Ahmed Khalil from comment #21)
> Hi Nick,
> 
> While working with enumerated histograms, a question popped in my mind, what
> if we wanted to count the records that are uploaded/download, then I don't
> think that enumerated histogram would work on that case, right?

Hmm.  Reading https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe, it sounds like we can't associate an integer to an enumeration value.  We just get to say "I saw this enumerated value".  So yes, I agree.

> As when dealing with enumerated histogram, you will have to supply a
> histogram name, and an enum that will have a counter that will be
> incremented with every call to
> TelemetryWrapper.addToEnumeratedHistogram(histName, enumEntry).

Roger that.  I think that suggests sticking with keyed histograms; do you agree?

Comment 23

[Ahmed Khalil]

I agree in using keyed histogram, but what are we going to do with ally's suggestion about using enumerated histograms?

Comment 24

[Nick Alexander :nalexander [he/him]]

(In reply to Ahmed Khalil from comment #23)
> I agree in using keyed histogram, but what are we going to do with ally's
> suggestion about using enumerated histograms?

Per Comment 22, I don't think we can use enumerated histograms at all.  They just don't solve our use case.

I'm going to flag a different privacy peer for direction.

Comment 25

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8691036 [details] [diff] [review]
telemetry_collection_sync.patch

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

Benjamin, could you read Ahmed's patch and ally's comments at https://bugzilla.mozilla.org/show_bug.cgi?id=1196478#c15 and provide concrete direction on how we should collect this data?  It's my opinion that enumerated histograms do not let us maintain the counts that we want, but we're not expert in this area and have possibly gotten it wrong once already.  Thanks!

Comment 26

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #25)
> Comment on attachment 8691036 [details] [diff] [review]
> telemetry_collection_sync.patch
> 
> Review of attachment 8691036 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Benjamin, could you read Ahmed's patch and ally's comments at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1196478#c15 and provide
> concrete direction on how we should collect this data?  It's my opinion that
> enumerated histograms do not let us maintain the counts that we want, but
> we're not expert in this area and have possibly gotten it wrong once
> already.  Thanks!

bsmedberg and I just discussed on IRC.  He suggests that for each type of thing we want to count (number of records upload/downloaded, number of successful syncs, number of errors of type X, number of errors of type Y) we have an enumerated histogram, where the enumeration is the Sync engine.  So, in pseudocode:

recordsUploaded[engine],
recordsDownloaded[engine],
databaseErrorCount[engine],
...

Comment 27

[:Grisha Kruglov]

This is going to be engulfed by the Sync Ping work.