Open
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@deeployed-peer-dev review |
WalkthroughThe PR migrates the customize‑enhance plugin to the new V2 instrumentation API, adds support for Changes
Sequence DiagramsequenceDiagram
participant C as CustomizeController
participant S as TestService1
participant I as CustomizeInstanceInterceptor
participant T as SpanDataHolder
participant L as Logger
C->>S: retString("str0")
S->>I: beforeMethod()
I->>T: createSpan()
S-->>I: return "str0"
I->>T: evaluateReturnExpression("str0")
I->>T: addTag("returnValue", "str0")
I->>T: addLog("returnValue", "str0")
I->>T: finishSpan()
I->>L: logSpan()
The diagram illustrates the flow from the controller invoking a service method, the interceptor capturing the call, evaluating the returned object to enrich the span with tags and logs, and finally completing the span. Uplevel your code reviews with Deeployed Peer! |
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: (20)
- Review: 4
- LGTM: 16
- 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
+66
to
68
| Map<String, String> spanTags = tags == null ? Collections.EMPTY_MAP : new HashMap<String, String>(tags.size()); | ||
| Map<String, String> spanLogs = logs == null ? Collections.EMPTY_MAP : new HashMap<String, String>(logs.size()); | ||
|
|
There was a problem hiding this comment.
The code uses Collections.EMPTY_MAP for spanTags and spanLogs when tags or logs are null.
Collections.EMPTY_MAP is a raw, immutable map. Later, spanTags.clear() and spanLogs.clear() are called (lines 93‑94), which will throw an UnsupportedOperationException when the maps are immutable.
Replace the immutable maps with mutable ones.
Suggested change
| Map<String, String> spanTags = tags == null ? Collections.EMPTY_MAP : new HashMap<String, String>(tags.size()); | |
| Map<String, String> spanLogs = logs == null ? Collections.EMPTY_MAP : new HashMap<String, String>(logs.size()); | |
| Map<String, String> spanTags = tags == null ? new HashMap<>() : new HashMap<>(tags.size()); | |
| Map<String, String> spanLogs = logs == null ? new HashMap<>() : new HashMap<>(logs.size()); |
Comment on lines
+93
to
+94
| spanTags.clear(); | ||
| spanLogs.clear(); |
There was a problem hiding this comment.
spanTags.clear(); and spanLogs.clear(); assume the maps are mutable.
After the change above, these calls are safe; otherwise, they would throw an exception.
Comment on lines
+137
to
+139
| void handleMethodException(Throwable t) { | ||
| ContextManager.activeSpan().log(t); | ||
| } |
There was a problem hiding this comment.
handleMethodException logs the exception without checking if a span is active.
If no span is active, ContextManager.activeSpan() will return null, causing a NullPointerException.
Add a guard to ensure a span is active before logging.
Suggested change
| void handleMethodException(Throwable t) { | |
| ContextManager.activeSpan().log(t); | |
| } | |
| if (ContextManager.isActive()) { | |
| ContextManager.activeSpan().log(t); | |
| } |
Comment on lines
41
to
45
| @Override | ||
| 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.
There's a missing MethodInvocationContext argument in the call to super.handleMethodException(t). It should forward the context as well.
Suggested change
| @Override | |
| public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, | |
| Class<?>[] argumentsTypes, Throwable t) { | |
| Class<?>[] argumentsTypes, Throwable t, MethodInvocationContext context) { | |
| super.handleMethodException(t); | |
| } | |
| super.handleMethodException(t, context); |
|
3 FN |
|
gpt-oss-20b |
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
3 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.
Summary
Implemented support for
returnedObjexpressions in the customize‑enhance plugin and migrated the plugin to use the new V2 instrumentation API. Added new test cases, documentation updates, and configuration examples for return‑value based tags and logs.File‑by‑file changes
CHANGES.md
returnedObjexpression for apm-customize-enhance-plugin”.apm-sniffer/apm-agent-core/src/test/java/org/apache/skywalking/apm/agent/core/util/CustomizeExpressionTest.java
org.apache.skywalking.apm.agent.core.util.apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/define/CustomizeInstanceInstrumentation.java
ClassInstanceMethodsEnhancePluginDefineV2.InstanceMethodsInterceptV2Point,getInstanceMethodsInterceptV2Points,getMethodsInterceptorV2).apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/define/CustomizeStaticInstrumentation.java
ClassStaticMethodsEnhancePluginDefineV2.StaticMethodsInterceptV2Point,getStaticMethodsInterceptV2Points,getMethodsInterceptorV2).apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/interceptor/BaseInterceptorMethods.java
returnedObjexpressions.SpanDataHolderto store span and tag/log data across method boundaries.beforeMethod,afterMethod, andhandleMethodExceptionusing V2MethodInvocationContext.apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/interceptor/CustomizeInstanceInterceptor.java
InstanceMethodsAroundInterceptorV2.MethodInvocationContext.apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/interceptor/CustomizeStaticInterceptor.java
StaticMethodsAroundInterceptorV2.MethodInvocationContext.docs/en/setup/service-agent/java-agent/Customize-enhance-trace.md
returnedObjusage for tags and logs.returnedObjdescription.test/plugin/scenarios/customize-scenario/bin/startup.sh
test/plugin/scenarios/customize-scenario/config/customize_enhance.xml
returnedObjtags and logs.test/plugin/scenarios/customize-scenario/config/expectedData.yaml
test/plugin/scenarios/customize-scenario/src/main/java/test/apache/skywalking/apm/testcase/customize/controller/CustomizeController.java
retStringandretModel0to generate traces.test/plugin/scenarios/customize-scenario/src/main/java/test/apache/skywalking/apm/testcase/customize/service/TestService1.java
retStringandretModel0methods returning the input.Result
The plugin now correctly parses
returnedObjexpressions, evaluates them against the method return value, and attaches the resulting values as tags or logs on the generated span. All tests pass with the new functionality.