Open
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Author
|
@deeployed-peer-dev review |
WalkthroughThis PR updates the customize-enhance-plugin to support `returnedObj` expressions for accessing method return values in tags/logs and migrates to the V2 API. It also adds comprehensive test coverage for the new functionality.Changes
Sequence Diagram```mermaid sequenceDiagram participant Client participant CustomizeController participant TestService1 participant CustomizeInstanceInterceptor participant BaseInterceptorMethods participant SpanDataHolder participant SkyWalkingCore |
There was a problem hiding this comment.
Commits
Files that changed from the base of the PR and between Base and 86ff5ed commits: 13Files Selected For Review: 10
- apm-sniffer/apm-agent-core/src/test/java/org/apache/skywalking/apm/agent/core/util/CustomizeExpressionTest.java with diff length 263.
- apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/define/CustomizeInstanceInstrumentation.java with diff length 2438.
- apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/define/CustomizeStaticInstrumentation.java with diff length 2327.
- apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/interceptor/BaseInterceptorMethods.java with diff length 9156.
- apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/interceptor/CustomizeInstanceInterceptor.java with diff length 2007.
- apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/interceptor/CustomizeStaticInterceptor.java with diff length 1683.
- test/plugin/scenarios/customize-scenario/bin/startup.sh with diff length 256.
- test/plugin/scenarios/customize-scenario/config/expectedData.yaml with diff length 852.
- test/plugin/scenarios/customize-scenario/src/main/java/test/apache/skywalking/apm/testcase/customize/controller/CustomizeController.java with diff length 237.
- test/plugin/scenarios/customize-scenario/src/main/java/test/apache/skywalking/apm/testcase/customize/service/TestService1.java with diff length 282.
Files excluded from review due to unsupported file extensions: 3
- CHANGES.md
- docs/en/setup/service-agent/java-agent/Customize-enhance-trace.md
- test/plugin/scenarios/customize-scenario/config/customize_enhance.xml
Review comments generated: (28)
- Review: 5
- LGTM: 23
- Identified issue already discussed: 0
Tips
Chat with Deeployed Bot (@@deeployed-peer-dev)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@@deeployed-peer-devin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@deeployed-peer-dev: ignoreanywhere in the PR description to pause further reviews from the bot.
Comment on lines
+137
to
+139
| void handleMethodException(Throwable t) { | ||
| ContextManager.activeSpan().log(t); | ||
| } |
There was a problem hiding this comment.
In the handleMethodException method, there's a potential issue where ContextManager.activeSpan() is called without checking if the context is active first, which could lead to a NullPointerException if no span is active.
Suggested change
| void handleMethodException(Throwable t) { | |
| ContextManager.activeSpan().log(t); | |
| } | |
| void handleMethodException(Throwable t) { | |
| if (ContextManager.isActive()) { | |
| ContextManager.activeSpan().log(t); | |
| } | |
| } |
| import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.v2.InstanceMethodsAroundInterceptorV2; | ||
| import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.v2.MethodInvocationContext; | ||
|
|
||
| public class CustomizeInstanceInterceptor extends BaseInterceptorMethods implements InstanceMethodsAroundInterceptorV2 { |
There was a problem hiding this comment.
The class now implements InstanceMethodsAroundInterceptorV2 instead of InstanceMethodsAroundInterceptor, which is consistent with the migration to the V2 API mentioned in the PR description.
Comment on lines
29
to
32
| public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, | ||
| MethodInterceptResult result) throws Throwable { | ||
| super.beforeMethod(method, allArguments); | ||
| MethodInvocationContext context) throws Throwable { | ||
| super.beforeMethod(method, allArguments, context); | ||
| } |
There was a problem hiding this comment.
The beforeMethod signature has been updated to include MethodInvocationContext parameter and the call to super.beforeMethod now passes this context instead of the previous parameters.
Comment on lines
35
to
39
| public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, | ||
| Object ret) throws Throwable { | ||
| super.afterMethod(method); | ||
| Object ret, MethodInvocationContext context) throws Throwable { | ||
| super.afterMethod(method, ret, context); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
The afterMethod signature has been updated to include MethodInvocationContext parameter and the call to super.afterMethod now passes the return value and context, which is necessary for the new returnedObj expression support.
Comment on lines
42
to
45
| public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, | ||
| Class<?>[] argumentsTypes, Throwable t) { | ||
| Class<?>[] argumentsTypes, Throwable t, MethodInvocationContext context) { | ||
| super.handleMethodException(t); | ||
| } |
There was a problem hiding this comment.
The handleMethodException signature has been updated to include MethodInvocationContext parameter, consistent with the V2 API migration.
Author
|
GLM-4.6 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Add support for
returnedObjexpression in apm-customize-enhance-pluginThis PR adds support for using
returnedObjexpression in the customize enhance plugin to access method return values in tags and logs. The plugin has also been updated to use the V2 API.Files Changed:
CHANGES.md
returnedObjexpression supportapm-sniffer/apm-agent-core/src/test/java/org/apache/skywalking/apm/agent/core/util/CustomizeExpressionTest.java
apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/define/CustomizeInstanceInstrumentation.java
apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/define/CustomizeStaticInstrumentation.java
apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/interceptor/BaseInterceptorMethods.java
returnedObjexpression evaluationapm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/interceptor/CustomizeInstanceInterceptor.java
apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/interceptor/CustomizeStaticInterceptor.java
docs/en/setup/service-agent/java-agent/Customize-enhance-trace.md
returnedObjexpression usagetest/plugin/scenarios/customize-scenario/config/customize_enhance.xml
returnedObjexpression scenariostest/plugin/scenarios/customize-scenario/config/expectedData.yaml
returnedObjexpression teststest/plugin/scenarios/customize-scenario/src/main/java/test/apache/skywalking/apm/testcase/customize/controller/CustomizeController.java
returnedObjexpression functionalitytest/plugin/scenarios/customize-scenario/src/main/java/test/apache/skywalking/apm/testcase/customize/service/TestService1.java
returnedObjexpression