Pr#83 branch5-oss-20B by ahmetkaansever · Pull Request #5 · Deeployed-Peer/skywalking-java · GitHub

Summary

Implemented support for returnedObj expressions 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
- Added new entry: “Add support returnedObj expression for apm-customize-enhance-plugin”.
apm-sniffer/apm-agent-core/src/test/java/org/apache/skywalking/apm/agent/core/util/CustomizeExpressionTest.java
- Updated package declaration to 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
- Switched to V2 plugin base class ClassInstanceMethodsEnhancePluginDefineV2.
- Updated imports and method signatures to V2 (InstanceMethodsInterceptV2Point, getInstanceMethodsInterceptV2Points, getMethodsInterceptorV2).
apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/define/CustomizeStaticInstrumentation.java
- Switched to V2 plugin base class ClassStaticMethodsEnhancePluginDefineV2.
- Updated imports and method signatures to V2 (StaticMethodsInterceptV2Point, getStaticMethodsInterceptV2Points, getMethodsInterceptorV2).
apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/interceptor/BaseInterceptorMethods.java
- Added support for evaluating and handling returnedObj expressions.
- Introduced SpanDataHolder to store span and tag/log data across method boundaries.
- Implemented beforeMethod, afterMethod, and handleMethodException using V2 MethodInvocationContext.
- Added helper methods for expression evaluation, tag/log population, and span tagging.
apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/interceptor/CustomizeInstanceInterceptor.java
- Updated to V2 interceptor interface InstanceMethodsAroundInterceptorV2.
- Adjusted method signatures to use MethodInvocationContext.
apm-sniffer/optional-plugins/customize-enhance-plugin/src/main/java/org/apache/skywalking/apm/plugin/customize/interceptor/CustomizeStaticInterceptor.java
- Updated to V2 interceptor interface StaticMethodsAroundInterceptorV2.
- Adjusted method signatures to use MethodInvocationContext.
docs/en/setup/service-agent/java-agent/Customize-enhance-trace.md
- Added examples showing returnedObj usage for tags and logs.
- Updated table to include returnedObj description.
test/plugin/scenarios/customize-scenario/bin/startup.sh
- Minor formatting fix.
test/plugin/scenarios/customize-scenario/config/customize_enhance.xml
- Added methods demonstrating returnedObj tags and logs.
test/plugin/scenarios/customize-scenario/config/expectedData.yaml
- Added expected trace data for the new return‑value based methods.
test/plugin/scenarios/customize-scenario/src/main/java/test/apache/skywalking/apm/testcase/customize/controller/CustomizeController.java
- Invoked new methods retString and retModel0 to generate traces.
test/plugin/scenarios/customize-scenario/src/main/java/test/apache/skywalking/apm/testcase/customize/service/TestService1.java
- Implemented retString and retModel0 methods returning the input.

Result

The plugin now correctly parses returnedObj expressions, 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.

…nce-plugin

@deeployed-peer-dev review

Walkthrough

The PR migrates the customize‑enhance plugin to the new V2 instrumentation API, adds support for returnedObj expressions, and updates the test suite to validate the new behavior. A test class is moved into the core util package to match the relocated implementation. The plugin definitions and interceptors now use V2 interfaces, evaluate return values, and tag/log spans accordingly. The customize‑scenario tests are updated to exercise these changes, including new spans derived from returned values.

Changes

File(s)	Summary
apm-sniffer/apm-agent-core/src/test/java/org/apache/skywalking/apm/agent/core/util/CustomizeExpressionTest.java	Moved test to core util package, updated package declaration and imports to match relocated CustomizeExpression.
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, apm-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	Migrated plugin to V2 API: definitions now extend ClassInstanceMethodsEnhancePluginDefineV2 / ClassStaticMethodsEnhancePluginDefineV2; interceptors implement InstanceMethodsAroundInterceptorV2 / StaticMethodsAroundInterceptorV2. Added returnedObj expression handling via CustomizeExpression.parseReturnExpression, storing a SpanDataHolder, and tagging/logging spans based on method return values.
test/plugin/scenarios/customize-scenario/bin/startup.sh, 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, test/plugin/scenarios/customize-scenario/src/main/java/test/apache/skywalking/apm/testcase/customize/service/TestService1.java	Updated test scenario to exercise returnedObj support: added new controller calls and service methods that return values, updated expected data to include new spans with tags/logs derived from return values, and fixed minor script formatting.

Sequence Diagram

sequenceDiagram
    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!

Commits Files that changed from the base of the PR and between Base and 86ff5ed commits: 13 Files 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-dev in 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: ignore anywhere in the PR description to pause further reviews from the bot.

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());

TP

spanTags.clear(); and spanLogs.clear(); assume the maps are mutable.
After the change above, these calls are safe; otherwise, they would throw an exception.

TP

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);
          
            }

BP

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);

FP_Hallucination

3 FN

gpt-oss-20b
3FN
2TP
1BP
1FP_Hallucination

chanjarster added 2 commits December 14, 2021 16:08

issue #8292 add returnedObj expression support for apm-customize-enha…

a65f6bd

…nce-plugin

update apm-customize-enhance-plugin use plugin V2 API

86ff5ed

This comment was marked as outdated.

Sign in to view

deeployed-peer-dev Bot reviewed Oct 29, 2025

View reviewed changes

		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());

Pr#83 branch5-oss-20B by ahmetkaansever · Pull Request #5 · Deeployed-Peer/skywalking-java · GitHub

Uh oh!

Pr#83 branch5-oss-20B - #5

Pr#83 branch5-oss-20B#5ahmetkaansever wants to merge 2 commits intotargetDeeployed-Peer/skywalking-java:targetfrom PR#83-branch5Deeployed-Peer/skywalking-java:PR#83-branch5Copy head branch name to clipboard

Conversation

ahmetkaansever commented Oct 29, 2025

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

Konuralpkilinc commented Oct 29, 2025

Uh oh!

deeployed-peer-dev Bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deeployed-peer-dev Bot left a comment

Choose a reason for hiding this comment

Chat with Deeployed Bot (@@deeployed-peer-dev)

Code suggestions

Pausing incremental reviews

Uh oh!

deeployed-peer-dev Bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

Konuralpkilinc Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

deeployed-peer-dev Bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

Konuralpkilinc Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

deeployed-peer-dev Bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

Konuralpkilinc Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

deeployed-peer-dev Bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

Konuralpkilinc Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Konuralpkilinc commented Oct 30, 2025

Uh oh!

Konuralpkilinc commented Oct 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Pr#83 branch5-oss-20B#5
ahmetkaansever wants to merge 2 commits into
targetfrom
PR#83-branch5

deeployed-peer-dev Bot commented Oct 29, 2025 •
edited
Loading