diff --git a/crates/symbolicator-js/src/lookup.rs b/crates/symbolicator-js/src/lookup.rs index 1f1f85cac..e656672c9 100644 --- a/crates/symbolicator-js/src/lookup.rs +++ b/crates/symbolicator-js/src/lookup.rs @@ -150,11 +150,16 @@ impl SourceMapLookup { let mut modules_by_abs_path = HashMap::with_capacity(modules.len()); for module in modules { let debug_id = module.debug_id.parse().ok(); + if debug_id.is_none() { + tracing::error!(debug_id = module.debug_id, "Invalid module debug ID"); + } let cached_module = SourceMapModule::new(&module.code_file, debug_id); modules_by_abs_path.insert(module.code_file.to_owned(), cached_module); } + let metrics = JsMetrics::new(scope.as_ref().parse().ok()); + let fetcher = ArtifactFetcher { objects, files_in_bundles, @@ -175,7 +180,7 @@ impl SourceMapLookup { used_artifact_bundles: Default::default(), - metrics: Default::default(), + metrics, scraping_attempts: Default::default(), }; @@ -188,8 +193,6 @@ impl SourceMapLookup { } /// Prepares the modules for processing - // This lint is currently buggy - #[allow(clippy::assigning_clones)] pub fn prepare_modules(&mut self, stacktraces: &mut [JsStacktrace]) { for stacktrace in stacktraces { for frame in &mut stacktrace.frames { @@ -563,7 +566,8 @@ impl ArtifactFetcher { // Fetch the minified file first let minified_source = self.get_file(&key).await; if minified_source.entry.is_err() { - self.metrics.record_not_found(SourceFileType::Source); + self.metrics + .record_not_found(SourceFileType::Source, debug_id.is_some()); } // Then fetch the corresponding sourcemap reference and debug_id @@ -616,7 +620,8 @@ impl ArtifactFetcher { if matches!(sourcemap.uri, CachedFileUri::Embedded) { self.metrics.record_sourcemap_not_needed(); } else if sourcemap.entry.is_err() { - self.metrics.record_not_found(SourceFileType::SourceMap); + self.metrics + .record_not_found(SourceFileType::SourceMap, debug_id.is_some()); } // Now that we (may) have both files, we can create a `SourceMapCache` for it @@ -767,7 +772,8 @@ impl ArtifactFetcher { let entry = match scraped_file { Ok(contents) => { - self.metrics.record_file_scraped(key.as_type()); + self.metrics + .record_file_scraped(key.as_type(), key.debug_id().is_some()); tracing::trace!(?key, "Found file by scraping the web"); let sourcemap_url = discover_sourcemaps_location(&contents) @@ -819,6 +825,7 @@ impl ArtifactFetcher { key.as_type(), resolved_with, *bundle_resolved_with, + key.debug_id().is_some(), ); } @@ -838,6 +845,7 @@ impl ArtifactFetcher { key.as_type(), ResolvedWith::DebugId, *bundle_resolved_with, + key.debug_id().is_some(), ); tracing::trace!(?key, "Found file in artifact bundles by debug-id"); let file_entry = CachedFileEntry { @@ -869,6 +877,7 @@ impl ArtifactFetcher { key.as_type(), ResolvedWith::Url, *resolved_with, + key.debug_id().is_some(), ); tracing::trace!(?key, url, "Found file in artifact bundles by url"); let file_entry = CachedFileEntry { diff --git a/crates/symbolicator-js/src/metrics.rs b/crates/symbolicator-js/src/metrics.rs index 996b2798c..3fedc21ef 100644 --- a/crates/symbolicator-js/src/metrics.rs +++ b/crates/symbolicator-js/src/metrics.rs @@ -45,16 +45,27 @@ pub struct JsMetrics { // Product managers are interested in these metrics as a "funnel": found_source_via_debugid: i64, - found_source_via_release: i64, - found_source_via_release_old: i64, - found_source_via_scraping: i64, - source_not_found: i64, + found_source_via_release_with_debugid: i64, + found_source_via_release_old_with_debugid: i64, + found_source_via_scraping_with_debugid: i64, + source_not_found_with_debugid: i64, + + found_source_via_release_without_debugid: i64, + found_source_via_release_old_without_debugid: i64, + found_source_via_scraping_without_debugid: i64, + source_not_found_without_debugid: i64, found_sourcemap_via_debugid: i64, - found_sourcemap_via_release: i64, - found_sourcemap_via_release_old: i64, - found_sourcemap_via_scraping: i64, - sourcemap_not_found: i64, + found_sourcemap_via_release_with_debugid: i64, + found_sourcemap_via_release_old_with_debugid: i64, + found_sourcemap_via_scraping_with_debugid: i64, + sourcemap_not_found_with_debugid: i64, + + found_sourcemap_via_release_without_debugid: i64, + found_sourcemap_via_release_old_without_debugid: i64, + found_sourcemap_via_scraping_without_debugid: i64, + sourcemap_not_found_without_debugid: i64, + sourcemap_not_needed: i64, // Engineers might also be interested in these metrics: @@ -62,22 +73,48 @@ pub struct JsMetrics { found_bundle_via_index: i64, found_bundle_via_release: i64, found_bundle_via_release_old: i64, + + had_debug_id: bool, + project_id: Option, } impl JsMetrics { - pub fn record_file_scraped(&mut self, file_ty: SourceFileType) { - if file_ty == SourceFileType::SourceMap { - self.found_sourcemap_via_scraping += 1; - } else { - self.found_source_via_scraping += 1; + pub fn new(project_id: Option) -> Self { + Self { + project_id, + ..Self::default() + } + } + + pub fn record_file_scraped(&mut self, file_ty: SourceFileType, had_debug_id: bool) { + match (file_ty, had_debug_id) { + (SourceFileType::SourceMap, true) => { + self.had_debug_id = true; + self.found_sourcemap_via_scraping_with_debugid += 1 + } + (SourceFileType::SourceMap, false) => { + self.found_sourcemap_via_scraping_without_debugid += 1 + } + (_, true) => { + self.had_debug_id = true; + self.found_source_via_scraping_with_debugid += 1; + } + (_, false) => self.found_source_via_scraping_without_debugid += 1, } } - pub fn record_not_found(&mut self, file_ty: SourceFileType) { - if file_ty == SourceFileType::SourceMap { - self.sourcemap_not_found += 1 - } else { - self.source_not_found += 1 + pub fn record_not_found(&mut self, file_ty: SourceFileType, had_debug_id: bool) { + match (file_ty, had_debug_id) { + (SourceFileType::SourceMap, true) => { + self.had_debug_id = true; + self.sourcemap_not_found_with_debugid += 1; + } + (SourceFileType::SourceMap, false) => self.sourcemap_not_found_without_debugid += 1, + (_, true) => { + self.had_debug_id = true; + self.source_not_found_with_debugid += 1; + } + (_, false) => self.source_not_found_without_debugid += 1, } } @@ -90,22 +127,51 @@ impl JsMetrics { file_ty: SourceFileType, file_identified_by: ResolvedWith, bundle_resolved_by: ResolvedWith, + had_debug_id: bool, ) { use ResolvedWith::*; use SourceFileType::*; match (file_ty, file_identified_by, bundle_resolved_by) { (SourceMap, DebugId, _) => { + self.had_debug_id = true; self.found_sourcemap_via_debugid += 1; } (SourceMap, Url, ReleaseOld) => { - self.found_sourcemap_via_release_old += 1; + if had_debug_id { + self.had_debug_id = true; + self.found_sourcemap_via_release_old_with_debugid += 1; + } else { + self.found_sourcemap_via_release_old_without_debugid += 1; + } } (SourceMap, _, _) => { - self.found_sourcemap_via_release += 1; + if had_debug_id { + self.had_debug_id = true; + self.found_sourcemap_via_release_with_debugid += 1; + } else { + self.found_sourcemap_via_release_without_debugid += 1; + } + } + (_, DebugId, _) => { + self.had_debug_id = true; + self.found_source_via_debugid += 1; + } + (_, Url, ReleaseOld) => { + if had_debug_id { + self.had_debug_id = true; + self.found_source_via_release_old_with_debugid += 1; + } else { + self.found_source_via_release_old_without_debugid += 1; + } + } + (_, _, _) => { + if had_debug_id { + self.had_debug_id = true; + self.found_source_via_release_with_debugid += 1; + } else { + self.found_source_via_release_without_debugid += 1; + } } - (_, DebugId, _) => self.found_source_via_debugid += 1, - (_, Url, ReleaseOld) => self.found_source_via_release_old += 1, - (_, _, _) => self.found_source_via_release += 1, }; match bundle_resolved_by { @@ -147,54 +213,70 @@ impl JsMetrics { aggregator.emit_count( "js.found_via_bundle_debugid", self.found_source_via_debugid, - &[("type", "source")], + &[("type", "source"), ("had_debugid", "true")], ); aggregator.emit_count( "js.found_via_bundle_url", - self.found_source_via_release, - &[("type", "source"), ("lookup", "release")], + self.found_source_via_release_with_debugid, + &[ + ("type", "source"), + ("lookup", "release"), + ("had_debugid", "true"), + ], ); aggregator.emit_count( "js.found_via_bundle_url", - self.found_source_via_release_old, - &[("type", "source"), ("lookup", "release-old")], + self.found_source_via_release_without_debugid, + &[ + ("type", "source"), + ("lookup", "release"), + ("had_debugid", "false"), + ], ); aggregator.emit_count( "js.found_via_scraping", - self.found_source_via_scraping, - &[("type", "source")], + self.found_source_via_scraping_with_debugid, + &[("type", "source"), ("had_debugid", "true")], ); aggregator.emit_count( - "js.file_not_found", - self.source_not_found, - &[("type", "source")], + "js.found_via_scraping", + self.found_source_via_scraping_without_debugid, + &[("type", "source"), ("had_debugid", "false")], ); // SourceMaps: aggregator.emit_count( "js.found_via_bundle_debugid", self.found_sourcemap_via_debugid, - &[("type", "sourcemap")], + &[("type", "sourcemap"), ("had_debugid", "true")], ); aggregator.emit_count( "js.found_via_bundle_url", - self.found_sourcemap_via_release, - &[("type", "sourcemap"), ("lookup", "release")], + self.found_sourcemap_via_release_old_with_debugid, + &[ + ("type", "sourcemap"), + ("lookup", "release-old"), + ("had_debugid", "true"), + ], ); aggregator.emit_count( "js.found_via_bundle_url", - self.found_sourcemap_via_release_old, - &[("type", "sourcemap"), ("lookup", "release-old")], + self.found_sourcemap_via_release_old_without_debugid, + &[ + ("type", "sourcemap"), + ("lookup", "release-old"), + ("had_debugid", "false"), + ], ); aggregator.emit_count( - "js.found_via_scraping", - self.found_sourcemap_via_scraping, - &[("type", "sourcemap")], + "js.file_not_found", + self.sourcemap_not_found_with_debugid, + &[("type", "sourcemap"), ("had_debugid", "true")], ); aggregator.emit_count( "js.file_not_found", - self.sourcemap_not_found, - &[("type", "sourcemap")], + self.sourcemap_not_found_without_debugid, + &[("type", "sourcemap"), ("had_debugid", "false")], ); aggregator.emit_count("js.sourcemap_not_needed", self.sourcemap_not_needed, &[]); @@ -219,6 +301,17 @@ impl JsMetrics { self.found_bundle_via_release_old, &[("method", "release-old")], ); + + // Count this project if any of its source/sourcemap files had debug ids. + // Also separately count it if such a file wasn't found. + if let Some(project_id) = self.project_id { + if self.had_debug_id { + aggregator.emit_set("js.debugid_projects", project_id, &[]); + } + if self.source_not_found_with_debugid > 0 || self.sourcemap_not_found_with_debugid > 0 { + aggregator.emit_set("js.debugid_projects_notfound", project_id, &[]); + } + } } }