Stage 2

tzanko-matev · tzanko-matev · commit 573fb78e4ef4 · 2025-10-27T15:44:57.000+02:00
diff --git a/codetracer-python-recorder/benches/trace_filter.rs b/codetracer-python-recorder/benches/trace_filter.rs
@@ -52,7 +52,7 @@ fn run_workload(engine: &TraceFilterEngine, dataset: &WorkloadDataset) {
         for &index in &dataset.event_indices {
             let code = dataset.codes[index].as_ref();
             let resolution = engine
-                .resolve(py, code)
+                .resolve(py, code, None)
                 .expect("trace filter resolution should succeed during benchmarking");
             let policy = resolution.value_policy();
             for name in dataset.locals.iter() {
@@ -66,7 +66,7 @@ fn prewarm_engine(engine: &TraceFilterEngine, dataset: &WorkloadDataset) {
     Python::with_gil(|py| {
         for code in &dataset.codes {
             let _ = engine
-                .resolve(py, code.as_ref())
+                .resolve(py, code.as_ref(), None)
                 .expect("prewarm resolution failed");
         }
     });
diff --git a/codetracer-python-recorder/src/runtime/tracer/events.rs b/codetracer-python-recorder/src/runtime/tracer/events.rs
@@ -176,28 +176,20 @@ impl Tracer for RuntimeTracer {
         code: &CodeObjectWrapper,
         _offset: i32,
     ) -> CallbackResult {
-        if self.module_name_from_globals {
-            if let Ok(qualname) = code.qualname(py) {
-                if qualname == "<module>" {
-                    let globals_name = match capture_frame(py, code) {
-                        Ok(snapshot) => {
-                            let mapping = snapshot.globals().unwrap_or_else(|| snapshot.locals());
-                            mapping
-                                .get_item("__name__")
-                                .ok()
-                                .flatten()
-                                .and_then(|value| value.extract::<String>().ok())
-                                .map(|name| name.trim().to_string())
-                                .filter(|name| !name.is_empty())
-                        }
-                        Err(_) => None,
-                    };
-
-                    self.filter
-                        .set_module_name_hint(code.id(), globals_name.clone());
-                }
+        let globals_name = match capture_frame(py, code) {
+            Ok(snapshot) => {
+                let mapping = snapshot.globals().unwrap_or_else(|| snapshot.locals());
+                mapping
+                    .get_item("__name__")
+                    .ok()
+                    .flatten()
+                    .and_then(|value| value.extract::<String>().ok())
+                    .map(|name| name.trim().to_string())
+                    .filter(|name| !name.is_empty())
             }
-        }
+            Err(_) => None,
+        };
+        self.filter.set_module_name_hint(code.id(), globals_name);
 
         if let Some(outcome) = self.evaluate_gate(py, code, true) {
             return Ok(outcome);
diff --git a/codetracer-python-recorder/src/runtime/tracer/filtering.rs b/codetracer-python-recorder/src/runtime/tracer/filtering.rs
@@ -128,19 +128,9 @@ impl FilterCoordinator {
             return Some(existing.clone());
         }
 
-        match engine.resolve(py, code) {
+        let hint = self.module_name_hints.get(&code_id).map(|s| s.as_str());
+        match engine.resolve(py, code, hint) {
             Ok(resolution) => {
-                let hint = self.module_name_hints.get(&code_id).cloned();
-                let resolution = if let Some(hint) = hint {
-                    if resolution.module_name() == Some(hint.as_str()) {
-                        resolution
-                    } else {
-                        Arc::new(resolution.clone_with_module_name(Some(hint)))
-                    }
-                } else {
-                    resolution
-                };
-
                 if resolution.exec() == ExecDecision::Trace {
                     self.scope_cache.insert(code_id, Arc::clone(&resolution));
                 } else {
diff --git a/codetracer-python-recorder/src/trace_filter/engine.rs b/codetracer-python-recorder/src/trace_filter/engine.rs
@@ -4,7 +4,7 @@
 //! and caches per-code-object resolutions so the hot tracing callbacks only pay a fast lookup.
 
 use crate::code_object::CodeObjectWrapper;
-use crate::module_identity::{is_valid_module_name, normalise_to_posix, ModuleIdentityResolver};
+use crate::module_identity::{is_valid_module_name, module_from_relative, normalise_to_posix};
 use crate::trace_filter::config::{
     ExecDirective, FilterSource, FilterSummary, ScopeRule, TraceFilterConfig, ValueAction,
     ValuePattern,
@@ -178,20 +178,6 @@ impl ScopeResolution {
     pub fn matched_rule_reason(&self) -> Option<&str> {
         self.matched_rule_reason.as_deref()
     }
-
-    pub(crate) fn clone_with_module_name(&self, module_name: Option<String>) -> ScopeResolution {
-        ScopeResolution {
-            exec: self.exec,
-            value_policy: Arc::clone(&self.value_policy),
-            module_name,
-            object_name: self.object_name.clone(),
-            relative_path: self.relative_path.clone(),
-            absolute_path: self.absolute_path.clone(),
-            matched_rule_index: self.matched_rule_index,
-            matched_rule_source: self.matched_rule_source,
-            matched_rule_reason: self.matched_rule_reason.clone(),
-        }
-    }
 }
 
 /// Runtime engine wrapping a compiled filter configuration.
@@ -202,7 +188,6 @@ pub struct TraceFilterEngine {
     default_value_source: usize,
     rules: Arc<[CompiledScopeRule]>,
     cache: DashMap<usize, Arc<ScopeResolution>>,
-    module_resolver: ModuleIdentityResolver,
 }
 
 impl TraceFilterEngine {
@@ -212,7 +197,6 @@ impl TraceFilterEngine {
         let default_value_action = config.default_value_action();
         let default_value_source = config.default_value_source();
         let rules = compile_rules(config.rules());
-        let module_resolver = ModuleIdentityResolver::new();
 
         TraceFilterEngine {
             config: Arc::new(config),
@@ -221,7 +205,6 @@ impl TraceFilterEngine {
             default_value_source,
             rules,
             cache: DashMap::new(),
-            module_resolver,
         }
     }
 
@@ -230,12 +213,13 @@ impl TraceFilterEngine {
         &self,
         py: Python<'py>,
         code: &CodeObjectWrapper,
+        module_hint: Option<&str>,
     ) -> RecorderResult<Arc<ScopeResolution>> {
         if let Some(entry) = self.cache.get(&code.id()) {
             return Ok(entry.clone());
         }
 
-        let resolution = Arc::new(self.resolve_uncached(py, code)?);
+        let resolution = Arc::new(self.resolve_uncached(py, code, module_hint)?);
         let entry = self
             .cache
             .entry(code.id())
@@ -247,6 +231,7 @@ impl TraceFilterEngine {
         &self,
         py: Python<'_>,
         code: &CodeObjectWrapper,
+        module_hint: Option<&str>,
     ) -> RecorderResult<ScopeResolution> {
         let filename = code
             .filename(py)
@@ -257,23 +242,31 @@ impl TraceFilterEngine {
 
         let mut context = ScopeContext::derive(filename, self.config.sources());
         context.refresh_object_name(qualname);
-        if let Some(absolute) = context.absolute_path.clone() {
-            if let Some(module) = self.resolve_module_name(py, &absolute) {
-                let replace = match context.module_name.as_deref() {
-                    None => true,
-                    Some(existing) => {
-                        !is_valid_module_name(existing)
-                            || (module == "__main__" && !existing.contains('.'))
-                    }
-                };
-                if replace {
-                    context.update_module_name(module, qualname);
+
+        if let Some(hint) = module_hint {
+            if is_valid_module_name(hint) {
+                context.module_name = Some(hint.to_string());
+                context.refresh_object_name(qualname);
+            }
+        }
+
+        if context
+            .module_name
+            .as_ref()
+            .map(|name| !is_valid_module_name(name))
+            .unwrap_or(true)
+            && context.module_name.is_none()
+        {
+            if let Some(absolute) = context.absolute_path.as_deref() {
+                if let Some(module) = module_name_from_packages(Path::new(absolute)) {
+                    context.module_name = Some(module);
+                    context.refresh_object_name(qualname);
+                } else {
+                    log::debug!(
+                        "[TraceFilter] unable to derive module name for '{}'; package selectors may not match",
+                        absolute
+                    );
                 }
-            } else if context.module_name.is_none() {
-                log::debug!(
-                    "[TraceFilter] unable to derive module name for '{}'; package selectors may not match",
-                    absolute
-                );
             }
         }
 
@@ -338,10 +331,6 @@ impl TraceFilterEngine {
     pub fn summary(&self) -> FilterSummary {
         self.config.summary()
     }
-
-    fn resolve_module_name(&self, py: Python<'_>, absolute: &str) -> Option<String> {
-        self.module_resolver.resolve_absolute(py, absolute)
-    }
 }
 
 #[derive(Debug, Clone)]
@@ -463,7 +452,8 @@ impl ScopeContext {
 
         let module_name = relative_path
             .as_deref()
-            .and_then(|rel| crate::module_identity::module_from_relative(rel));
+            .and_then(|rel| module_from_relative(rel))
+            .filter(|name| is_valid_module_name(name));
 
         ScopeContext {
             module_name,
@@ -482,11 +472,6 @@ impl ScopeContext {
             (None, true) => None,
         };
     }
-
-    fn update_module_name(&mut self, module: String, qualname: &str) {
-        self.module_name = Some(module);
-        self.refresh_object_name(qualname);
-    }
 }
 
 fn normalise_relative(relative: PathBuf) -> String {
@@ -506,6 +491,39 @@ fn normalise_relative(relative: PathBuf) -> String {
     components.join("/")
 }
 
+fn module_name_from_packages(path: &Path) -> Option<String> {
+    let mut segments: Vec<String> = Vec::new();
+    let mut current = path.parent();
+
+    while let Some(dir) = current {
+        let init = dir.join("__init__.py");
+        if init.exists() {
+            if let Some(name) = dir.file_name().and_then(|s| s.to_str()) {
+                if is_valid_module_name(name) {
+                    segments.push(name.to_string());
+                    current = dir.parent();
+                    continue;
+                }
+            }
+        }
+        break;
+    }
+
+    segments.reverse();
+
+    if let Some(stem) = path.file_stem().and_then(|s| s.to_str()) {
+        if stem != "__init__" && is_valid_module_name(stem) {
+            segments.push(stem.to_string());
+        }
+    }
+
+    if segments.is_empty() {
+        return None;
+    }
+
+    Some(segments.join("."))
+}
+
 fn py_attr_error(attr: &str, err: PyErr) -> recorder_errors::RecorderError {
     target!(
         ErrorCode::FrameIntrospectionFailed,
@@ -518,9 +536,8 @@ fn py_attr_error(attr: &str, err: PyErr) -> recorder_errors::RecorderError {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::module_identity::module_name_from_roots;
     use crate::trace_filter::config::TraceFilterConfig;
-    use pyo3::types::{PyAny, PyCode, PyDict, PyList, PyModule};
+    use pyo3::types::{PyAny, PyCode, PyList, PyModule};
     use std::ffi::CString;
     use std::fs;
     use std::io::Write;
@@ -564,7 +581,7 @@ mod tests {
             let wrapper = CodeObjectWrapper::new(py, &code_obj);
             let engine = TraceFilterEngine::new(config.clone());
 
-            let first = engine.resolve(py, &wrapper)?;
+            let first = engine.resolve(py, &wrapper, None)?;
             assert_eq!(first.exec(), ExecDecision::Trace);
             assert_eq!(first.matched_rule_index(), Some(0));
             assert_eq!(first.module_name(), Some("app.foo"));
@@ -583,7 +600,7 @@ mod tests {
                 ValueAction::Allow
             );
 
-            let second = engine.resolve(py, &wrapper)?;
+            let second = engine.resolve(py, &wrapper, None)?;
             assert!(Arc::ptr_eq(&first, &second));
             Ok(())
         })
@@ -619,7 +636,7 @@ mod tests {
             let wrapper = CodeObjectWrapper::new(py, &code_obj);
 
             let engine = TraceFilterEngine::new(config);
-            let resolution = engine.resolve(py, &wrapper)?;
+            let resolution = engine.resolve(py, &wrapper, None)?;
 
             assert_eq!(resolution.exec(), ExecDecision::Trace);
             assert_eq!(resolution.matched_rule_index(), Some(1));
@@ -672,13 +689,6 @@ mod tests {
                 normalise_to_posix(Path::new(file_path.to_string_lossy().as_ref())).unwrap();
 
             let module = py.import("app.foo").expect("import app.foo");
-            let modules_any = sys.getattr("modules").expect("sys.modules");
-            let modules: Bound<'_, PyDict> =
-                modules_any.downcast_into::<PyDict>().expect("modules dict");
-            modules
-                .set_item("app.foo", module.as_any())
-                .expect("register module");
-
             let func: Bound<'_, PyAny> = module.getattr("foo").expect("get foo");
             let code_obj = func
                 .getattr("__code__")
@@ -690,18 +700,11 @@ mod tests {
             assert_eq!(recorded_filename, file_path.to_string_lossy());
 
             let engine = TraceFilterEngine::new(config.clone());
-            let expected_root =
-                normalise_to_posix(Path::new(project_root.to_string_lossy().as_ref())).unwrap();
-            let resolver_roots = engine.module_resolver.module_roots();
-            assert!(resolver_roots.iter().any(|root| root == &expected_root));
-            let derived_from_roots = module_name_from_roots(resolver_roots, absolute_path.as_str());
-            assert_eq!(derived_from_roots, Some("app.foo".to_string()));
-            let resolution = engine.resolve(py, &wrapper)?;
+            let resolution = engine.resolve(py, &wrapper, None)?;
             assert_eq!(resolution.absolute_path(), Some(absolute_path.as_str()));
             assert_eq!(resolution.module_name(), Some("app.foo"));
             assert_eq!(resolution.exec(), ExecDecision::Skip);
 
-            modules.del_item("app.foo").expect("remove module");
             sys_path.del_item(0).expect("restore sys.path");
             Ok(())
         })
@@ -727,7 +730,7 @@ mod tests {
             let wrapper = CodeObjectWrapper::new(py, &code_obj);
 
             let engine = TraceFilterEngine::new(config);
-            let resolution = engine.resolve(py, &wrapper)?;
+            let resolution = engine.resolve(py, &wrapper, None)?;
 
             assert_eq!(resolution.exec(), ExecDecision::Skip);
             assert_eq!(resolution.relative_path(), Some("app/foo.py"));
diff --git a/design-docs/module-name-resolution-globals-implementation-plan.status.md b/design-docs/module-name-resolution-globals-implementation-plan.status.md
@@ -23,3 +23,7 @@
 ### Stage 1 – Capture `__name__` at `py_start`
 - **Status:** Completed  
   `RuntimeTracer` now captures `frame.f_globals['__name__']` for `<module>` code when the feature flag is on, threads the hint through `FilterCoordinator`, and prefers it during both filter decisions and runtime naming. Added integration coverage ensuring opt-in sessions record `<__main__>` for scripts, plus unit updates for the new plumbing.
+
+### Stage 2 – Simplify Filter Engine
+- **Status:** Completed  
+  Removed the resolver dependency from `TraceFilterEngine`, teach it to accept globals-based hints, and added a package-structure fallback when relative paths are unavailable. Filtering decisions now rely on the new hint flow while keeping legacy behaviour intact for `module_name_from_globals` opt-out scenarios.
