From 347dcb86d126aceb26aedef6968d34a7eb35e80f Mon Sep 17 00:00:00 2001 From: Beatriz Rizental Date: Wed, 8 Jul 2020 11:19:56 +0200 Subject: [PATCH] Bug 1605097 - Persist X-Debug-ID header and use persisted on creation of requests (#1042) * Persist debug view tag header on store_ping * Get persisted headers from ping file on PingRequest creation BONUS: refactor the request module to use a builder-style creational pattern for PingRequests. --- CHANGELOG.md | 1 + glean-core/ffi/src/lib.rs | 2 +- glean-core/src/lib.rs | 12 +- glean-core/src/ping/mod.rs | 38 +++++ glean-core/src/upload/directory.rs | 93 +++++------ glean-core/src/upload/mod.rs | 105 ++++++------- glean-core/src/upload/request.rs | 238 +++++++++++++++++++---------- glean-core/src/util.rs | 2 +- glean-core/tests/common/mod.rs | 28 ++-- glean-core/tests/event.rs | 2 +- glean-core/tests/ping_maker.rs | 18 ++- 11 files changed, 324 insertions(+), 215 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05e963b7a4..43ca3f7dda 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ * General * Remove locale from baseline ping. ([1609968](https://bugzilla.mozilla.org/show_bug.cgi?id=1609968), [#1016](https://github.com/mozilla/glean/pull/1016)) + * Persist X-Debug-ID header on store ping. ([1605097](https://bugzilla.mozilla.org/show_bug.cgi?id=1605097), [#1042](https://github.com/mozilla/glean/pull/1042)) * Python * BUGFIX: correctly set the `app_build` metric to the newly provided `application_build_id` initialization option. diff --git a/glean-core/ffi/src/lib.rs b/glean-core/ffi/src/lib.rs index a3107dbbaa..0c185f9cf9 100644 --- a/glean-core/ffi/src/lib.rs +++ b/glean-core/ffi/src/lib.rs @@ -460,7 +460,7 @@ pub unsafe extern "C" fn glean_process_ping_upload_response( let document_id_str = CStr::from_ptr(document_id) .to_str() .map_err(|_| glean_core::Error::utf8_error())?; - ping_uploader.process_ping_upload_response(document_id_str, status.into(), None); + ping_uploader.process_ping_upload_response(document_id_str, status.into()); }; Ok(()) }); diff --git a/glean-core/src/lib.rs b/glean-core/src/lib.rs index e2e370fc42..53d67e3ec1 100644 --- a/glean-core/src/lib.rs +++ b/glean-core/src/lib.rs @@ -504,7 +504,7 @@ impl Glean { } self.upload_manager - .process_ping_upload_response(uuid, status, self.debug_view_tag()); + .process_ping_upload_response(uuid, status); } /// Take a snapshot for the given store and optionally clear it. @@ -567,6 +567,7 @@ impl Glean { } Some(content) => { if let Err(e) = ping_maker.store_ping( + self, &doc_id, &ping.name, &self.get_data_path(), @@ -577,12 +578,7 @@ impl Glean { return Err(e.into()); } - self.upload_manager.enqueue_ping( - &doc_id, - &url_path, - content, - self.debug_view_tag(), - ); + self.upload_manager.enqueue_ping_from_file(&doc_id); log::info!( "The ping '{}' was submitted and will be sent as soon as possible", @@ -710,7 +706,7 @@ impl Glean { /// /// This will return `false` in case `value` is not a valid tag. /// - /// When the debug view tag is set pings requests receive a `X-Debug-Id` header with the value of the tag + /// When the debug view tag is set, pings are sent with a `X-Debug-ID` header with the value of the tag /// and are sent to the ["Ping Debug Viewer"](https://mozilla.github.io/glean/book/dev/core/internal/debug-pings.html). /// /// ## Arguments diff --git a/glean-core/src/ping/mod.rs b/glean-core/src/ping/mod.rs index cd0eaa360a..332b6b5a97 100644 --- a/glean-core/src/ping/mod.rs +++ b/glean-core/src/ping/mod.rs @@ -159,6 +159,39 @@ impl PingMaker { json!(map) } + /// Build the metadata JSON to be persisted with a ping. + /// + /// Currently the only type of metadata we need to persist is the value of the `X-Debug-ID` header. + /// + /// ## Arguments + /// + /// * `glean` - the Glean instance to collect metadata from. + /// + /// ## Return value + /// + /// Returns a JSON object representing the metadata that needs to be persisted with this ping. + /// + /// The structure of the metadata json is: + /// + /// ```json + /// { + /// "headers": { + /// "X-Debug-ID": "test-tag" + /// } + /// } + /// ``` + fn get_metadata(&self, glean: &Glean) -> Option { + if let Some(debug_view_tag) = glean.debug_view_tag() { + Some(json!({ + "headers": { + "X-Debug-ID": debug_view_tag, + }, + })) + } else { + None + } + } + /// Collect a snapshot for the given ping from storage and attach required meta information. /// /// ## Arguments @@ -261,6 +294,7 @@ impl PingMaker { /// Store a ping to disk in the pings directory. pub fn store_ping( &self, + glean: &Glean, doc_id: &str, ping_name: &str, data_path: &Path, @@ -282,6 +316,10 @@ impl PingMaker { file.write_all(url_path.as_bytes())?; file.write_all(b"\n")?; file.write_all(::serde_json::to_string(ping_content)?.as_bytes())?; + if let Some(metadata) = self.get_metadata(glean) { + file.write_all(b"\n")?; + file.write_all(::serde_json::to_string(&metadata)?.as_bytes())?; + } } if let Err(e) = std::fs::rename(&temp_ping_path, &ping_path) { diff --git a/glean-core/src/upload/directory.rs b/glean-core/src/upload/directory.rs index 0e8d97a2f4..e9b9afd1d4 100644 --- a/glean-core/src/upload/directory.rs +++ b/glean-core/src/upload/directory.rs @@ -9,14 +9,15 @@ use std::fs::{self, File}; use std::io::{BufRead, BufReader}; use std::path::{Path, PathBuf}; -use serde_json::Value as JsonValue; +use serde::Deserialize; use uuid::Uuid; +use super::request::HeaderMap; use crate::{DELETION_REQUEST_PINGS_DIRECTORY, PENDING_PINGS_DIRECTORY}; /// A representation of the data extracted from a ping file, -/// this will contain the document_id, path and JSON encoded body of a ping, respectively. -type PingPayload = (String, String, JsonValue); +/// this will contain the document_id, path, JSON encoded body of a ping and the persisted headers. +type PingPayload = (String, String, String, Option); /// Get the file name from a path as a &str. /// @@ -39,6 +40,26 @@ fn get_file_name_as_str(path: &Path) -> Option<&str> { } } +/// Process a ping's metadata. +/// +/// The metadata is an optional third line in the ping file, +/// currently it contains only additonal headers to be added to each ping request. +/// Therefore, we will process the contents of this line +/// and return a HeaderMap of the persisted headers. +fn process_metadata(path: &str, metadata: &str) -> Option { + #[derive(Deserialize)] + struct PingMetadata { + pub headers: HeaderMap, + } + + if let Ok(metadata) = serde_json::from_str::(metadata) { + return Some(metadata.headers); + } else { + log::warn!("Error while parsing ping metadata: {}", path); + } + None +} + /// Manages the pending pings directories. #[derive(Debug, Clone)] pub struct PingDirectoryManager { @@ -110,19 +131,16 @@ impl PingDirectoryManager { log::info!("Processing ping at: {}", path.display()); - // The way the ping file is structured, - // first line should always have the path - // and second line should have the body with the ping contents in JSON format + // The way the ping file is structured: + // first line should always have the path, + // second line should have the body with the ping contents in JSON format + // and third line might contain ping metadata e.g. additional headers. let mut lines = BufReader::new(file).lines(); - if let (Some(Ok(path)), Some(Ok(body))) = (lines.next(), lines.next()) { - if let Ok(parsed_body) = serde_json::from_str::(&body) { - return Some((document_id.to_string(), path, parsed_body)); - } else { - log::warn!( - "Error processing ping file: {}. Can't parse ping contents as JSON.", - document_id - ); - } + if let (Some(Ok(path)), Some(Ok(body)), Ok(metadata)) = + (lines.next(), lines.next(), lines.next().transpose()) + { + let headers = metadata.map(|m| process_metadata(&path, &m)).flatten(); + return Some((document_id.into(), path, body, headers)); } else { log::warn!( "Error processing ping file: {}. Ping file is not formatted as expected.", @@ -144,8 +162,8 @@ impl PingDirectoryManager { /// /// # Return value /// - /// `Vec<(String, String, JsonValue)>` - - /// a vector of tuples containing the document_id, path and body of each request. + /// `Vec<(String, String, JsonValue, HeaderMap)>` - + /// a vector of tuples containing the document_id, path, body and headers of each request. pub fn process_dir(&self) -> Vec { log::info!("Processing persisted pings."); @@ -221,8 +239,6 @@ impl PingDirectoryManager { #[cfg(test)] mod test { use std::fs::File; - use std::io::prelude::*; - use uuid::Uuid; use super::*; use crate::metrics::PingType; @@ -327,45 +343,6 @@ mod test { assert!(!wrong_contents_file_path.exists()); } - #[test] - fn non_json_ping_body_files_are_deleted_and_ignored() { - let (mut glean, dir) = new_glean(None); - - // Register a ping for testing - let ping_type = PingType::new("test", true, true, vec![]); - glean.register_ping_type(&ping_type); - - // Submit the ping to populate the pending_pings directory - glean.submit_ping(&ping_type, None).unwrap(); - - let directory_manager = PingDirectoryManager::new(&dir.path()); - - let non_json_body_file_path = dir - .path() - .join(PENDING_PINGS_DIRECTORY) - .join(Uuid::new_v4().to_string()); - let mut non_json_body_file = File::create(&non_json_body_file_path).unwrap(); - non_json_body_file - .write_all( - b"https://doc.rust-lang.org/std/fs/struct.File.html - This is not JSON!!!!", - ) - .unwrap(); - - // Try and process the pings folder - let data = directory_manager.process_dir(); - - // Verify there is just the one request - assert_eq!(data.len(), 1); - - // Verify request was returned for the "test" ping - let request_ping_type = data[0].1.split('/').nth(3).unwrap(); - assert_eq!(request_ping_type, "test"); - - // Verify that file was indeed deleted - assert!(!non_json_body_file_path.exists()); - } - #[test] fn takes_deletion_request_pings_into_account_while_processing() { let (glean, dir) = new_glean(None); diff --git a/glean-core/src/upload/mod.rs b/glean-core/src/upload/mod.rs index 6c85f4984b..5bea989eb9 100644 --- a/glean-core/src/upload/mod.rs +++ b/glean-core/src/upload/mod.rs @@ -17,11 +17,10 @@ use std::thread; use std::time::{Duration, Instant}; use once_cell::sync::OnceCell; -use serde_json::Value as JsonValue; use crate::error::Result; use directory::PingDirectoryManager; -pub use request::PingRequest; +pub use request::{HeaderMap, PingRequest}; pub use result::{ffi_upload_result, UploadResult}; mod directory; @@ -205,18 +204,18 @@ impl PingUploadManager { let mut local_queue = local_queue .write() .expect("Can't write to pending pings queue."); - for (document_id, path, body) in local_manager.process_dir() { + for (document_id, path, body, headers) in local_manager.process_dir() { if Self::is_enqueued(&local_queue, &document_id) { continue; } - let request = PingRequest::new( - &document_id, - &path, - body, - &local_language_binding_name, - None, - ); - local_queue.push_back(request); + let mut request = PingRequest::builder(&local_language_binding_name) + .document_id(document_id) + .path(path) + .body(body); + if let Some(headers) = headers { + request = request.headers(headers); + } + local_queue.push_back(request.build()); } local_flag.store(true, Ordering::SeqCst); }) @@ -266,23 +265,14 @@ impl PingUploadManager { ))); } - /// Creates a `PingRequest` and adds it to the queue. - /// - /// Duplicate requests won't be added. - pub fn enqueue_ping( - &self, - document_id: &str, - path: &str, - body: JsonValue, - debug_view_tag: Option<&String>, - ) { + fn enqueue_ping(&self, document_id: &str, path: &str, body: &str, headers: Option) { let mut queue = self .queue .write() .expect("Can't write to pending pings queue."); - // Checks if a ping with a certain `document_id` is already enqueued. - if Self::is_enqueued(&queue, document_id) { + // Checks if a ping with this `document_id` is already enqueued. + if Self::is_enqueued(&queue, &document_id) { log::trace!( "Attempted to enqueue a duplicate ping {} at {}.", document_id, @@ -292,14 +282,30 @@ impl PingUploadManager { } log::trace!("Enqueuing ping {} at {}", document_id, path); - let request = PingRequest::new( - &document_id, - &path, - body, - self.language_binding_name.as_str(), - debug_view_tag, - ); - queue.push_back(request); + let mut request = PingRequest::builder(&self.language_binding_name) + .document_id(document_id) + .path(path) + .body(body); + if let Some(headers) = headers { + request = request.headers(headers); + } + + queue.push_back(request.build()); + } + + /// Reads a ping file, creates a `PingRequest` and adds it to the queue. + /// + /// Duplicate requests won't be added. + /// + /// # Arguments + /// + /// * `document_id` - The UUID of the ping in question. + pub fn enqueue_ping_from_file(&self, document_id: &str) { + if let Some((doc_id, path, body, headers)) = + self.directory_manager.process_file(document_id) + { + self.enqueue_ping(&doc_id, &path, &body, headers) + } } /// Clears the pending pings queue, leaves the deletion-request pings. @@ -413,13 +419,7 @@ impl PingUploadManager { /// /// `document_id` - The UUID of the ping in question. /// `status` - The HTTP status of the response. - /// `debug_view_tag` - The value of the `X-Debug-Id` header, if this is `None` the header is not added. - pub fn process_ping_upload_response( - &self, - document_id: &str, - status: UploadResult, - debug_view_tag: Option<&String>, - ) { + pub fn process_ping_upload_response(&self, document_id: &str, status: UploadResult) { use UploadResult::*; match status { HttpStatus(status @ 200..=299) => { @@ -442,11 +442,7 @@ impl PingUploadManager { document_id, status ); - if let Some((document_id, path, body)) = - self.directory_manager.process_file(document_id) - { - self.enqueue_ping(&document_id, &path, body, debug_view_tag); - } + self.enqueue_ping_from_file(&document_id); } }; } @@ -523,7 +519,6 @@ mod test { use std::thread; use std::time::Duration; - use serde_json::json; use uuid::Uuid; use super::UploadResult::*; @@ -561,7 +556,7 @@ mod test { } // Enqueue a ping - upload_manager.enqueue_ping(&Uuid::new_v4().to_string(), PATH, json!({}), None); + upload_manager.enqueue_ping(&Uuid::new_v4().to_string(), PATH, "", None); // Try and get the next request. // Verify request was returned @@ -585,7 +580,7 @@ mod test { // Enqueue a ping multiple times let n = 10; for _ in 0..n { - upload_manager.enqueue_ping(&Uuid::new_v4().to_string(), PATH, json!({}), None); + upload_manager.enqueue_ping(&Uuid::new_v4().to_string(), PATH, "", None); } // Verify a request is returned for each submitted ping @@ -618,7 +613,7 @@ mod test { // Enqueue a ping multiple times for _ in 0..max_pings_per_interval { - upload_manager.enqueue_ping(&Uuid::new_v4().to_string(), PATH, json!({}), None); + upload_manager.enqueue_ping(&Uuid::new_v4().to_string(), PATH, "", None); } // Verify a request is returned for each submitted ping @@ -631,7 +626,7 @@ mod test { // Enqueue just one more ping. // We should still be within the default rate limit time. - upload_manager.enqueue_ping(&Uuid::new_v4().to_string(), PATH, json!({}), None); + upload_manager.enqueue_ping(&Uuid::new_v4().to_string(), PATH, "", None); // Verify that we are indeed told to wait because we are at capacity assert_eq!(PingUploadTask::Wait, upload_manager.get_upload_task(false)); @@ -657,7 +652,7 @@ mod test { // Enqueue a ping multiple times for _ in 0..10 { - upload_manager.enqueue_ping(&Uuid::new_v4().to_string(), PATH, json!({}), None); + upload_manager.enqueue_ping(&Uuid::new_v4().to_string(), PATH, "", None); } // Clear the queue @@ -905,7 +900,7 @@ mod test { let path2 = format!("/submit/app_id/test-ping/1/{}", doc2); // Enqueue a ping - upload_manager.enqueue_ping(&doc1, &path1, json!({}), None); + upload_manager.enqueue_ping(&doc1, &path1, "", None); // Try and get the first request. let req = match upload_manager.get_upload_task(false) { @@ -915,10 +910,10 @@ mod test { assert_eq!(doc1, req.document_id); // Schedule the next one while the first one is "in progress" - upload_manager.enqueue_ping(&doc2, &path2, json!({}), None); + upload_manager.enqueue_ping(&doc2, &path2, "", None); // Mark as processed - upload_manager.process_ping_upload_response(&req.document_id, HttpStatus(200), None); + upload_manager.process_ping_upload_response(&req.document_id, HttpStatus(200)); // Get the second request. let req = match upload_manager.get_upload_task(false) { @@ -928,7 +923,7 @@ mod test { assert_eq!(doc2, req.document_id); // Mark as processed - upload_manager.process_ping_upload_response(&req.document_id, HttpStatus(200), None); + upload_manager.process_ping_upload_response(&req.document_id, HttpStatus(200)); // ... and then we're done. assert_eq!(upload_manager.get_upload_task(false), PingUploadTask::Done); @@ -987,8 +982,8 @@ mod test { let path = format!("/submit/app_id/test-ping/1/{}", doc_id); // Try to enqueue a ping with the same doc_id twice - upload_manager.enqueue_ping(&doc_id, &path, json!({}), None); - upload_manager.enqueue_ping(&doc_id, &path, json!({}), None); + upload_manager.enqueue_ping(&doc_id, &path, "", None); + upload_manager.enqueue_ping(&doc_id, &path, "", None); // Get a task once match upload_manager.get_upload_task(false) { diff --git a/glean-core/src/upload/request.rs b/glean-core/src/upload/request.rs index 0b0ef6a492..f9eb0badc6 100644 --- a/glean-core/src/upload/request.rs +++ b/glean-core/src/upload/request.rs @@ -13,6 +13,9 @@ use std::io::prelude::*; use crate::system; +/// A representation for request headers. +pub type HeaderMap = HashMap; + /// Creates a formatted date string that can be used with Date headers. fn create_date_header_value(current_time: DateTime) -> String { // Date headers are required to be in the following format: @@ -39,6 +42,135 @@ fn create_user_agent_header_value( ) } +/// Attempt to gzip the contents of a ping. +fn gzip_content(path: &str, content: &[u8]) -> Option> { + let mut gzipper = GzEncoder::new(Vec::new(), Compression::default()); + + // Attempt to add the content to the gzipper. + if let Err(e) = gzipper.write_all(content) { + log::error!("Failed to write to the gzipper: {} - {:?}", path, e); + return None; + } + + gzipper.finish().ok() +} + +pub struct Builder { + document_id: Option, + path: Option, + body: Option>, + headers: HeaderMap, +} + +impl Builder { + /// Creates a new builder for a PingRequest. + pub fn new(language_binding_name: &str) -> Self { + let mut headers = HashMap::new(); + headers.insert("Date".to_string(), create_date_header_value(Utc::now())); + headers.insert( + "User-Agent".to_string(), + create_user_agent_header_value(crate::GLEAN_VERSION, language_binding_name, system::OS), + ); + headers.insert( + "Content-Type".to_string(), + "application/json; charset=utf-8".to_string(), + ); + headers.insert("X-Client-Type".to_string(), "Glean".to_string()); + headers.insert( + "X-Client-Version".to_string(), + crate::GLEAN_VERSION.to_string(), + ); + + Self { + document_id: None, + path: None, + body: None, + headers, + } + } + + /// Sets the document_id for this request. + pub fn document_id>(mut self, value: S) -> Self { + self.document_id = Some(value.into()); + self + } + + /// Sets the path for this request. + pub fn path>(mut self, value: S) -> Self { + self.path = Some(value.into()); + self + } + + /// Sets the body for this request. + /// + /// This method will also attempt to gzip the body contents + /// and add headers related to the body that was just added. + /// + /// Namely these headers are the "Content-Length" with the length of the body + /// and in case we are successfull on gzipping the contents, the "Content-Encoding"="gzip". + /// + /// **Important** + /// If we are unable to gzip we don't panic and instead just set the uncompressed body. + /// + /// # Panics + /// + /// This method will panic in case we try to set the body before setting the path. + pub fn body>(mut self, value: S) -> Self { + // Attempt to gzip the body contents. + let original_as_string = value.into(); + let gzipped_content = gzip_content( + self.path + .as_ref() + .expect("Path must be set before attempting to set the body"), + original_as_string.as_bytes(), + ); + let add_gzip_header = gzipped_content.is_some(); + let body = gzipped_content.unwrap_or_else(|| original_as_string.into_bytes()); + + // Include headers related to body + self = self.header("Content-Length", &body.len().to_string()); + if add_gzip_header { + self = self.header("Content-Encoding", "gzip"); + } + + self.body = Some(body); + self + } + + /// Sets a header for this request. + pub fn header>(mut self, key: S, value: S) -> Self { + self.headers.insert(key.into(), value.into()); + self + } + + /// Sets multiple headers for this request at once. + pub fn headers(mut self, values: HeaderMap) -> Self { + self.headers.extend(values); + self + } + + /// Consume the builder and create a PingRequest. + /// + /// # Panics + /// + /// This method will panic if any of the required fields are missing: + /// `document_id`, `path` and `body`. + pub fn build(self) -> PingRequest { + PingRequest { + document_id: self + .document_id + .expect("document_id must be set before attempting to build PingRequest"), + path: self + .path + .expect("path must be set before attempting to build PingRequest"), + body: self + .body + .expect("body must be set before attempting to build PingRequest"), + headers: self.headers, + } + } +} + /// Represents a request to upload a ping. #[derive(PartialEq, Debug, Clone)] pub struct PingRequest { @@ -52,46 +184,18 @@ pub struct PingRequest { /// the value `gzip`. pub body: Vec, /// A map with all the headers to be sent with the request. - pub headers: HashMap<&'static str, String>, + pub headers: HeaderMap, } impl PingRequest { - /// Creates a new PingRequest. - /// - /// Automatically creates the default request headers. + /// Creates a new builder-style structure to help build a PingRequest. /// /// ## Arguments /// - /// * `document_id` - The UUID of the ping in question. - /// * `path` - The path to upload this ping to. The format should be `/submit///`. - /// * `body` - A JSON object with the contents of the ping in question. - /// * `debug_view_tag` - The value of the `X-Debug-Id` header, if this is `None` the header is not added. - pub fn new( - document_id: &str, - path: &str, - body: JsonValue, - language_binding_name: &str, - debug_view_tag: Option<&String>, - ) -> Self { - // We want uploads to be gzip'd. Instead of doing this for each platform - // we have language bindings for, apply compression here. - let original_as_string = body.to_string(); - let gzipped_content = Self::gzip_content(path, original_as_string.as_bytes()); - let add_gzip_header = gzipped_content.is_some(); - let body = gzipped_content.unwrap_or_else(|| original_as_string.into_bytes()); - let body_len = body.len(); - - Self { - document_id: document_id.into(), - path: path.into(), - body, - headers: Self::create_request_headers( - add_gzip_header, - body_len, - language_binding_name, - debug_view_tag, - ), - } + /// * `language_binding_name` - The name of the language used by the binding that instantiated this Glean instance. + /// This is used to build the User-Agent header value. + pub fn builder(language_binding_name: &str) -> Builder { + Builder::new(language_binding_name) } /// Verifies if current request is for a deletion-request ping. @@ -119,48 +223,6 @@ impl PingRequest { .and_then(|payload| serde_json::from_str::(payload).ok()) .and_then(|json| serde_json::to_string_pretty(&json).ok()) } - - /// Attempt to gzip the provided ping content. - fn gzip_content(path: &str, content: &[u8]) -> Option> { - let mut gzipper = GzEncoder::new(Vec::new(), Compression::default()); - - // Attempt to add the content to the gzipper. - if let Err(e) = gzipper.write_all(content) { - log::error!("Failed to write to the gzipper: {} - {:?}", path, e); - return None; - } - - gzipper.finish().ok() - } - - /// Creates the default request headers. - fn create_request_headers( - is_gzipped: bool, - body_len: usize, - language_binding_name: &str, - debug_view_tag: Option<&String>, - ) -> HashMap<&'static str, String> { - let mut headers = HashMap::new(); - headers.insert("Date", create_date_header_value(Utc::now())); - headers.insert( - "User-Agent", - create_user_agent_header_value(crate::GLEAN_VERSION, language_binding_name, system::OS), - ); - headers.insert("X-Client-Type", "Glean".to_string()); - headers.insert( - "Content-Type", - "application/json; charset=utf-8".to_string(), - ); - headers.insert("Content-Length", body_len.to_string()); - if is_gzipped { - headers.insert("Content-Encoding", "gzip".to_string()); - } - headers.insert("X-Client-Version", crate::GLEAN_VERSION.to_string()); - if let Some(tag) = debug_view_tag { - headers.insert("X-Debug-ID", tag.clone()); - } - headers - } } #[cfg(test)] @@ -169,15 +231,35 @@ mod test { use chrono::offset::TimeZone; #[test] - fn test_date_header_resolution() { + fn date_header_resolution() { let date: DateTime = Utc.ymd(2018, 2, 25).and_hms(11, 10, 37); let test_value = create_date_header_value(date); assert_eq!("Sun, 25 Feb 2018 11:10:37 GMT", test_value); } #[test] - fn test_user_agent_header_resolution() { + fn user_agent_header_resolution() { let test_value = create_user_agent_header_value("0.0.0", "Rust", "Windows"); assert_eq!("Glean/0.0.0 (Rust on Windows)", test_value); } + + #[test] + fn correctly_builds_ping_request() { + let request = PingRequest::builder(/* language_binding_name */ "Rust") + .document_id("woop") + .path("/random/path/doesnt/matter") + .body("{}") + .build(); + + assert_eq!(request.document_id, "woop"); + assert_eq!(request.path, "/random/path/doesnt/matter"); + + // Make sure all the expected headers were added. + assert!(request.headers.contains_key("Date")); + assert!(request.headers.contains_key("User-Agent")); + assert!(request.headers.contains_key("Content-Type")); + assert!(request.headers.contains_key("X-Client-Type")); + assert!(request.headers.contains_key("X-Client-Version")); + assert!(request.headers.contains_key("Content-Length")); + } } diff --git a/glean-core/src/util.rs b/glean-core/src/util.rs index 18895b54b1..575fec7d5f 100644 --- a/glean-core/src/util.rs +++ b/glean-core/src/util.rs @@ -179,7 +179,7 @@ pub mod floating_point_context { } } -/// The debug view tag is the value for the `X-Debug-Id` header of tagged ping requests, +/// The debug view tag is the value for the `X-Debug-ID` header of tagged ping requests, /// thus is it must be a valid header value. /// /// In other words, it must match the regex: "[a-zA-Z0-9-]{1,20}" diff --git a/glean-core/tests/common/mod.rs b/glean-core/tests/common/mod.rs index e2fc2dfa81..514e5dde34 100644 --- a/glean-core/tests/common/mod.rs +++ b/glean-core/tests/common/mod.rs @@ -86,10 +86,10 @@ pub fn iso8601_to_chrono(datetime: &iso8601::DateTime) -> chrono::DateTime Result> { +/// A vector of all queued pings. Each entry is a pair `(url, json_data, metadata)`, where +/// `url` is the endpoint the ping will go to, `json_data` is the JSON +/// payload and metadata is optional persisted data related to the ping. +pub fn get_queued_pings(data_path: &Path) -> Result)>> { get_pings(&data_path.join("pending_pings")) } @@ -101,13 +101,14 @@ pub fn get_queued_pings(data_path: &Path) -> Result> { /// /// # Returns /// -/// A vector of all queued `deletion-request` pings. Each entry is a pair `(url, json_data)`, -/// where `url` is the endpoint the ping will go to, and `json_data` is the JSON payload. -pub fn get_deletion_pings(data_path: &Path) -> Result> { +/// A vector of all queued `deletion-request` pings. Each entry is a pair `(url, json_body, metadata)`, +/// where `url` is the endpoint the ping will go to, `json_body` is the JSON payload +/// and metadata is optional persisted data related to the ping. +pub fn get_deletion_pings(data_path: &Path) -> Result)>> { get_pings(&data_path.join("deletion_request")) } -fn get_pings(pings_dir: &Path) -> Result> { +fn get_pings(pings_dir: &Path) -> Result)>> { let entries = read_dir(pings_dir)?; Ok(entries .filter_map(|entry| entry.ok()) @@ -118,9 +119,14 @@ fn get_pings(pings_dir: &Path) -> Result> { .filter_map(|entry| File::open(entry.path()).ok()) .filter_map(|file| { let mut lines = BufReader::new(file).lines(); - if let (Some(Ok(url)), Some(Ok(json))) = (lines.next(), lines.next()) { - if let Ok(parsed_json) = serde_json::from_str::(&json) { - Some((url, parsed_json)) + if let (Some(Ok(url)), Some(Ok(body)), Ok(metadata)) = + (lines.next(), lines.next(), lines.next().transpose()) + { + let parsed_metadata = metadata.map(|m| { + serde_json::from_str::(&m).expect("metadata should be valid JSON") + }); + if let Ok(parsed_body) = serde_json::from_str::(&body) { + Some((url, parsed_body, parsed_metadata)) } else { None } diff --git a/glean-core/tests/event.rs b/glean-core/tests/event.rs index fe5ba1e81b..14cf0d0c8c 100644 --- a/glean-core/tests/event.rs +++ b/glean-core/tests/event.rs @@ -177,7 +177,7 @@ fn test_sending_of_event_ping_when_it_fills_up() { assert_eq!(10, click.test_get_value(&glean, "events").unwrap().len()); - let (url, json) = &get_queued_pings(glean.get_data_path()).unwrap()[0]; + let (url, json, _) = &get_queued_pings(glean.get_data_path()).unwrap()[0]; assert!(url.starts_with(format!("/submit/{}/events/", glean.get_application_id()).as_str())); assert_eq!(500, json["events"].as_array().unwrap().len()); assert_eq!( diff --git a/glean-core/tests/ping_maker.rs b/glean-core/tests/ping_maker.rs index 2382ad2e63..436e38e711 100644 --- a/glean-core/tests/ping_maker.rs +++ b/glean-core/tests/ping_maker.rs @@ -147,7 +147,7 @@ fn seq_number_must_be_sequential() { } #[test] -fn test_clear_pending_pings() { +fn clear_pending_pings() { let (mut glean, _) = new_glean(None); let ping_maker = PingMaker::new(); let ping_type = PingType::new("store1", true, false, vec![]); @@ -174,7 +174,7 @@ fn test_clear_pending_pings() { } #[test] -fn test_no_pings_submitted_if_upload_disabled() { +fn no_pings_submitted_if_upload_disabled() { // Regression test, bug 1603571 let (mut glean, _) = new_glean(None); @@ -194,3 +194,17 @@ fn test_no_pings_submitted_if_upload_disabled() { assert!(ping_type.submit(&glean, None).is_ok()); assert_eq!(0, get_queued_pings(glean.get_data_path()).unwrap().len()); } + +#[test] +fn metadata_is_correctly_added_when_necessary() { + let (mut glean, _) = new_glean(None); + glean.set_debug_view_tag("valid-tag"); + let ping_type = PingType::new("store1", true, true, vec![]); + glean.register_ping_type(&ping_type); + + assert!(glean.submit_ping(&ping_type, None).is_ok()); + + let (_, _, metadata) = &get_queued_pings(glean.get_data_path()).unwrap()[0]; + let headers = metadata.as_ref().unwrap().get("headers").unwrap(); + assert_eq!(headers.get("X-Debug-ID").unwrap(), "valid-tag"); +}