Fix symbol extraction for instrumented methods - #6339
Conversation
Resolve original method declarations behind prepended wrappers so existing method probes do not hide application symbols or source metadata. Preserve inherited visibility overrides and gem filtering, and cover full and incremental extraction with active probes.
tylfin
added
the
AI Generated
dd-octo-sts
Bot
added
the
debugger
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 13bdc1c | Docs | View more details | Give us feedback! |
Add a YARD docstring to declared_instance_method matching the surrounding private methods, and rename the parameter from name to method_name for consistency with the rest of the extractor. Drop the inline comment that described prepend semantics by contrast.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
All reviewed changes are covered by regression tests with no unresolved blocking issues.
Review effort: Lite
Findings: None
What changed in this PR
Fixes symbol extraction for methods wrapped by Dynamic Instrumentation probes, preserving original method metadata and source information.
Changes:
- Resolves original methods through prepend chains.
- Applies the fix to full and incremental extraction.
- Adds regression tests, RBS typing, and a changelog entry.
| File | Description |
|---|---|
unreleased/20260918164420.json |
Documents the fix. |
spec/datadog/symbol_database/extractor_spec.rb |
Adds probe and visibility regression coverage. |
sig/datadog/symbol_database/extractor.rbs |
Declares the new helper signature. |
lib/datadog/symbol_database/extractor.rb |
Resolves original method metadata. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98ca024434
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Replace the tautological visibility-override test, which passed against a no-op declared_instance_method, with a prepend-skip assertion on the resolved owner. Add a fallback-branch test for the super_method-nil path. Assert the DI wrapper is actually installed in the active-probes before block so a silent hook no-op cannot leave the tests green. Use let(:filename) instead of the @filename instance variable in the active-probes context.
extract resolves every method through declared_instance_method three times (find_source_file, calculate_class_line_range, extract_method_scopes), each walk re-walking the super_method chain. Memoize per (mod, method_name) for the duration of a single extract call so each method is resolved once. extract_all stays uncached: its two passes must re-resolve to detect methods that moved files between passes. Document that the singleton-method path in find_source_file is not memoized because method-logpoint probes prepend instance methods only.
BenchmarksBenchmark execution time: 2026-09-28 21:16:47 Comparing candidate commit a3d8a2a in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 52 metrics, 0 unstable metrics.
|
extract computed find_source_file up to three times per call (once in user_code_module?, once in extract, once in the scope builder) and extract_targetable_lines twice per public/protected method (once in calculate_class_line_range, once in extract_method_scope). The @method_resolution_cache memoized only the cheap declared_instance_method reflection, leaving the expensive iseq compilation duplicated. extract now resolves source_file once and threads it into the scope builders. extract_class_scope builds one record per method holding the resolved location, targetable lines, visibility, arity, and parameters; calculate_class_line_range reduces over the public/protected records and build_method_scopes maps over the user-code records, so each method is resolved and iseq-compiled once. declared_instance_method is now a pure helper with no instance state.
Typing analysisNote: Ignored files are excluded from the next sections.
|
build_method_records returned an Array<Hash> with nine fixed keys consumed via record[:key] lookups, leaving the contract untyped in RBS and a typo'd key silently yielding nil. Introduce Datadog::SymbolDatabase::MethodRecord, a keyword_init Struct, so the record shape is a named type with typed accessors and a self-documenting constructor.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6770656749
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
calculate_class_line_range skipped private methods, so a class whose earliest or latest method was private got an underreported start_line or end_line. The extract_all path (convert_node_to_scope) already spanned all visibilities; this makes the extract path match. Adds a test with private methods bounding the class to pin the span.
build_method_records produced an Array<MethodRecord> so calculate_class_line_range and build_method_scopes could reuse one resolution per method. The record only existed to feed a class line range that spanned gem-defined methods too, which contradicted the extract_all path (convert_node_to_scope derives the range from user-code method scopes) and could put a gem file's line numbers on a user-file CLASS scope. Drop MethodRecord and build the METHOD scopes once in extract_class_scope via build_class_method_scopes (reusing build_instance_method_scope, one iseq compile per method). Derive the class line range from those scopes with line_range_from_scopes, shared with convert_node_to_scope so the two paths cannot diverge again.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ecb24beff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Address review comment: a method removed or redefined between collecting method_names and resolving them makes declared_instance_method raise NameError. With only the method-level rescue, that discarded every scope for the class instead of just the affected method — regressing the per-method isolation the earlier extract_method_scope rescue provided. Add a per-method rescue inside the filter_map block, matching the shape used by collect_method_names_by_file and build_file_scope, so a single method's failure drops only that method. The outer method-level rescue is retained as the class-level backstop. - Fixed in lib/datadog/symbol_database/extractor.rb build_class_method_scopes - Added regression coverage in spec/datadog/symbol_database/extractor_spec.rb asserting surviving methods still produce scopes when one raises NameError Verified: rspec (targeted + full extractor_spec, 156 examples 0 failures), steep check (no type errors), standardrb and rubocop clean. Co-Authored-By: Claude <noreply@anthropic.com>
find_source_file resolves the source location of each instance and singleton method inside its two loops. This PR routed those loops through declared_instance_method, which raises NameError when a method is removed or redefined between name collection and lookup. Without a per-method rescue, one such failure aborted the whole lookup — including the const_source_location fallbacks — and returned nil, the same regression fixed in build_class_method_scopes. Add a per-method rescue to both loops so a single method's failure is skipped and later user-code paths are still found. The method-level rescue is retained as the backstop. - Fixed in lib/datadog/symbol_database/extractor.rb find_source_file (instance-method loop and singleton-method loop) - Added regression coverage in spec/datadog/symbol_database/extractor_spec.rb for both loops: a later user-code path is still found when an earlier method raises NameError Verified: rspec (targeted + full extractor_spec, 156 examples 0 failures), steep check (no type errors), standardrb and rubocop clean. Co-Authored-By: Claude <noreply@anthropic.com>
What does this PR do?
Preserve original method symbols and source metadata when DI probes prepend wrappers, in both full and incremental symbol extraction.
Motivation:
An existing method logpoint can make its target disappear from code search and source lookup: reflection resolves the prepended DI wrapper, which is then excluded as gem code.
Resolve the method declared by the inspected class or module instead of extracting the wrapper.
Change log entry
Yes. Fix methods disappearing from code search and source lookup when method logpoints are already installed.
Additional Notes:
How to test the change?
Unit tests added