Closed Bug 1635659 Opened 5 years ago Closed 5 years ago

Refactor the "pre-account" telemetry ping into the "Account Ecosystem" ping

Categories

(Toolkit :: Telemetry, enhancement, P3)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
81 Branch
Tracking Flags:
Tracking Status
firefox81 --- fixed

People

(Reporter: rfkelly, Assigned: markh)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1635659 - Refactor the "pre-account" telemetry ping into the "account-ecosystem" ping.
47 bytes, text/x-phabricator-request
Details | Review
data-review.md
4.52 KB, text/plain
chutten
: data-review+
Details

Bug 1529232 landed a "pre-account ecosystem ping" as a first step towards more general ecosystem telemetry infrastructure. However, the design of the system changed since that ping was landed, and it was eventually preffed off in Bug 1613692. The code is still in the tree though:

From a bit of poking around, I think we can refactor this ping to be the general "Account Ecosystem ping" without doing too much surgery, and that's likely to be easier than trying to make a new ping from scratch.

As background, the major changes to ecosystem telemetry since this ping landed are:

  • The previous design separated data into a "pre-account" ping sent by all clients, and a "post-account" ping sent only by signed-in clients. The current design has only a single "account ecosystem" ping sent by all clients.
  • The previous design included a uid field that was a HMAC of the user's FxA account uid. The current design uses an identifier that's harder to link back to a specific account, called ecosystemAnonId.

Items to tackle as part of this refactor:

  • We need to agree on the final schema for this ping and update the in-tree docs to match; see discussion in https://github.com/mozilla/gcp-ingestion/pull/1264
  • The ping needs to be submitted to a special endpoint for routing purposes; again, ref https://github.com/mozilla/gcp-ingestion/pull/1264
  • The ping should only be sent for signed-in users, and should obtain the ecosystemAnonId value using the facilities provided by Bug 1635656.
  • IIUC the sending of this ping is currently preffed off by default, and we should keep it that way to start.
  • IIUC the ping currently includes just two metrics (total_uri_count and active_ticks) and we should keep them in the ping, but re-name the store in Scalars.yaml
  • A new round of data-review.

:janerik, you did the work on the pre-account ping, does this seem like a reasonable plan to you?

Flags: needinfo?(jrediger)

That overall plan seems good. Reusing the existing parts of the code is reasonable.
I'm not sure if :chutten already agreed on how we do the routing though, so that's a point that needs to be decided first.

Are you going to take over most of the implementation with us for review?

Flags: needinfo?(jrediger) → needinfo?(rfkelly)

Are you going to take over most of the implementation with us for review?

Tentatively, yes; if it sounds OK to you, plan A will be for us (the Sync Team) to take a look at implementing this based on the code you already landed in tree, with your advice and review.

Flags: needinfo?(rfkelly)

From meeting with Raphael, we also need to make sure we've got a way to reliably trigger the sending of this ping, for QA automation.

Severity: -- → N/A
Priority: -- → P3

Adding :lina, since she's planning to take a preliminary look at this in our next two-week sprint.

Lina, it's probably worth taking a look at the meta-bug here as well, since it has some work on improving the robustness of the ping already broken out into separate bugs (such as Bug 1635660 and Bug 1635662).

The ping should only be sent for signed-in users, and should obtain the ecosystemAnonId value using the facilities provided by Bug 1635656.

To clarify: for the initial milestone we will only send this ping for signed-in users; longer-term we plan to also send an ecosystem telemetry ping for non-signed-in users (which would be missing the ecosystem_anon_id field in that case, because we don't know who the user is if they haven't signed in).

Depends on: 1652392

(In reply to Ryan Kelly [:rfkelly] from comment #0)

  • IIUC the ping currently includes just two metrics (total_uri_count and active_ticks) and we should keep them in the ping, but re-name the store in Scalars.yaml

That's correct for Scalars.yaml, but there are 10 histograms defined with this ping in Histograms.json - SEARCH_COUNTS and 9 PWMGR_* (ie, lockwise) ones. My first WIP patch will keep all of these unless I hear otherwise - does that sound OK, or was the intent to remove them?

Somewhat confusingly, there are also ~21 scalars that link to the existing ecosystem-telemetry.html documentation, but don't refer to the ecosystem pings - best I can tell, they will only be in the main ping. My intention is to not touch these.

SEARCH_COUNTS and 9 PWMGR_* (ie, lockwise) ones. My first WIP patch will keep all of these unless I hear otherwise - does that sound OK,
or was the intent to remove them?

Let's remove the PWMGR_* ones, assuming they're simple to bring back later when we're ready to deal with them. We don't have any short-term plans to analyze password-related telemetry with this system anymore, I expect they're left over from when Lockwise was the proof-of-concept target for AET.

Very early WIP, but early feedback is good feedback.

In terms of successfully submitting this ping to the telemetry pipeline, here's an example of a successful submission that was recently provided by :klukas:

curl -H "X-Debug-ID: klukas" -vvv -k -X POST "https://stage.ingestion.nonprod.dataops.mozgcp.net/submit/telemetry/$(uuidgen)/account-ecosystem/Firefox/77.0a1/default/20200415104457?v=4" -d '{"type":"account-ecosystem","id":"2aac7cec-0b1d-ac42-a369-610223b786a4","creationDate":"2020-05-13T10:41:04.858Z","version":4,"application":{"architecture":"x86-64","buildId":"20200415104457","name":"Firefox","version":"67.0a1","displayVersion":"67.0a1","vendor":"Mozilla","platformVersion":"67.0a1","xpcomAbi":"x86_64-gcc3","channel":"default"},"payload":{"ecosystemClientId":"abcdefghabcdefghabcdefghabcdefgh","ecosystemAnonId":"eyJraWQiOiJMbFU0a2VPbWhUdXE5ZkNObnBJbGRZR1Q5dlQ5ZElEd251X1NCdFRnZUVRIiwiYWxnIjoiRUNESC1FUytBMjU2S1ciLCJlbmMiOiJBMjU2R0NNIiwiZXBrIjp7Imt0eSI6IkVDIiwieCI6InpyVHRDYTZDdnhNN0NNMXNXdHlubVVkUW9MSzVpdU01YjZSbHJwWUhxZGsiLCJ5IjoiWEhoWFVJQ21RS0dNbnEwRXVFLXBqRFZ2UGRtTUNHTkRoODNZamEtSVRNcyIsImNydiI6IlAtMjU2In19.aO4mqhu_1C5A4ac99-DMjsbqloeeb9YBQ1kH0ZRYD46pHe_eP35Iyw.LgmHq54T_sSgm5Th.sqjkEhqe5hecP7GsVwqSF4f-9tA2M1_qO3KfOkU_GkE.3bh32H0xW3uvwwyrbEOJ8g","scalars":{"parent":{"browser.engagement.total_uri_count":3}}}}'

If we end up producing an outgoing request shaped like that (but maybe with some more scalars/histograms) than we'll be in good shape here.

It's not entirely clear to me how much of that formatting we get for free from TelemetryController.submitExternalPing; hopefully a lot! :-)

:klukas, if we want to check for pings that were successfully sent via the above endpoint, what table(s) should we look in in re:dash? You previously provided this query which has been very helpful, but IIUC that only covers pings submitted via the FxA telemetry endpoint, not the Desktop telemetry endpoint.

Flags: needinfo?(jklukas)

I'm putting a new WIP up that is getting towards reasonable shape. If you follow the instructions at the top of the file, it submits pings to:

curl 'https://stage.ingestion.nonprod.dataops.mozgcp.net/submit/telemetry/2759f236-8569-4f1e-bf35-eb6617827c2f/account-ecosystem/Firefox/80.0a1/default/20200712214458?v=4' -H 'User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:80.0) Gecko/20100101 Firefox/80.0' -H 'Accept: /' -H 'Accept-Language: en-US,en;q=0.5' --compressed -H 'Content-Type: application/json; charset=UTF-8' -H 'Date: Wed, 15 Jul 2020 06:50:36 GMT' -H 'Content-Encoding: gzip' -H 'Connection: keep-alive' -H 'Sec-Fetch-Dest: empty' -H 'Sec-Fetch-Mode: no-cors' -H 'Sec-Fetch-Site: cross-site' -H 'TE: Trailers' --data-raw $<snip escaped binary data>

and while there's a 200 response, I don't see the ping show up in Ryan's error dashboard or success dashboard. The ping is always submitted with ecosystem_anon_id='my-anon-ecosystem-id'

I've made a first-cut at updating the in-tree ecosystem docs, but they are screaming out for an external, stable place we can link to which describes how this system hangs together - it doesn't seem realistic to explain the entire ecosystem telemetry system in multiple, redundant places - a single canonical place is probably better.

Apart from wiring up the real anon users ID and tweaking the docs to link to an external description, I'm not aware of much else I should do here. I haven't requested review from anyone specific, but please have a look and let me know if anything screams out.

Attachment #9163416 - Attachment description: Bug 1635659 - Refactor the "pre-account" telemetry ping into the "Account Ecosystem" ping. → Bug 1635659 - Refactor the "pre-account" telemetry ping into the "account-ecosystem" ping.
Assignee: nobody → markh
Status: NEW → ASSIGNED

The pipeline will always return a 200 response unless there is some server error state. Validation of the payload happens further down the pipeline. You can read more about the edge specification on DTMO: https://docs.telemetry.mozilla.org/concepts/pipeline/http_edge_spec.html

The table for successfully validated telemetry/account-ecosystem pings in the stage environment is moz-fx-data-shar-nonprod-efed.telemetry_live.account_ecosystem_v4.

If pings fail validation, they end up in an error table. See https://sql.telemetry.mozilla.org/queries/72838/source

In particular, it looks like these pings lack the required payload.ecosystemClientId field. The current schema for telemetry/account-ecosystem expects that we will always be able to provide an ecosystemClientId, whereas the firefox-accounts/account-ecosystem schema requires we have at least one of ecosystem_client_id or ecosystem_anon_id present in the ping.

The ping is always submitted with ecosystem_anon_id='my-anon-ecosystem-id'

The payload.ecosystemAnonId field is expected to be a valid JWE, so this value will also cause the payload to be routed to the error table. You'll only see pings hit moz-fx-data-shar-nonprod-efed.telemetry_live.account_ecosystem_v4 once they have valid JWEs that decrypt to a 64-character hex string value.

Flags: needinfo?(jklukas)

(In reply to Ryan Kelly [:rfkelly] from comment #9)

It's not entirely clear to me how much of that formatting we get for free from TelemetryController.submitExternalPing; hopefully a lot! :-)

submitExternalPing gives you everything in the Common Ping Format. (So, everything outside of the payload which you provide in the call).

And for AET we specifically do not want to include clientId or environment which are the optional parts of the common ping format. It sounds like TelemetryController.submitExternalPing likely allows us to opt out of those pieces.

(In reply to Jeff Klukas [:klukas] (UTC-4) from comment #14)

And for AET we specifically do not want to include clientId or environment which are the optional parts of the common ping format. It sounds like TelemetryController.submitExternalPing likely allows us to opt out of those pieces.

Note that EcosystemTelemetry.jsm defines a "partial environment", specified using overrideEnvironment and documented here, which I haven't changed. Should that be removed and addEnvironment: false used instead so there's no environment whatsoever?

(The "old" ping also avoided including the clientId via addClientId: false and that also hasn't been changed in my patch)

Flags: needinfo?(jklukas)

The new version of the patch causes the pings to end up on the "success" dashboard \o/.

  • Every ping submits a new GUID for the ecosystem_client_id - bug 1635662 will give us a real value.

  • The validator wants a real, decryptable ecosystem_anon_id, so @rfkelly kindly generated one for me, which I hard-coded into the source.

  • Ryan and I were surprised to find these 2 fields need to be in snake_case rather than the camelCase used by almost everything else in non-glean client telemetry, so though I'd call that out to ensure that really is the intent here?

The validator wants a real, decryptable ecosystem_anon_id

Cross-linking for completeness, the real values will come from the work in Bug 1635657.

Ryan and I were surprised to find these 2 fields need to be in snake_case rather than the camelCase used by almost everything else in non-glean client telemetry, so though I'd call that out to ensure that really is the intent here?

The casing situation is intentional. It’s a desktop telemetry convention to use camelCase names, so we defined the telemetry/account-ecosystem doctype to expect camelCase names.

Elsewhere, we are now generally preferring to use snake_case field names in schemas, partly because it matches how we show fields in BigQuery. So the firefox-accounts/account-ecosystem doc type expects snake case names.

We specifically check for fields with the wrong casing so that we can reject the ping and not end up accidentally spilling a non-decoded ecosystem_anon_id value to BQ.

Note that EcosystemTelemetry.jsm defines a "partial environment", specified using overrideEnvironment and documented here, which I haven't changed. Should that be removed and addEnvironment: false used instead so there's no environment whatsoever?

I had forgotten about that aspect of the pre-account ping. I would assume that the rationale behind the reduced environment is still valid in the AET context, so the "partial environment" is likely fine. I defer to :chutten who likely has the perspective to confirm that. It's also not clear to me where we are in terms of data review. Perhaps it's clear to :chutten whether we'll need to send this AET-ized pre-account ping through data review, or if it's already covered by previous review, or if we need a general data review for AET as a whole.

Flags: needinfo?(jklukas)

It's also not clear to me where we are in terms of data review.

I'm also not clear TBH, which I think means it's worth our time to treat it as needing a fresh review.

The casing situation is intentional. It’s a desktop telemetry convention to use camelCase names, so we defined the
telemetry/account-ecosystem doctype to expect camelCase names.

IIUC from what Mark said, this is not the observed behaviour - pings that send "ecosystemAnonId" are rejected, while pings that send "ecosystem_anon_id" are accepted.

IIUC from what Mark said, this is not the observed behaviour - pings that send "ecosystemAnonId" are rejected, while pings that send "ecosystem_anon_id" are accepted.

The above would be the case for the firefox-accounts/account-ecosystem doctype, but the camelCase names should be accepted for the telemetry/account-ecosystem doctype.

I can see in moz-fx-data-shar-nonprod-efed.payload_bytes_decoded.telemetry_telemetry__account_ecosystem_v4 that there are successfully validated telemetry/account-ecosystem payloads containing ecosystemClientId in camelCase. So, it looks to me like this is working as intended.

Attached file data-review.md (obsolete) —

I' hope to start requesting formal reviews on the patch in the next day or 2, but in the mean time I think we understand the data collection well enough that we can get the data review process started.

There's a reasonable chance I screwed something up here, so I'm only requesting feedback on the review request at this stage - I'll request formal data review asap after.

Attachment #9166759 - Flags: feedback?(rfkelly)
Attachment #9166759 - Flags: feedback?(chutten)
Comment on attachment 9166759 [details] data-review.md Looks like a fine request to me.
Attachment #9166759 - Flags: feedback?(chutten) → feedback+
Comment on attachment 9166759 [details] data-review.md Thanks, this looks pretty good to me! > List all proposed measurements and indicate the category of data collection for each measurement This list is missing `ecosystemAnonId`. Cross-linking for context, the data-review for when FxA added anonId to their metrics in [here](https://github.com/mozilla/fxa/pull/5929#issuecomment-659086657) and includes some commentary about Trust approving us to treat this identifier as Category 2 data. > Default Telemetry mechanism as well as a new preference, `toolkit.telemetry.ecosystemtelemetry.enabled` I'd like to treat the `toolkit.telemetry.ecosystemtelemetry.enabled` pref as a temporary measure for managing rollout of this feature, not as a long-lived pref for users to opt out of this telemetry. If we want to offer finer-grained opt-out we should make UI for this. That said, I do see a bunch of other prefs for enabling/disabling different bits of telemetry, so maybe my expectations are off base here. :chutten, what do you think about keeping this pref long-term? > Validation of early data will check schedule & data is as expected (https://bugzilla.mozilla.org/show_bug.cgi?id=1529234). > Later specific analysis of included probes will follow when probes are added. You could also link to the specific analysis proposed in [Are our Daily Active Users really Active, and really Users?](https://docs.google.com/document/d/1-e_UOVziGzbgZOIUzdxTOnlK6p9ImId8mNg9GvXNRvc/).
Flags: needinfo?(chutten)
Attachment #9166759 - Flags: feedback?(rfkelly) → feedback+

(In reply to Ryan Kelly [:rfkelly] from comment #25)

I'd like to treat the toolkit.telemetry.ecosystemtelemetry.enabled pref as
a temporary measure for managing rollout of this feature, not as a
long-lived pref for users to opt out of this telemetry. If we want to offer
finer-grained opt-out we should make UI for this.

FWIW, I just removed that from the data-review request - I don't think the existence or not of that pref has any bearing on the request.

I made all the other requested changes, thanks.

Attached file data-review.md —
Attachment #9166759 - Attachment is obsolete: true
Attachment #9166930 - Flags: data-review?(chutten)
Depends on: 1656154

(In reply to Ryan Kelly [:rfkelly] from comment #25)

Default Telemetry mechanism as well as a new preference, toolkit.telemetry.ecosystemtelemetry.enabled

I'd like to treat the toolkit.telemetry.ecosystemtelemetry.enabled pref as
a temporary measure for managing rollout of this feature, not as a
long-lived pref for users to opt out of this telemetry. If we want to offer
finer-grained opt-out we should make UI for this.

That said, I do see a bunch of other prefs for enabling/disabling different
bits of telemetry, so maybe my expectations are off base here. :chutten,
what do you think about keeping this pref long-term?

Ah, you've spotted one of Telemetry's untidy corners. Those prefs were meant to be short-term, but alas without an automated expiry mechanism they will probably live forever unless/until the features are removed wholesale.

Flags: needinfo?(chutten)
Comment on attachment 9166930 [details] data-review.md 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. Is there a control mechanism that allows the user to turn the data collection on and off? Yes. This collection is Telemetry so can be controlled through Firefox's Preferences. If the request is for permanent data collection, is there someone who will monitor the data over time? Yes, :markh is responsible for the ping itself and its containing permanent collections that aren't otherwise owned. Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? Category 2, Interaction. Except for the Accounts Ecosystem identifiers which are Category 4 that have already been approved by Trust. Is the data collection request for default-on or default-off? Default on for all channels. Does the instrumentation include the addition of any new identifiers? 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. This collection is permanent. --- Result: datareview+
Attachment #9166930 - Flags: data-review?(chutten) → data-review+
Depends on: 1656420
Depends on: 1656426
Pushed by rkelly@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5fe3de70b098 Refactor the "pre-account" telemetry ping into the "account-ecosystem" ping. r=chutten,rfkelly

https://hg.mozilla.org/mozilla-central/rev/5fe3de70b098

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox81: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Regressions: 1658237

This is live in today's nightly, so I did a bit of manual QA. Here's how it went and what I observed, for posterity:

  • Since handling already-signed-in-accounts is a key part of our approach, I opted to test on my main profile using my already-signed-in main account:
    • Firefox reported its version as v81.0a1 (2020-08-09)
    • First, I checked that AET was properly "off" for my account to begin with:
      • Checked that signedInUser.json in my profile did not have ecosystemAnonId in the profile data.
      • about:telemetry archited ping data did not report any account-ecosystem pings
      • about:config reported that toolkit.telemetry.ecosystemtelemetry.enabled was set to its default value of false.
    • Looking in about:telemetry at the current data, in "Raw JSON" view, under payload/stores there was an account-ecosystem entry, confirming that this build of Firefox did indeed have the current patch (but was only gathering the data in memory, not submitting it).
    • I flipped the toolkit.telemetry.ecosystemtelemetry.enabled pref to true and restarted.
    • I confirmed in about:telemetry archived pings that no account-ecosystem pings had yet been sent (since no event has yet occurred to trigger them), then restarted again
    • I confirmed in about:telemetry that one account-ecosystem ping had been generated, with reason=shutdown and appropriate-looking ecosystemClientId and ecosystemAnonId values.
    • I checked signedInuser.json in my profile directory and confirmed that my profile data now contained an ecosytemAnonId field, matching the one in the account-ecosystem ping.
  • Next, I tried creating a new profile and signing in with that same account (my main account):
    • First, I observed that my bookmarks and history seemed to sync correctly after signing in, an important invariant :-)
    • I checked signedInUser.json in the new profile and if has an ecosystemAnonId field, with value matching the one from my main profile
    • I checked in about:telemetry to confirm that there were no account-ecosystem pings generated yet.
    • I flipped the toolkit.telemetry.ecosystemtelemetry.enabled pref to true and restarted.
    • I checked in about:telemetry archived data, and saw one account-ecosystem ping with reason=shutdown.
      • N.B: I was a little surprised to see a ping generated here, since I didn't think any would be generated until after the next shutdown.
      • This ping contained the same ecosystemAnonId as my main profile, but a different ecosystemClientId.
    • I signed out of this profile.
    • I checked about:telemetry to confirm another account-ecosystem ping had been produced, with reason=logout and the same identifiers as before.
  • Next, I tried signing back in to that same profile but with a newly-created account:
    • I opted out of syncing all data types in order to disable sync (which should not disable AET)
    • I confirmed in about:telemetry that no new pings were generated (because logging in doesn't trigger a send of the ping)
    • I restarted, then checked about:telemetry to find a new account-ecosystem ping with reason=shutdown.
      • This ping had the same ecosystemClientId as before, since it's the same Firefox profile.
      • This ping had a different value for ecosystemAnonId, since it was a different account.
    • I signed out again, and confirmed that another account-ecosystem ping was generated, with reason=logout.
  • Next, I tried signing back in to that same profile, with the same account, but doing a password reset:
    • I confirmed in about:telemetry that no new pings were generated (because logging in doesn't trigger a send of the ping)
    • I restarted
    • I checked about:telemetry to see that a new account-ecosystem ping had been generated, with reason=shutdown.
      • This ping had the same ecosystemClientId value as before, which is expected.
      • This ping had the same ecosystemAnonId value as before, which was a little surprising.
        • N.B. I think this is because FxA has not yet implemented rotation of the ecosystemAnonId on password reset.
  • Finally, I checked in this redash dashboard to see whether all these pings had successfully been sent.
    • After waiting about 10 mins to ensure the pings had been processed, I confirmed that the total number of pings in redash matched the total number of archived pings across my two test profiles.
    • In redash I observed pings from the following, in order:
      • ecoClientId=2562, ecoUid=ee02 (my main profile, main account)
      • ecoClientId=b808, ecoUid=ee02 (new profile, main account)
      • ecoClientId=b808, uid=abc7 (new profile, new account)

So as far as I can tell, everything here is working as expected, and we should be in a good position to enable this pref by default in Nightly for some broader testing.

Jared, can you confirm whether the note about about the ecosystemAnonId not changing on password reset is currently expected behaviour?

Flags: needinfo?(jhirsch)

For completeness, ni? myself to come back to this after using my main profile for a while, and confirm whether it submitted any pings with reason=periodic.

Flags: needinfo?(rfkelly)

ni? myself to come back to this after using my main profile for a while, and confirm whether it submitted any pings with reason=periodic.

Yep, I have one in my main profile with reason=periodic and timestamped midnight, so I think periodic pings are also working as intended.

Flags: needinfo?(rfkelly)

Jared, can you confirm whether the note about about the ecosystemAnonId not changing on password reset is currently expected behaviour?

Hmm. This observed behavior sounds like a bug. The ecosystemAnonId should be updated on password reset:

I think I understand your steps above in comment #32, but if you wouldn't mind, please file a bug against the FxA repo with details steps to reproduce.

(Just curious, is there any profile caching on the client that might account for this?)

Flags: needinfo?(jhirsch)

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

(Just curious, is there any profile caching on the client that might account for this?)

This does indeed seem possible, although if I'm reading the above correctly, the fact a restart happened after the rotate makes this less likely (although still possible)

(Note that we re-fetch profiles with if-none-match, so this assumes the etag would indeed change in this scenario, otherwise we'd never see the new id)

(Just curious, is there any profile caching on the client that might account for this?)

I think that the fact that I signed out and signed back in should have cleared any locally-cached profile info, but I'll try to reproduce and see how it goes...

(In reply to Ryan Kelly [:rfkelly] from comment #38)

I think that the fact that I signed out and signed back in should have cleared any locally-cached profile info, but I'll try to reproduce and see how it goes...

FTR, it will be cached across restarts. However, what I think should have happened is that we would have re-requested the profile very soon after the restart and well before the telemetry code asked for it. If it was possible to reset your password while signed in and there was no restart, I think we'd be likely to use the old value.

However, is it actually possible to reset your password while signed in? I'd think the only sane way to do that would be to sign out, then reset in the process of signing back in. I don't think it would be possible to use the cached value in that scenario. However, it probably would be possible if it was reset on a different profile - it might require a restart before the new anon ID was picked up.

There's probably another couple of observers we should listen to to mitigate this.

ni? myself to try to reproduce

Flags: needinfo?(rfkelly)

I'm adding environment and payload.reason to the schema (see https://bugzilla.mozilla.org/show_bug.cgi?id=1659514). Note that all the actual probes are currently being sent to the additional_properties json string field in BigQuery. If we want to represent those as individual fields in BigQuery, we will need to add them to the schema, but that also commits us to the ping structure. So I am open to discussion about how we might need to evolve the telemetry/account-ecosystem ping to accommodate future use cases.

No longer depends on: 1652392

If we want to represent those as individual fields in BigQuery

I do want this, yes, thank you for mentioning it :-)

we will need to add them to the schema,
but that also commits us to the ping structure. So I am open to discussion about how we might need to
evolve the telemetry/account-ecosystem ping to accommodate future use cases.

To be honest, I don't know enough about evolving ping structure to know what sorts of issues we might encounter here in future.

FWIW, I don't see the broad structure of the ping changing much here - we're likely to want to add things to it, but those will be done in a similar way to the existing metrics and will appear in a similar place in the payload (under e.g. scalars or histograms sub-keys). But that's a pretty naive take.

Will we be in a position to make backwards-compatible changes (e.g. adding a new optional payload field) in future?

FWIW, I don't see the broad structure of the ping changing much here

Looking more into this, I'm fairly sure the right thing to do here is to more or less copy the structure of how we handle main pings. Main pings have a fairly generic schema for validation, and then we have the mozilla-schema-generator repo that uses probe scraper output to determine a concrete BQ schema that includes a field per probe. I expect we basically just need to add some new configuration to the schema generator to tell it that the account_ecosystem_v4 table should expect probes registered with the account-ecosystem metrics store. I have not personally done much work with the schema generator, so I'll need to consult with other members of my team to make sure this is the right way forward. Expect me to spin off a data platform bug for that investigation (https://bugzilla.mozilla.org/show_bug.cgi?id=1659750)

See Also: → 1659750

Hmm. This observed behavior sounds like a bug. The ecosystemAnonId should be updated on password reset:
I think I understand your steps above in comment #32, but if you wouldn't mind, please file a bug against the FxA repo with details
steps to reproduce.

This does indeed appear to be an FxA server-side issue, I've filed https://github.com/mozilla/fxa/issues/6231 with steps to reproduce.

Flags: needinfo?(rfkelly)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: