-
Notifications
You must be signed in to change notification settings - Fork 18
refactor(agent): Refactor and optimize AppMapPackage #317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Introduces a new configuration property `appmap.hooks.exclude` to allow disabling specific AppMap hook classes by their fully qualified name. This addresses issues where certain hooks, such as `SqlQuery`, might cause `NoClassDefFoundError` due to classloading conflicts or unexpected interactions with the target application's environment. The new property can be set via a system property `-Dappmap.hooks.exclude=<CLASS_NAME>` or an environment variable `APPMAP_HOOKS_EXCLUDE=<CLASS_NAME>`. The agent's `ClassFileTransformer` now checks this exclusion list during hook processing, preventing the instrumentation of specified hook classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR comprehensively refactors the AppMapPackage class to improve performance, readability, and maintainability. The primary optimization replaces linear exclusion pattern matching with a PrefixTrie data structure, dramatically improving lookup performance from O(N*M) to O(M) complexity.
Changes:
- Introduced PrefixTrie data structure for efficient exclusion pattern matching
- Added comprehensive documentation clarifying exclude mode vs. methods mode
- Added 42 comprehensive tests covering both configuration modes and edge cases
- Added option to exclude specific hooks via
appmap.hooks.excludeproperty - Fixed NullPointerException when exclude field is null/empty in appmap.yml
- Removed unused
allMethodsfield and obsoletePatternThresholdproperty
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| AppMapPackage.java | Core refactoring with PrefixTrie integration, improved documentation, and clearer mode detection logic |
| PrefixTrie.java | New efficient prefix-matching data structure for exclusion patterns |
| AppMapPackageTest.java | Comprehensive test suite with 42 new tests covering both modes and edge cases |
| AppMapConfigTest.java | Added test for empty exclude field handling |
| AppMapConfig.java | Removed obsolete pattern threshold warning |
| Properties.java | Removed unused PatternThreshold, added ExcludedHooks configuration |
| ClassFileTransformer.java | Added hook exclusion capability using new ExcludedHooks property |
| AppMapConfigurationLoader.java | Changed to not throw IOException when logging config load fails |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
agent/src/main/java/com/appland/appmap/config/AppMapPackage.java
Outdated
Show resolved
Hide resolved
This commit comprehensively refactors the AppMapPackage class to improve readability, performance, and maintainability. Replace linear exclusion matching with a PrefixTrie data structure, reducing lookup complexity from O(N*M) to O(M), where N is the number of exclusion patterns and M is the length of the class name. This provides dramatic performance improvements for configurations with many exclusion patterns. Exclusion patterns can now be specified relative to the package path (e.g., "internal" instead of "com.example.internal"), improving configuration clarity while maintaining backward compatibility. Add comprehensive documentation explaining the two mutually exclusive configuration modes (exclude mode vs. methods mode). Refactor the complex find() method into three clear helpers with explicit mode detection. Add a warning when both 'exclude' and 'methods' are specified, making the precedence rules explicit to users. Enhance LabelConfig to support matching against both simple and fully qualified class names for better user experience. Remove unused class resolution logic. Add 42 comprehensive tests covering both configuration modes, edge cases, regex patterns, backward compatibility, and complex scenarios. - Fix NullPointerException when 'exclude' field is empty in appmap.yml - Fix package boundary matching (prevent "com.examples" matching "com.example") - Remove unused 'allMethods' field (added in 2022, never implemented) - Remove obsolete pattern threshold warning (no longer needed with PrefixTrie) - Clean up unused imports
a6f3893 to
dec4b99
Compare
This change comprehensively refactors the AppMapPackage class to improve
readability, performance, and maintainability.
Performance Improvements
Replace linear exclusion matching with a PrefixTrie data structure,
reducing lookup complexity from O(N*M) to O(M), where N is the number of
exclusion patterns and M is the length of the class name. This provides
dramatic performance improvements for configurations with many exclusion
patterns.
Exclusion patterns can now be specified relative to the package path
(e.g., "internal" instead of "com.example.internal"), improving
configuration clarity while maintaining backward compatibility.
Clarity and Documentation
Add comprehensive documentation explaining the two mutually exclusive
configuration modes (exclude mode vs. methods mode). Refactor the complex
find() method into three clear helpers with explicit mode detection.
Add a warning when both 'exclude' and 'methods' are specified, making the
precedence rules explicit to users.
Enhance LabelConfig to support matching against both simple and fully
qualified class names for better user experience. Remove unused class
resolution logic.
Test Coverage
Add 42 comprehensive tests covering both configuration modes, edge cases,
regex patterns, backward compatibility, and complex scenarios.
Bug Fixes and Cleanup