Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

telemetry ids should be obscured in log #57

Merged
merged 3 commits into from
Aug 26, 2022
Merged

telemetry ids should be obscured in log #57

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d13e408
Redact unique id
Hoverbear Aug 24, 2022
98593be
fmt
Hoverbear Aug 24, 2022
084cb3e
Merge branch 'main' into hoverbear/ds-267-telemetry-ids-should-be-obs…
Hoverbear Aug 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions src/dependency_registry/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,7 @@ impl DependencyRegistry {
// Refresh the cache
// We don't want to fail if we can't build telemetry data...
let maybe_telemetry = if let Some(telemetry) = telemetry_handle {
match telemetry
.await
.map_err(|v| eyre!(v))
.and_then(|v| v.as_header_data().map_err(|e| eyre!(e)))
{
match telemetry.await.map_err(|v| eyre!(v)) {
Ok(telemetry) => Some(telemetry), // But we do want to fail if we can build it but can't parse it
Err(err) => {
tracing::debug!(%err, "Telemetry build error");
Expand All @@ -102,8 +98,15 @@ impl DependencyRegistry {
let http_client = reqwest::Client::new();
let mut req = http_client.get(DEPENDENCY_REGISTRY_REMOTE_URL);
if let Some(telemetry) = maybe_telemetry {
req = req.header(TELEMETRY_HEADER_NAME, &telemetry);
tracing::trace!(%telemetry, "Fetching new registry data from {DEPENDENCY_REGISTRY_REMOTE_URL}");
match telemetry.as_header_data() {
Ok(header_data) => {
req = req.header(TELEMETRY_HEADER_NAME, &header_data);
tracing::trace!(telemetry = %telemetry.redact_header_data(header_data), "Fetching new registry data from {DEPENDENCY_REGISTRY_REMOTE_URL}");
}
Err(err) => {
tracing::debug!(err = %eyre!(err), "Failed to serialize header data, skipping it")
}
};
} else {
tracing::trace!("Fetching new registry data from {DEPENDENCY_REGISTRY_REMOTE_URL}");
}
Expand Down
13 changes: 11 additions & 2 deletions src/telemetry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,22 +104,31 @@ impl Telemetry {
self
}

#[tracing::instrument(skip_all)]
pub(crate) async fn send(&self) -> eyre::Result<Response> {
let header_data = self.as_header_data()?;
tracing::trace!(data = %header_data, "Sending telemetry data to {TELEMETRY_REMOTE_URL}");
tracing::trace!(data = %self.redact_header_data(header_data.clone()), "Sending telemetry data to {TELEMETRY_REMOTE_URL}");
let http_client = reqwest::Client::new();
let req = http_client
.post(TELEMETRY_REMOTE_URL)
.header(TELEMETRY_HEADER_NAME, &header_data)
.timeout(Duration::from_millis(250));
let res = req.send().await?;
tracing::debug!(telemetry = %header_data, "Sent telemetry data to {TELEMETRY_REMOTE_URL}");
tracing::debug!(telemetry = %self.redact_header_data(header_data.clone()), "Sent telemetry data to {TELEMETRY_REMOTE_URL}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be worth it to manually implement whatever this (%) is using (Display? Debug? maybe even both?) and redact the distinct ID there (which would necessitate a type / newtype wrapper for it, probably).

Otherwise, if we ever print out this header_data somewhere else, we'll have to remember this function exists and to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

% is shorthand for Display. ? is shorthand for debug.

I played with wrapping the type a bit yeah. tokio-rs/valuable#75 would make this quite easy. 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened https://linear.app/detsys/issue/DS-273/fsm-encode-the-obscured-telemetry-distinct-id-in-the-structs-type so that we can tackle this separately.

Ok(res)
}

pub(crate) fn as_header_data(&self) -> Result<String, serde_json::Error> {
serde_json::to_string(&self)
}

pub(crate) fn redact_header_data(&self, mut val: String) -> String {
if let Some(distinct_id) = self.distinct_id {
let distinct_id_string = distinct_id.to_string();
val = val.replace(&distinct_id_string, "<redacted>");
}
val
}
}

async fn distinct_id() -> eyre::Result<Uuid> {
Expand Down