1134727 - Add a URL parameter to the Telemetry Submission URL to indicate payload version

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Mark Reid [:mreid]]

In order to be able to differentiate between old- and new-style Telemetry pings without having to uncompress and parse the JSON document, we would like to introduce a URL parameter for the payload version.

Something as simple as appending "?v=<ping.version>" would work.

This will greatly simplify the routing of data to the current telemetry backend and the new data pipeline.

I have verified that the current backend will not be impacted by this change (URL parameters are not passed through - only the URL Path).

Comment 1

[Alessio Placitelli [:Dexter]]

As discussed over IRC, we're sending our data using a POST request. With this kind of requests, parameters are sent along with the body and encoded with it (in our case, gzipped): the server would have to uncompress the body to get the version.

An alternative approach could be to use HTTP headers to convey the version information: does it create any problem server side?

Comment 2

[Mark Reid [:mreid]]

Similar to the URL parameter, the current Telemetry backend will ignore extra request headers, so that side of things will continue to work fine.

It should work with the new routing + backend, but I defer to :whd on that.

Wes, can we do the required matching using an HTTP header instead of a URL param?

Comment 3

[Benjamin Smedberg]

I don't understand why we can't do this with a URL parameter on the client. Alessio, doesn't the client know the payload version when it submits, and can just change the URL accordingly?

Comment 4

[Alessio Placitelli [:Dexter]]

Sorry, my bad. As Benjamin mentioned the URL can be changed without and parameters can be added without having them encoded in the body.

Comment 5

[Mark Reid [:mreid]]

Are there other pros/cons to consider here? Would it make more sense to have this piece of information as a URL parameter or a HTTP Header?

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: Adds the "v=" parameter and test coverage.

This patch adds the "v=VERSION" query parameter when submitting Telemetry pings. Also adds test coverage for both the new and the old ping format.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8568009 [details] [diff] [review]
Adds the "v=" parameter and test coverage.

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

Cancelling for the open question in test_TelemetryPing.js

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +554,5 @@
>    doPing: function doPing(ping, isPersisted) {
>      this._log.trace("doPing - Server " + this._server + ", Persisted " + isPersisted);
>      let deferred = Promise.defer();
> +    let isNewPing = isNewPingFormat(ping);
> +    let versionString = isNewPing ? PING_FORMAT_VERSION : 1;

That doesn't look like a string, should be |version|?

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ +225,5 @@
> +  Assert.ok(!!request.queryString);
> +
> +  // Make sure the version in the query string matches the new ping format version.
> +  let params = request.queryString.split("&");
> +  Assert.ok(params.find(p => p == ("v=" + PING_FORMAT_VERSION)));

How does that actually work?
Shouldn't we split by '?' and '&'?

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
@@ +393,5 @@
> +    Assert.ok(!!request.queryString);
> +
> +    // Make sure the version in the query string matches the old ping format version.
> +    let params = request.queryString.split("&");
> +    Assert.ok(params.find(p => p == "v=1"));

Are the other query parts covered by other tests? Would be nice to add here if not.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8568009 [details] [diff] [review]
Adds the "v=" parameter and test coverage.

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

Cleared things up on IRC.
This looks good with the comments here and other ones above fixed or cleared up.

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +554,5 @@
>    doPing: function doPing(ping, isPersisted) {
>      this._log.trace("doPing - Server " + this._server + ", Persisted " + isPersisted);
>      let deferred = Promise.defer();
> +    let isNewPing = isNewPingFormat(ping);
> +    let versionString = isNewPing ? PING_FORMAT_VERSION : 1;

That doesn't look like a string, should be |version|?

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ +221,5 @@
>    yield sendPing(false, false);
>    let request = yield gRequestIterator.next();
> +
> +  // Check that we have a version query parameter in the URL.
> +  Assert.ok(!!request.queryString);

This is "" if no query string was present.
Change the check to notEqual("") or .length>1.

@@ +225,5 @@
> +  Assert.ok(!!request.queryString);
> +
> +  // Make sure the version in the query string matches the new ping format version.
> +  let params = request.queryString.split("&");
> +  Assert.ok(params.find(p => p == ("v=" + PING_FORMAT_VERSION)));

Ok, cleared the issue here up on IRC - i missed that queryString is just the part after the "?".

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
@@ +393,5 @@
> +    Assert.ok(!!request.queryString);
> +
> +    // Make sure the version in the query string matches the old ping format version.
> +    let params = request.queryString.split("&");
> +    Assert.ok(params.find(p => p == "v=1"));

Are the other query parts covered by other tests? Would be nice to add here if not.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Ok, strange, that also submitted the previous review comments.

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: Adds the "v=" parameter and test coverage (v2)

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: Adds the "v=" parameter and test coverage (v2)

Forgot to refresh the patch :-/

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/47af9d903789