Skip to content

Commit

Permalink
Fix "file found in bundle" metrics
Browse files Browse the repository at this point in the history
When we got a file from the cache, we were not properly recording this metric.
  • Loading branch information
Swatinem committed Feb 5, 2024
1 parent 35f1a2f commit c47708c
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 14 deletions.
30 changes: 19 additions & 11 deletions crates/symbolicator-js/src/bundle_lookup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@ use std::time::Duration;
use symbolicator_service::caches::ByteViewString;
use symbolicator_sources::RemoteFileUri;

use crate::interface::ResolvedWith;
use crate::lookup::{CachedFileEntry, FileKey};

type FileInBundleCacheInner = moka::sync::Cache<(RemoteFileUri, FileKey), CachedFileEntry>;
type FileInBundleCacheInner =
moka::sync::Cache<(RemoteFileUri, FileKey), (CachedFileEntry, ResolvedWith)>;

/// A cache that memoizes looking up files in artifact bundles.
#[derive(Clone)]
Expand All @@ -33,11 +35,11 @@ impl FileInBundleCache {
.time_to_idle(Duration::from_secs(60 * 60))
.name("file-in-bundle")
.weigher(|_k, v| {
let content_size = v
.entry
.as_ref()
.map(|cached_file| cached_file.contents.len())
.unwrap_or_default();
let content_size =
v.0.entry
.as_ref()
.map(|cached_file| cached_file.contents.len())
.unwrap_or_default();
(std::mem::size_of_val(v) + content_size)
.try_into()
.unwrap_or(u32::MAX)
Expand All @@ -55,12 +57,12 @@ impl FileInBundleCache {
&self,
bundle_uris: impl Iterator<Item = RemoteFileUri>,
mut key: FileKey,
) -> Option<CachedFileEntry> {
) -> Option<(RemoteFileUri, CachedFileEntry, ResolvedWith)> {
for bundle_uri in bundle_uris {
// XXX: this is a really horrible workaround for not being able to look up things via `(&A, &B)` instead of `&(A, B)`.
let lookup_key = (bundle_uri, key);
if let Some(file_entry) = self.cache.get(&lookup_key) {
return Some(file_entry);
if let Some((file_entry, resolved_with)) = self.cache.get(&lookup_key) {
return Some((lookup_key.0, file_entry, resolved_with));
}
key = lookup_key.1;
}
Expand All @@ -71,7 +73,13 @@ impl FileInBundleCache {
///
/// Files are inserted under a specific bundle so that e.g. files with the same
/// `abs_path` belonging to different events are disambiguated.
pub fn insert(&self, bundle_uri: &RemoteFileUri, key: &FileKey, file_entry: &CachedFileEntry) {
pub fn insert(
&self,
bundle_uri: &RemoteFileUri,
key: &FileKey,
resolved_with: ResolvedWith,
file_entry: &CachedFileEntry,
) {
let mut file_entry = file_entry.clone();
if matches!(key, FileKey::SourceMap { .. }) {
if let Ok(cached_file) = file_entry.entry.as_mut() {
Expand All @@ -80,6 +88,6 @@ impl FileInBundleCache {
}
}
let key = (bundle_uri.clone(), key.clone());
self.cache.insert(key, file_entry)
self.cache.insert(key, (file_entry, resolved_with))
}
}
24 changes: 21 additions & 3 deletions crates/symbolicator-js/src/lookup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -967,10 +967,18 @@ impl ArtifactFetcher {
}

// First see if we have already cached this file for any of this event's bundles.
if let Some(file_entry) = self
if let Some((bundle_uri, file_entry, resolved_with)) = self
.files_in_bundles
.try_get(self.artifact_bundles.keys().rev().cloned(), key.clone())
{
if let Some(Ok((_, bundle_resolved_with))) = self.artifact_bundles.get(&bundle_uri) {
self.metrics.record_file_found_in_bundle(
key.as_type(),
resolved_with,
*bundle_resolved_with,
);
}

return Some(file_entry);
}

Expand All @@ -994,7 +1002,12 @@ impl ArtifactFetcher {
entry: CachedFile::from_descriptor(key.abs_path(), descriptor),
resolved_with: ResolvedWith::DebugId,
};
self.files_in_bundles.insert(bundle_uri, key, &file_entry);
self.files_in_bundles.insert(
bundle_uri,
key,
ResolvedWith::DebugId,
&file_entry,
);
return Some(file_entry);
}
}
Expand All @@ -1020,7 +1033,12 @@ impl ArtifactFetcher {
entry: CachedFile::from_descriptor(Some(abs_path), descriptor),
resolved_with: *resolved_with,
};
self.files_in_bundles.insert(bundle_uri, key, &file_entry);
self.files_in_bundles.insert(
bundle_uri,
key,
ResolvedWith::Url,
&file_entry,
);
return Some(file_entry);
}
}
Expand Down

0 comments on commit c47708c

Please sign in to comment.