Page MenuHomePhabricator

Review identifiers schema fragment (Morten)
Closed, ResolvedPublic


Please review core identifiers patch

Specific things to look for / think about:

  • clarity of field descriptions, if there's any ambiguity
  • whether it's okay for this schema fragment to reference /fragments/analytics/common fragment and declare client_dt as required (the intention is to avoid referencing both every time, since any schema that would be using this fragment will be using client_dt anyway)

Note: T259714 is blocked until the identifiers fragment is available for use in schemas


Due Date
Aug 20 2020, 4:00 AM

Event Timeline

mpopov triaged this task as High priority.Aug 5 2020, 8:13 PM
mpopov created this task.
mpopov renamed this task from Review identifiers schema fragment to Review identifiers schema fragment (Morten).Aug 5 2020, 8:16 PM

Reviewing the descriptions: I made this edit as I think fields are supposed to be required, not events.

I'm unsure what the message of this sentence is: "All new schemas for product analytics would include the core identifiers fragment". It could be interpreted as saying that all new schemas should include the core identifiers fragment (because of client_dt being required, as mentioned in this task's description). Or it could be something else.

I find the descriptions that immediately define the scope to be easier to understand and/or work with. pageview_id declares the scope to be "web" immediately. device_id doesn't declare its scope to be apps until the last sentence that mentions that MediaWiki events don't send it. I'll add this as a comment on Gerrit when I have time later, as I'm thinking that whatever's there is the authoritative description and Wikitech only copies it.

Lastly, having this fragment requiring client_dt makes sense to me.

As mentioned, I also left a comment in Gerrit. I see that the descriptions on Wikitech provide more information than the ones in the repository, which makes sense to me. Moving this to the review column.

Thank you for catching that mistake on the wikitech page and fixing it!

I updated the wikitech page and added some clarifications. Please let me know if those are useful and address your concerns.

Yeah, I think the descriptions and clarifications on the wikitech page are great, nice work!

Looks like a sentence in the note in the "Schema fragments" section was cut off: "This fragment provides the client_dt field which." Apart from that I didn't find anything. And again, I think the documentation here is good and really useful!

mpopov changed Due Date from Aug 13 2020, 4:00 AM to Aug 20 2020, 4:00 AM.Aug 13 2020, 9:22 PM
mpopov closed this task as Resolved.Aug 24 2020, 1:26 PM

Thank you, Morten!