Skip to content

Conversation

@dividedmind
Copy link
Contributor

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

  • 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
  • Add an option to exclude specific hooks
  • Don't throw when loading logging config fails (harmless but message is scary)

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.
Copy link

Copilot AI left a 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.exclude property
  • Fixed NullPointerException when exclude field is null/empty in appmap.yml
  • Removed unused allMethods field and obsolete PatternThreshold property

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.

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
@dividedmind dividedmind force-pushed the feat/exclusion-improvements branch from a6f3893 to dec4b99 Compare January 17, 2026 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants