1674918 - Ion study enrollment pings should use schema name, not add-on ID

Status: RESOLVED FIXED

(Firefox Graveyard :: Pioneer, defect)

Description

[Robert Helmer [:rhelmer]]

This came up in bug 1674847, we are using the study add-on ID where we should be using the studyName (declared in the manifest.json):

https://searchfox.org/mozilla-central/rev/16d30bafd4e5276d6d3c632fb52a6c71e739cc44/browser/components/ion/content/ion.js#779-787

This is both for enrollment and deletion pings. Anthony is working around this for the first study, but we should fix and not make him do this for future studies :)

Comment 1

[Alessio Placitelli [:Dexter]]

Before changing this in m-c (and the core-addon), I think we should get a better understanding on how things work end to end and set some practices around this. My current understanding (from various Slack conversations, including this one):

• schemaNamespace must identify the directory in which schemas are deployed (and that becomes the source of truth), that is to say the name of the subdirectory under [mozilla-pipeline-schemas/schemas, e.g. pioneer-citp-news-disinfo or pioneer-meta where study schemas are found.
• schemaName must identify the name of the schema of the possible sent payloads, e.g. measurements for the measurements payload of the pioneer-citp-news-disinfo study;
• studyName which is, by convention, equal to schemaNamespace stripped of the initial pioneer- prefix. Note that this also matches the encryptionKeyId value.

@Anthony, I have a few questions for you:

• it seems to me that the only really required values are schemaNamespace and schemaName. To reduce the potential error surfaces, can we derive studyName and encryptionKeyId from the other two (either on the client or in the pipeline);
• am I right in understanding that the person who creates the schema in mozilla-pipeline-schemas decides the name of the directory, thus the studyNamespace and all the related variables?
• what are the allowed characters / format for directories in the mozilla-pipeline-schemas directory? (we can use the same ruleset to validate on the client!)

Note that the docs here also may require some amending: studyName is documented to be the name of the addon there (hence the confusion!)

Comment 2

[Anthony Miyaguchi [:amiyaguchi]]

This is actually misnamed, we should be using the schemaName, not the studyName, in both the enrollment and deletion pings.

In the terms of the ingestion pipeline, the schemaNamespace is the document_namespace, the schemaName is the document_type, and the schemaVersion is the document_version. These are the essential fields for routing.

The studyName is a free field that is injected into the study tables. This value is supposed to be the name of the addon artifact (e.g. [e-mail]), but it can take on whatever value. The pipeline does not care. One use-case could be to version the addon so that the study authors do not have to inject the values into the schemas themselves.

To directly answer your questions:

• The schemaVersion is equally as important for routing since a study table may have several different versions. The encryptionKeyId should certainly be derived from the studyNamespace, as this is how it is provisioned by ops. The studyName may be derived from the studyNamespace, but it may take on other values since it is injected into the table as-is.
• The person who creates the schema in mozilla-pipeline-schemas does have control over the namespace, as it's intended to be easy for self-service. However, it must go through data engineering review (see for example https://github.com/mozilla-services/mozilla-pipeline-schemas/pull/632#pullrequestreview-515202045).
• Validation for schema names and namespaces is defined here: https://github.com/mozilla-services/mozilla-pipeline-schemas/blob/7b15df75e9d6fbac1a2f7309b298cdc30017a39b/CMakeLists.txt#L56-L65

I think the source of confusion is the similarity of studyName and schemaName. However, I'd like it to be clear that only schema* fields in the envelope are used for routing, and thus important to the ingestion pipeline.

Comment 3

[Robert Helmer [:rhelmer]]

Thanks, I think part of the naming confusion is what we called it in the manifest.json declaration:

    "telemetry": {
        "pioneer_id": true,
        "study_name": "citp-news-disinfo",
        "ping_type": "pioneer-study",
        "schemaNamespace": "pioneer-citp-news-disinfo",
        "public_key": {
            "id": "citp-news-disinfo",
            "key": {
                "crv": "P-256",
                "kty": "EC",
                "x": "LDByX3lSRSU624OfR9EMO3So_0uRt2sNCVzPdQUKbrY",
                "y": "4Qu2FsVM8834l0GJG2ZA0JyJlX5Oe83jV54PZNyCSCA"
            }
        }
    }

Technically the schemaName is only set by their pings inside their add-on though, right? So about:ion would not know about these (we need to be able to send deletion pings even if the add-on was previously installed and the user left).

This is something we should follow up and fix in the next version, since the new Ion core will be responsible for assembling these pings IIRC.

Comment 4

[Anthony Miyaguchi [:amiyaguchi]]

The schemaName for the enrollment and deletion pings are fixed, so only the namespace needs to be known. The schemaNamespace might be tricker, but we can infer the schemaNamespace based on the public_key.id by prefixing it with pioneer-.

Comment 5

[Alessio Placitelli [:Dexter]]

(In reply to Robert Helmer [:rhelmer] from comment #3)

Technically the schemaName is only set by their pings inside their add-on though, right? So about:ion would not know about these (we need to be able to send deletion pings even if the add-on was previously installed and the user left).

Exactly. I believe we should find some way to use the addon id or use remote-settings for storing that bit of information.

This is something we should follow up and fix in the next version, since the new Ion core will be responsible for assembling these pings IIRC.

Definitely. Rob, does this require any follow-up work in tree?
Anthony did you hot-patched the pipeline to account for that for the current studies? (so that we get the correct deletion-request, etc.)?
If that's the case, then we can ignore the current implementation and make this about fixing the core addon.

Comment 6

[Anthony Miyaguchi [:amiyaguchi]]

I patched this only for [e-mail]. The debug study will still send to the wrong namespace. I will bring this up as a blocker if we do decide to provision a new study in the future without having fixed this is some form.

Comment 7

[Robert Helmer [:rhelmer]]

(In reply to Alessio Placitelli [:Dexter] from comment #5)

(In reply to Robert Helmer [:rhelmer] from comment #3)

Technically the schemaName is only set by their pings inside their add-on though, right? So about:ion would not know about these (we need to be able to send deletion pings even if the add-on was previously installed and the user left).

Exactly. I believe we should find some way to use the addon id or use remote-settings for storing that bit of information.

This is something we could add to the remote-settings schema.

This is something we should follow up and fix in the next version, since the new Ion core will be responsible for assembling these pings IIRC.

Definitely. Rob, does this require any follow-up work in tree?

Let's discuss whether it's worth getting this in for the next release, per comment 6 this is hot-patched for the only planned study.

Anthony did you hot-patched the pipeline to account for that for the current studies? (so that we get the correct deletion-request, etc.)?
If that's the case, then we can ignore the current implementation and make this about fixing the core addon.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:rhelmer, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 9

[Alessio Placitelli [:Dexter]]

This was hotfixed in the Princeton study (I assume) and was implemented in the new Core Addon. Can this be closed?

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to Alessio Placitelli [:Dexter] from comment #9)

This was hotfixed in the Princeton study (I assume) and was implemented in the new Core Addon. Can this be closed?

Yes it was hotfixed, if there's any chance that we could ship additional partner studies on about:ion we should fix it, unless we are OK hotfixing these as well.

I think the probability of this is fairly low so it's probably not worth doing the code change but I wanted to raise the specter of this :)

Comment 11

[Alessio Placitelli [:Dexter]]

You know what? Let's keep this around until we release the new architecture :) Let's not jinx it :P

Comment 12

[MarniePW [:marnie]]

Closing; no more studies will be shipping on about:ion.