1623304 - Ping upload using the Firefox network stack

Status: RESOLVED FIXED

(Toolkit :: Telemetry, task, P3)

Description

[Jan-Erik Rediger [:janerik]]

FOG needs to upload pings after submission.
The Upload should use the Firefox network stack to do this.
This requires the new upload manager to land.

(the "depends on" bug is closed, but the changes are not in master yet)

Comment 1

[Alessio Placitelli [:Dexter]]

I just had a chat with :baku for other reasons, he suggested looking at fetch and fetchObserver (usable in C++ as well, example) instead of using XHR for new code.

Comment 2

[Chris H-C :chutten]

C++ via getting a JS Context, I notice : |

I wonder if there's a friendly and direct networking API in native that we can more easily use without nsI* or JSContext gymnastics in our Rust layer...

Comment 3

[Chris H-C :chutten]

Getting a little more detailed with the requirements, this shall:

1. Send pings when it is pleasant to do so. Scheduling is opportunistic and asynchronous and perhaps on idle. Especially since networking things might have to be main-thread.
2. Control some headers
   • We do not want anything hitting this endpoint that doesn't need to. This especially means Cookie headers, but extends to anything outside of Content-Type (json), Content-Encoding (gzip, please), Date, User-Agent (useful if something goes wrong), and the basic protocol-level stuff (looking at you, Host)
   • We do want to add some extra things for our own purposes. They all start with X- so I don't expect that to be troubling.

Comment 4

[Chris H-C :chutten]

While waiting for the dependencies to land I explored the problem space a bit and learned some things:

Viaduct

• The viaduct crate should probably re-export url::Url so that we don't have to match their url version. I filed an Issue
• It's not strictly clear who should be initializing the backend or how. I went the route of adding some JS (let viaduct = Cc["@mozilla.org/toolkit/viaduct;1"].createInstance(Ci.mozIViaduct); viaduct.Initialize();) right before we call in to init nsITelemetry (which is where we're currently init-ing FOG). I'm using the version of the viaduct impl that is currently up for review in bug 1628068 so I'm way too early to be asking questions like these... but it might still reveal that there's a timing thing we should pay attention to.

Glean SDK

• Getting the correct version of glean_core was a little tricky because I needed to specify it on both the fog and glean crates (in t/c/g/ and t/c/g/api/ respectively). I wonder if there's anything we can do about that. And in the future when we'll be depending on the glean_sdk crate for the Rust Specific Metrics API, what's to keep us from getting out of sync and accidentally vendoring 2 versions of glean_core?
• The ping upload changes aren't usable from Rust at the moment because PingRequest isn't exported. See bug 1634745
• Where exactly should we be calling glean.on_ready_to_submit_pings()? Filed bug 1634754 for the documentation clarification, but in concrete terms I guess we should be calling it... in api::initialize?

Scheduling

• viaduct asserts if we try to do (sync) networking on the main thread (good!).
• There's nothing stopping us from using std::thread (which is what I did in my exploration), but as :lina notes it only works for us until we want to dispatch runnables in which case we should use the quite tolerable-looking nsIThread stuff that rkv uses. And I can't help but notice a RefPtr<nsIThread> could sit nicely inside AppState.
  • Dumping to a bg thread like this is enough for now but I wonder if we'll ever in the future need to look into scheduling on idle. For now let's try our best not to schedule anything on a timer and instead allow ourselves to be triggered by activity elsewhere in the process (by say Ping::submit and startup? And IPC? Whatever we choose we must remember to document.)

Anyhoo, that's as far as I can take this for now. Time to de-prioritize and unassign until we see some action in the dependencies.

Comment 5

[Chris H-C :chutten]

Attachment: Bug 1623304 - Ensure Viaduct is initialized for FOG r?janerik!

Comment 6

[Chris H-C :chutten]

Attachment: Bug 1623304 - Add logic for a Ping Scheduler r?janerik!

Depends on D76747

Comment 7

[Chris H-C :chutten]

Attachment: Bug 1623304 - Add viaduct (and deps) to fog crate r?janerik!

Depends on D76748

Comment 8

[Chris H-C :chutten]

Attachment: Bug 1623304 - Upload FOG pings using Viaduct. r?janerik!

Depends on D76749

Comment 9

[Chris H-C :chutten]

Depends on getting glean_sdk to 30.1.0 so we can have a Vec<u8> out of the ping request.

Comment 10

[Chris H-C :chutten]

Current testing method is the following patch:

diff --git a/toolkit/components/glean/src/lib.rs b/toolkit/components/glean/src/lib.rs
index 4c8119a20dc0..53e30bfa722a 100644
--- a/toolkit/components/glean/src/lib.rs
+++ b/toolkit/components/glean/src/lib.rs
@@ -102,5 +102,12 @@ pub unsafe extern "C" fn fog_init(
         }
     }
 
+    // DO NOT MERGE
+    log::error!("Testing ping sending");
+    let ping = glean::metrics::Ping::new("ping_name", false, true, vec![]);
+    let success = ping.submit(None);
+    log::error!("Ping success? {}", success);
+    // DO NOT MERGE
+
     NS_OK
 }

diff --git a/toolkit/components/glean/src/api.rs b/toolkit/components/glean/src/api.rs
index a700df32a520..8d45ba404442 100644
--- a/toolkit/components/glean/src/api.rs
+++ b/toolkit/components/glean/src/api.rs
@@ -147,6 +147,7 @@ fn register_uploader() {
             const SERVER: &str = "https://incoming.telemetry.mozilla.org";
             let url = Url::parse(SERVER)?.join(&ping_request.path)?;
             let mut req = Request::post(url);
+            req = req.header("X-Debug-ID", "chutten_fog_test")?;
             for (&header_key, header_value) in ping_request.headers.iter() {
                 req = req.header(header_key, header_value)?;
             }

Apply the patch, build and run Firefox, then check the Debug Ping Viewer.

Currently the pings come up empty (didn't add the PingRequest's body to the viaduct Request.), but if they come up at all then it's working.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1845fca9e3ec
Ensure Viaduct is initialized for FOG r=janerik
https://hg.mozilla.org/integration/autoland/rev/932d51ae751e
Add logic for a Ping Scheduler r=janerik
https://hg.mozilla.org/integration/autoland/rev/e7470ab5ae58
Add viaduct (and deps) to fog crate r=janerik
https://hg.mozilla.org/integration/autoland/rev/28e076bc443b
Upload FOG pings using Viaduct. r=janerik

Comment 12

[Cristian Brindusan [:cbrindusan]]

Backed out 4 changesets (Bug 1623304) for android build bustages.
https://hg.mozilla.org/integration/autoland/rev/88d6b81695fa03273de9cc46f5a579bf2bacab32

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=28e076bc443b0995390080aa5770fd170052407b&selectedTaskRun=LvnAdAgYT5GNYfb2cHxwhA-0

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304754297&repo=autoland&lineNumber=24308

Comment 13

[Chris H-C :chutten]

Attachment: Bug 1623304 - For now don't build FOG on Android r?TravisLong!

We will eventually have a solution for what FOG does for Gecko when not in
Firefox Desktop, but for now skip the Android question entirely.

Depends on D76750

Comment 14

[Chris H-C :chutten]

In the not-too-distant future we'll need to support configurations where we still build FOG into Gecko and not initialize it or its networking underpinnings. But for now we'll just not build anything at all on Android.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b0d93fa2118
Ensure Viaduct is initialized for FOG r=janerik
https://hg.mozilla.org/integration/autoland/rev/eac8f459598f
Add logic for a Ping Scheduler r=janerik
https://hg.mozilla.org/integration/autoland/rev/b878e891386f
Add viaduct (and deps) to fog crate r=janerik
https://hg.mozilla.org/integration/autoland/rev/b68cb2878c3c
Upload FOG pings using Viaduct. r=janerik
https://hg.mozilla.org/integration/autoland/rev/39e6ee02e1dd
For now don't build FOG on Android r=TravisLong

Comment 16

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/0b0d93fa2118
https://hg.mozilla.org/mozilla-central/rev/eac8f459598f
https://hg.mozilla.org/mozilla-central/rev/b878e891386f
https://hg.mozilla.org/mozilla-central/rev/b68cb2878c3c
https://hg.mozilla.org/mozilla-central/rev/39e6ee02e1dd