1587844 - Add support for overriding the "version" field in the common ping format

Status: RESOLVED WONTFIX

(Toolkit :: Telemetry, enhancement, P1)

Description

[Mark Reid [:mreid]]

This is a band-aid to Bug 1453377, but would do the trick for currently-known use cases.

Longer term, we plan to move to using Glean, where everything will be wonderful.

Summing up a longer conversation on Slack:

• Add a new aOption called overridePingVersion to specify a version here
• read it in assemblePing to fill in the top-level version field
• Have it default to 4 as it currently does
• Require it to be an int >= 4, otherwise error
• Leave the ?v=CONSTANT in the submission URL unchanged, verify that pings still flow through as expected

Comment 1

[Mark Reid [:mreid]]

The current workaround is to rename the ping's docType for incompatible changes. This is undesirable since it results in weirdly-named tables in the data lake.

For example, if we rename main pings to main_2, we would end up with corresponding tables named main_v4 and main_2_v4. Under the above proposal, we would report new pings with a version of 5 and end up with main_v4 and main_v5.

Comment 2

[Mark Reid [:mreid]]

(In reply to Mark Reid [:mreid] from comment #0)

• Require it to be an int >= 4, otherwise error

This requirement is to make sure this check continues to work as expected.

Comment 3

[Chris H-C :chutten]

The version is reported in two places:

1. The common ping format's version field
2. A query string on the submission url (added here).

Adjusting #1 is actually fairly straightforward, technically speaking. There are corner cases to avoid, but we have some decent test coverage here.

#2 is much harder as the submission url is built at a time potentially much later than the ping is assembled.

We know that GCP ignores the query parameter. We need confirmation that AWS will do the right thing if there's a disagreement between #1 and #2 ("the right thing" here meaning "use the value from #1").

Comment 4

[Mark Reid [:mreid]]

It looks like the AWS pipeline wants the hard-coded value of v=4 in the submission URL. ni? :whd to confirm that this is still true in the "tee to GCP and AWS" world.

Based on reviewing the AWS Telemetry decoder, it appears that sourceVersion is indeed determined solely from the document contents, so disagreement between the version field inside the ping and the query string parameter should be fine.

We should still test it to make sure.

Comment 5

[Wesley Dawson [:whd]]

(In reply to Mark Reid [:mreid] from comment #4)

It looks like the AWS pipeline wants the hard-coded value of v=4 in the submission URL. ni? :whd to confirm that this is still true in the "tee to GCP and AWS" world.

This is correct, that edge filter still applies to AWS ingestion and is also present in GCP. That filter could be changed to check that v != nil and achieve basically the same purpose (filter out v2 main pings).

From what I remember, v was introduced specifically to enable the above edge filtering and is unused downstream of the edge, as :mreid's analysis of the code suggests. Testing would indeed be prudent.

Comment 6

[Chris H-C :chutten]

So the option of changing the common ping version for just one doctype should work, but we'll of course need to write and run some tests.

Meanwhile, the option of changing the name of the doctype is still present and doesn't require us to repurpose a field designed to version the common ping format.

It might be the laziness and resistance to risky changes in me, but I'm still leaning towards changing the doctype name for incompatible schema changes. Mark, do you want to have a meeting about this so we can lay it all out and make a decision?

Comment 7

[Mark Reid [:mreid]]

That sounds good to me, will you schedule or shall I?

Comment 8

[Chris H-C :chutten]

Scheduled.

Comment 9

[Alessio Placitelli [:Dexter]]

Here's an unsolicited comment with some historical background :)

The version of the common ping format is the version number of.. the telemetry ping format. At the beginning of "unified telemetry", this was used to distinguish between the "new" payload and the legacy one.

Bumping this version would be "weird": this is really a telemetry internal implementation detail, used by "internal pipeline stuff". IMHO the payload should not influence this version number.

With that said, I understand that the problem of schema incompatible changes that you're facing right now. Should we add some notes here why changing the ping name would not work in this case?

Comment 10

[Chris H-C :chutten]

Mark, Alessio, and I had a chat today about this.

We covered the things that don't make sense:

• Incrementing the common ping version to add a payloadVersion or something is too heavy a lift on client and server given the use we'd get out of it.
• Changing to a non-integral compound version would be worse.
• Putting the version number in the doctype name would result in datasets named like sync_2_v4

This leaves us with two options:

1. Change the doctype name meaningfully (e.g. how the "untrustedModules" ping became the "third-party-modules" ping)
   • This does mean that older datasets will remain top-of-list. e.g. there will be a untrusted_modules table at the same level in the dataset hierarchy as third_party_modules, and will continue to stay there for the foreseeable future.
   • But it doesn't require any changes to the client or the pipeline, and in the case that the non-backwards-compatible payload change actually does reflect a change in the collection, it actually makes sense.
2. Allow custom pings to override the common ping version
   • This would require a small amount of engineering on the client (add an option to submitExternalPing, override in assemblePing) , and a lot of post-facto validation. But it shouldn't endanger traffic other than custom pings with custom versions.

For now we agreed to leave #2 on the shelf and recommend #1 wherever possible. In the event that #1 doesn't make sense, we can reopen this bug (or file a new one) for the client engineering necessary to make this happen.