RFC: Initial implementation of post function for Qt

[ChinYing-Li]

This PR is a work in progress, and needs some comments before I can move on.
Right now, I implemented a post function in Javascript, so that the post function could be used in a QML file.

It's not immediately clear to me how this Uploader implementation would be exposed to a Qt application.
Will the Qt application import the entire Glean.js library? (From https://github.com/mozilla-mobile/mozilla-vpn-client/tree/main/glean, this doesn't seem to be the case.)

[Dexterp37]

It's not immediately clear to me how this Uploader implementation would be exposed to a Qt application.
Will the Qt application import the entire Glean.js library? (From https://github.com/mozilla-mobile/mozilla-vpn-client/tree/main/glean, this doesn't seem to be the case.)

Hi @ChinYing-Li ! Thank you for this PR. The uploader is used by the Qt platform object. It should implement the Uploader interface (as the test uploader does). After you're done, you can check that this works by running the Qt sample.

[ChinYing-Li]

I copied the post function of BrowserUploader to qt/uploader.ts to see why people aren't using the webext's implementation for Qt platform, but it seems that the sample Qt app can run without running into any problem.
Terminal output for your reference:

chin-ying@chinying-Leopard-10SFK:~/repo/opensource/js/glean.js/samples/qt-qml-app$ .venv/bin/python3 main.py
qml: Initialized Glean succesfully.
qml: Adding to the `button_clicked` metric.
qml: Adding to the `button_clicked` metric.
qml: Adding to the `button_clicked` metric.
qml: Attempted to delete an entry from an invalid index. Ignoring.
qml: {
  "metrics": {
    "datetime": {
      "sample.app_started": "2021-05-24T06:51:47.559-04:00"
    },
    "counter": {
      "sample.button_clicked": 3
    }
  },
  "ping_info": {
    "seq": 0,
    "start_time": "2021-05-24T06:51-04:00",
    "end_time": "2021-05-24T06:52-04:00"
  },
  "client_info": {
    "telemetry_sdk_build": "0.12.0",
    "client_id": "d0c6f2e3-bf7a-4b41-b067-3c6fcb8cc438",
    "first_run_date": "2021-05-24-04:00",
    "os": "Linux",
    "os_version": "Unknown",
    "architecture": "Unknown",
    "locale": "en-US",
    "app_build": "Unknown",
    "app_display_version": "Unknown"
  }
}

I wonder if I missed some caveats of using the webext implementation in Qt. @Dexterp37

[Dexterp37]

I copied the post function of BrowserUploader to qt/uploader.ts to see why people aren't using the webext's implementation for Qt platform, but it seems that the sample Qt app can run without running into any problem.

...

I wonder if I missed some caveats of using the webext implementation in Qt. @Dexterp37

We were/are not sure about what subset of APIs are available on QT/QML: sadly not all of them are :-) Looks like we're lucky this time! My main concern was about fetch: to make sure that's working, you could spin up a local webserver (nc -l 8080) and pass serverEndpoint as a configuration option when initializing Glean in the qt-sample app ({ serverEndpoint: "http://localhost:8080"}). If you see the request being attempted, then this could be working :)

Would you kindly also update the code in this PR to reflect what you're currently testing?

[brizental]

I copied the post function of BrowserUploader to qt/uploader.ts to see why people aren't using the webext's implementation for Qt platform, but it seems that the sample Qt app can run without running into any problem.

Interesting. This should not be the case, as far as I remember Qt does not provide the fetch API. That is a browser only thing.

I see in the code changes that you have included a uploader.js file, but have not changed the exported uploader over on https://github.com/mozilla/glean.js/blob/main/glean/src/platform/qt/index.ts. That needs to be changed in order for the uploader to be actually picked up. Maybe this is the case why it seemed to work with the web ext uploader.

[ChinYing-Li]

We were/are not sure about what subset of APIs are available on QT/QML: sadly not all of them are :-) Looks like we're lucky this time! My main concern was about fetch: to make sure that's working, you could spin up a local webserver (nc -l 8080) and pass serverEndpoint as a configuration option when initializing Glean in the qt-sample app ({ serverEndpoint: "http://localhost:8080"}). If you see the request being attempted, then this could be working :)

Thank you for the pointers; it turns out that the webext's BrowserUploader does not work when listening on local webserver (using nc -l 8080).
The current implementation, which uses XMLHttpRequest, partially worked: only the first log is received at port 8080.

chin-ying@chinying-Leopard-10SFK:~/repo/opensource/js/glean.js/glean$ nc -l 8080
POST /submit/qt-qml-app/custom/1/3ee47bdb-9cdb-4d06-96f0-19434386ac76 HTTP/1.1
Host: localhost:8080
X-Client-Type: Glean.js
X-Client-Version: 0.12.0
Content-Type: application/json; charset=UTF-8
Content-Length: 441
Connection: Keep-Alive
Accept-Encoding: gzip, deflate
Accept-Language: en-US,*
User-Agent: Mozilla/5.0

{"metrics":{"datetime":{"sample.app_started":"2021-05-26T19:06:55.964-04:00"}},"ping_info":{"seq":0,"start_time":"2021-05-26T19:06-04:00","end_time":"2021-05-26T19:06-04:00"},"client_info":{"telemetry_sdk_build":"0.12.0","client_id":"29aa3260-d94f-4dc0-ae3b-721284c081d3","first_run_date":"2021-05-26-04:00","os":"Linux","os_version":"Unknown","architecture":"Unknown","locale":"en-US","app_build":"Unknown","app_display_version":"Unknown"}}

I will further investigate and update when the patch is ready!

[ChinYing-Li]

So the problem of "hanging after receiving the first log" seems to be a server side issue instead of a client side issue.
I used the snippet below to create a local server:

import socket
import sys

try:
    sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
    sock.bind(('127.0.0.1', 8080))  # (host, port) because AF_INET
    print("Listening...")
    sock.listen(1)
except Exception as e:
    print(repr(e))
    print('Failed to create socket')
    sys.exit()

if __name__ == "__main__":
    while True:
        conn, addr = sock.accept()
        print('Connected by', addr)
        print(conn.recv(4096)) # buffer size
        conn.close()

Every time the Submit ping in the Qt sample app is clicked, the server logs:

Connected by ('127.0.0.1', 52400)
b'POST /submit/qt-qml-app/custom/1/e575aec8-8564-42f5-9580-ce3e8fdb28b8 HTTP/1.1\r\nHost: localhost:8080\r\nX-Client-Type: Glean.js\r\nX-Client-Version: 0.12.0\r\nContent-Type: application/json; charset=UTF-8\r\nContent-Length: 441\r\nConnection: Keep-Alive\r\nAccept-Encoding: gzip, deflate\r\nAccept-Language: en-US,*\r\nUser-Agent: Mozilla/5.0\r\n\r\n{"metrics":{"datetime":{"sample.app_started":"2021-05-28T18:43:19.156-04:00"}},"ping_info":{"seq":0,"start_time":"2021-05-28T18:43-04:00","end_time":"2021-05-28T18:43-04:00"},"client_info":{"telemetry_sdk_build":"0.12.0","client_id":"f9a3d823-2e0d-4407-a92e-951606ad2efb","first_run_date":"2021-05-28-04:00","os":"Linux","os_version":"Unknown","architecture":"Unknown","locale":"en-US","app_build":"Unknown","app_display_version":"Unknown"}}'

And the client (Qt app in debug mode) logs to terminal:

qml: Attempted to delete an entry from an invalid index. Ignoring.
qml: {
  "metrics": {
    "datetime": {
      "sample.app_started": "2021-05-28T18:43:19.156-04:00"
    }
  },
  "ping_info": {
    "seq": 0,
    "start_time": "2021-05-28T18:43-04:00",
    "end_time": "2021-05-28T18:43-04:00"
  },
  "client_info": {
    "telemetry_sdk_build": "0.12.0",
    "client_id": "f9a3d823-2e0d-4407-a92e-951606ad2efb",
    "first_run_date": "2021-05-28-04:00",
    "os": "Linux",
    "os_version": "Unknown",
    "architecture": "Unknown",
    "locale": "en-US",
    "app_build": "Unknown",
    "app_display_version": "Unknown"
  }
}
qml: Some network error occurred. undefined
qml: Recoverable upload failure while attempting to send ping e575aec8-8564-42f5-9580-ce3e8fdb28b8, will retry. Error was no status.
qml: Some network error occurred. undefined
qml: Recoverable upload failure while attempting to send ping e575aec8-8564-42f5-9580-ce3e8fdb28b8, will retry. Error was no status.
qml: Some network error occurred. undefined
qml: Recoverable upload failure while attempting to send ping e575aec8-8564-42f5-9580-ce3e8fdb28b8, will retry. Error was no status.
qml: Reached maximum recoverable failures for the current uploading window. You are done.

The qml: Some network error occurred. undefined lines are due to the fact that the server force-closed the connection. The error is raised by Qt app whenever I manually spin up and shut down a server.
Any suggestion is appreciated!

[brizental]

The qml: Some network error occurred. undefined lines are due to the fact that the server force-closed the connection. The error is raised by Qt app whenever I manually spin up and shut down a server.

Hm, could this be happening because there is no response from the server to the client? Note that Glean.js expects some response from the server. The body of the request shouldn't matter, only the status code. Try adding a conn.send statement returning a response that starts with "HTTP/1.1 200 OK\n" before closing the connection. I have not tested this, but it might work.

Wondering if we should provide a test server in our development documentation. WDYT? @ChinYing-Li @Dexterp37

[Dexterp37]

Wondering if we should provide a test server in our development documentation. WDYT? @ChinYing-Li @Dexterp37

We do have a simple test server if we want to make sure the ingestion is not the offending part.

[ChinYing-Li]

I tested the code with https://github.com/mozilla/gzipServer, and it does work.
PR ready for review. Thank you for all your help!

[brizental]

Great work here, @ChinYing-Li ! Let's also add some tests to the new uploader. We need to make sure that

1. When the request is completed succesfully, the correct UploadResult is returned;
2. When the request is not completed succesfully, the correct UploadResult is returned;
3. When the request times out, the correct UploadResult is returned.

Another thing. We need to be extra sure that the promise returned by the Uploader always resolves. I notice the onabort event is not covered. Although the request unlikely to be aborted, I think it is best to make sure we cover all cases. Another option would be to resort to the onloadend event which is triggered when the request is completed withor without errors.

[ChinYing-Li]

Just to update what I've been trying to do:
I attempted to use sinon.SinonFakeServer to mock the local server, as it seems doable, according to this blog.
However, SinonFakeServer doesn't seem to intercept the xhr, and the unit tests for QtUploader always timeout (Promise not resolved).

Please let me know if you have any thought on this.
Will continue investigating!

[Dexterp37]

Just to update what I've been trying to do:
I attempted to use sinon.SinonFakeServer to mock the local server, as it seems doable, according to this blog.

Hey, our of curiosity, why are you trying to mock the server, @ChinYing-Li ?

[ChinYing-Li]

Hey, our of curiosity, why are you trying to mock the server, @ChinYing-Li ?

I thought it's better to write unit tests for QtUploader, and I think a mock server is the right way to go for unit tests. Looking at the unit tests written for BrowserUploader, I didn't really see how I can stub QtUploader.post using sinon.sandbox. Do you have any suggestion?

[brizental]

However, SinonFakeServer doesn't seem to intercept the xhr, and the unit tests for QtUploader always timeout (Promise not resolved).

The problem with the test is that it awaits on the request before calling server.respond(). Note that the request will only be resolved once the server responds, so if we await on it before prompting a resposne from the server, the request will always timeout.

What you can do is something like this:

// Make the request, but don't await on it.
const response = QtUploader.post("/hello", "");
// Tell the server to respond to pending requests
server.respond();
// Only then await on the request and assert. Now it is ok to await because we know the server will respond :)
assert.deepStrictEqual(
        await response,
        { status: status, result: UploadResultStatus.Success }
);

This is untested, so might need some tweaking. It should work though :)

Hey, our of curiosity, why are you trying to mock the server, @ChinYing-Li ?

I think it is a fine approach to provide a fake server here. I mocked fetch directly for the browser uploader tests, but that was necessary because the tests run in a Node.js environment which doesn't provide the fetch API. That is not the case for the Qt tests, Node.js does provide the XHR API.

I didn't really see how I can stub QtUploader.post using sinon.sandbox. Do you have any suggestion?

If you were to mock the request itself instead of providing a fake server, you would not mock QtUploader.post but the XHR class itself. I think the current approach is better.

[ChinYing-Li]

Unit tests for QtUploader working.
Now that I've split the unit tests for uploader into two files (to accomodate to mocha/max-top-suite rule), I think the browser/uploader tests were silently skipped. I'll look into this, and let people know once the PR is ready for review.

[brizental]

This is looking great, thanks for the hard work here! There are a just a few last nits to address, otherwise, good to go.

[brizental]

Great work here, @ChinYing-Li ! Thanks a lot, this is of great help to the project. We appreciate you taking bugs on Glean.js :)