feat: Improve agent logging and refactor internals #311
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The AppMap agent's logging has been enhanced for better readability and detail. This includes: - Setting a standardized log format for all messages to yyyy-MM-dd HH:mm:ss [thread] AppMap level: message. - Refining the AppMapConfig toString() method to provide a more structured and comprehensive output of the configuration details, including name, config file path, and package information. - Adjusting log levels for system properties output in Agent.java from info to debug, and removing a redundant stack trace in debug mode for cleaner logs.
There was a problem hiding this comment.
Pull request overview
This PR refactors the AppMap agent's internal instrumentation logic and improves logging and configuration output. The main goal is to enhance maintainability, debuggability, and code clarity while optimizing the instrumentation process.
Key changes:
- Refactored
Hook.apply()to return aSet<ApplicationAction>that indicates which instrumentation types were applied (ByteBuddy marking vs Javassist instrumentation), allowing conditional annotation of classes - Extracted parameter name resolution logic into a dedicated
getParameterNames()method in theParametersclass with improved handling of LocalVariableTable attributes - Standardized logging format and adjusted log levels for cleaner output, plus improved
AppMapConfig.toString()for better configuration visibility
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Hook.java | Refactored apply() method to return Set<ApplicationAction>, added exception handling with suppression after first occurrence, and improved logging for instrumentation actions |
| ClassFileTransformer.java | Updated to use new Hook.apply() return type for conditional ByteBuddy annotation application, optimizing the instrumentation process |
| Parameters.java | Extracted parameter name resolution into getParameterNames() method, renamed clone() to freshCopy(), removed unused imports, and improved JavaDoc formatting |
| AppMapConfig.java | Added standardized logging format configuration and replaced JSON-based toString() with a custom YAML-like formatted output |
| Agent.java | Changed system properties logging from info to debug level for cleaner default output |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
agent/src/main/java/com/appland/appmap/output/v1/Parameters.java
Outdated
Show resolved
Hide resolved
agent/src/main/java/com/appland/appmap/output/v1/Parameters.java
Outdated
Show resolved
Hide resolved
agent/src/main/java/com/appland/appmap/transform/ClassFileTransformer.java
Outdated
Show resolved
Hide resolved
The logic for extracting parameter names from `LocalVariableAttribute` was quite complex and interleaved with the parameter value construction. This commit extracts that logic into a new private static method `getParameterNames` to improve readability and maintainability of the `Parameters` constructor. As opposed to the previous implementation, the new method traverses all local variable tables (as the spec suggests there can be several) and doesn't spam the logs if debug info is missing. Additionally: - Updated `Parameters` constructor to use the new `getParameterNames` method. - Replaced `this.staticParameters.clone()` with `this.staticParameters.freshCopy()` for clarity, as it's not a deep clone but a copy of value types, kinds and names. - Cleaned up unused imports and removed `clear()` method as it's not used and modifies the object unexpectedly. - Made minor improvements to null checks and error handling within `Parameters` methods for robustness.
Refactor `Hook.apply` to return a `Set<ApplicationAction>` indicating whether the method was marked for ByteBuddy instrumentation or instrumented by Javassist. This allows `ClassFileTransformer` to conditionally apply the `AppMapInstrumented` annotation only when ByteBuddy instrumentation is actually needed. Additionally, add improved logging for Javassist instrumentation failures and guard against excessive logging of these exceptions.
dividedmind
force-pushed
the
improve-debugging
branch
from
b7276e9 to
0cb2362
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes several improvements to the AppMap agent, focusing on logging, configuration output, and internal refactoring for better maintainability and robustness.
Key Changes:
Enhanced Logging and Configuration Output:
yyyy-MM-dd HH:mm:ss [thread] AppMap level: messagefor consistency and readability.AppMapConfig.toString()method has been updated to provide a more detailed and structured output of the configuration.Refactored Parameter Name Resolution:
LocalVariableAttributehas been extracted into a newgetParameterNamesmethod within theParametersclass. This improves code clarity and maintainability.Improved Instrumentation Hooks:
Hook.applyhas been refactored to return aSet<ApplicationAction>, indicating the type of instrumentation applied (ByteBuddy or Javassist).ClassFileTransformerto conditionally apply theAppMapInstrumentedannotation only when ByteBuddy instrumentation is used, optimizing the process.These changes contribute to a more robust, debuggable, and maintainable AppMap agent.