Skip to content

Commit

Permalink
Bug 1605097 - Persist X-Debug-ID header and use persisted on creation…
Browse files Browse the repository at this point in the history 
… 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.
  • Loading branch information
Beatriz Rizental authored Jul 8, 2020
1 parent 22c5def commit 347dcb8
Show file tree
Hide file tree
Showing 11 changed files with 324 additions and 215 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
2 changes: 1 addition & 1 deletion glean-core/ffi/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
});
Expand Down
12 changes: 4 additions & 8 deletions glean-core/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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(),
Expand All @@ -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",
Expand Down Expand Up @@ -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
Expand Down
38 changes: 38 additions & 0 deletions glean-core/src/ping/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<JsonValue> {
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
Expand Down Expand Up @@ -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,
Expand All @@ -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) {
Expand Down
93 changes: 35 additions & 58 deletions glean-core/src/upload/directory.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<HeaderMap>);

/// Get the file name from a path as a &str.
///
Expand All @@ -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<HeaderMap> {
#[derive(Deserialize)]
struct PingMetadata {
pub headers: HeaderMap,
}

if let Ok(metadata) = serde_json::from_str::<PingMetadata>(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 {
Expand Down Expand Up @@ -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::<JsonValue>(&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.",
Expand All @@ -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<PingPayload> {
log::info!("Processing persisted pings.");

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
Loading

0 comments on commit 347dcb8

Please sign in to comment.