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 migrates the Customize Enhance Plugin to the V2 API and adds support for the Changes
Sequence DiagramsequenceDiagram
participant A as APM Agent
participant CE as Customize Enhance Plugin
participant T as Target Method
participant C as Context
A->>CE: Instrumentation (V2 API)
CE->>T: Method Call
T-->>CE: Return Value
CE->>C: Evaluate returnedObj Expression
C->>CE: Populate Tags/Logs
CE-->>A: Enhanced Span Data
|
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: (29)
- Review: 15
- LGTM: 14
- 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.
| */ | ||
|
|
||
| package org.apache.skywalking.apm.plugin.customize.util; | ||
| package org.apache.skywalking.apm.agent.core.util; |
There was a problem hiding this comment.
The package name has been updated to reflect the new location. This change aligns with the migration to the V2 API structure.
Suggested change
| package org.apache.skywalking.apm.agent.core.util; | |
| package org.apache.skywalking.apm.agent.core.util; |
Comment on lines
21
to
22
| import org.junit.Assert; | ||
| import org.junit.Test; |
There was a problem hiding this comment.
The import for CustomizeExpression has been removed since it's now in the same package, making the import unnecessary.
Suggested change
| import org.junit.Assert; | |
| import org.junit.Test; | |
| import org.junit.Assert; | |
| import org.junit.Test; |
Comment on lines
+59
to
+101
| void beforeMethod(Method method, Object[] allArguments, MethodInvocationContext miContext) { | ||
| Map<String, Object> configuration = CustomizeConfiguration.INSTANCE.getConfiguration(method); | ||
| String operationName = MethodConfiguration.getOperationName(configuration); | ||
| Map<String, Object> context = CustomizeExpression.evaluationContext(allArguments); | ||
| if (context == null || context.isEmpty()) { | ||
| ContextManager.createLocalSpan(operationName); | ||
| } else { | ||
| Map<String, Object> evalContext = CustomizeExpression.evaluationContext(allArguments); | ||
|
|
||
| Map<String, String> tags = MethodConfiguration.getTags(configuration); | ||
| Map<String, String> spanTags = new HashMap<String, String>(); | ||
| Map<String, String> logs = MethodConfiguration.getLogs(configuration); | ||
| Map<String, String> spanLogs = new HashMap<String, String>(); | ||
| Map<String, String> tags = MethodConfiguration.getTags(configuration); | ||
| Map<String, String> logs = MethodConfiguration.getLogs(configuration); | ||
| 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()); | ||
|
|
||
| if (evalContext == null || evalContext.isEmpty()) { | ||
| SpanDataHolder spanDataHolder = new SpanDataHolder( | ||
| ContextManager.createLocalSpan(operationName), | ||
| tags, logs, spanTags, spanLogs | ||
| ); | ||
| miContext.setContext(spanDataHolder); | ||
| } else { | ||
| List<String> operationNameSuffixes = MethodConfiguration.getOperationNameSuffixes(configuration); | ||
| StringBuilder operationNameSuffix = new StringBuilder(); | ||
| if (operationNameSuffixes != null && !operationNameSuffixes.isEmpty()) { | ||
| for (String expression : operationNameSuffixes) { | ||
| operationNameSuffix.append(Constants.OPERATION_NAME_SEPARATOR); | ||
| operationNameSuffix.append(CustomizeExpression.parseExpression(expression, context)); | ||
| } | ||
| } | ||
| if (tags != null && !tags.isEmpty()) { | ||
| for (Map.Entry<String, String> expression : tags.entrySet()) { | ||
| spanTags.put(expression.getKey(), CustomizeExpression.parseExpression(expression.getValue(), context)); | ||
| } | ||
| } | ||
| if (logs != null && !logs.isEmpty()) { | ||
| for (Map.Entry<String, String> entries : logs.entrySet()) { | ||
| String expression = logs.get(entries.getKey()); | ||
| spanLogs.put(entries.getKey(), CustomizeExpression.parseExpression(expression, context)); | ||
| operationNameSuffix.append(CustomizeExpression.parseExpression(expression, evalContext)); | ||
| } | ||
| } | ||
| evalAndPopulate(evalContext, tags, spanTags); | ||
| evalAndPopulate(evalContext, logs, spanLogs); | ||
|
|
||
| operationName = operationNameSuffix.insert(0, operationName).toString(); | ||
| AbstractSpan localSpan = ContextManager.createLocalSpan(operationName); | ||
|
|
||
| AbstractSpan span = ContextManager.createLocalSpan(operationName); | ||
| if (!spanTags.isEmpty()) { | ||
| for (Map.Entry<String, String> tag : spanTags.entrySet()) { | ||
| span.tag(Tags.ofKey(tag.getKey()), tag.getValue()); | ||
| tagSpanTags(localSpan, spanTags); | ||
| tagSpanLogs(localSpan, spanLogs); | ||
|
|
||
| spanTags.clear(); | ||
| spanLogs.clear(); | ||
|
|
||
| SpanDataHolder spanDataHolder = new SpanDataHolder( | ||
| localSpan, tags, logs, spanTags, spanLogs | ||
| ); | ||
| miContext.setContext(spanDataHolder); | ||
| } | ||
| } |
There was a problem hiding this comment.
The beforeMethod method has been significantly refactored to use the V2 API and support returnedObj expressions. The logic is now split into two branches based on whether the evaluation context is empty. The code correctly handles the new SpanDataHolder pattern and properly manages span tags and logs.
Comment on lines
+103
to
+135
| void afterMethod(Method method, Object ret, MethodInvocationContext miContext) { | ||
| if (!ContextManager.isActive()) { | ||
| return; | ||
| } | ||
| SpanDataHolder spanDataHolder = (SpanDataHolder) miContext.getContext(); | ||
| if (spanDataHolder == null || spanDataHolder.localSpan == null) { | ||
| return; | ||
| } | ||
| AbstractSpan localSpan = spanDataHolder.localSpan; | ||
| if (ret == null) { | ||
| ContextManager.stopSpan(localSpan); | ||
| return; | ||
| } | ||
| Map<String, String> tags = spanDataHolder.tags; | ||
| Map<String, String> logs = spanDataHolder.logs; | ||
| Map<String, String> spanTags = spanDataHolder.spanTags; | ||
| Map<String, String> spanLogs = spanDataHolder.spanLogs; | ||
|
|
||
| try { | ||
| Map<String, Object> evalContext = CustomizeExpression.evaluationReturnContext(ret); | ||
|
|
||
| evalReturnAndPopulate(evalContext, tags, spanTags); | ||
| evalReturnAndPopulate(evalContext, logs, spanLogs); | ||
|
|
||
| tagSpanTags(localSpan, spanTags); | ||
| tagSpanLogs(localSpan, spanLogs); | ||
|
|
||
| spanTags.clear(); | ||
| spanLogs.clear(); | ||
| } finally { | ||
| ContextManager.stopSpan(localSpan); | ||
| } | ||
| } |
There was a problem hiding this comment.
The afterMethod method now properly handles return values and evaluates returnedObj expressions. The method correctly checks for active context and uses the SpanDataHolder to manage span data. The implementation ensures that return value expressions are processed before stopping the span.
Comment on lines
+141
to
+151
| private void evalAndPopulate(Map<String, Object> context, Map<String, String> exprMap, Map<String, String> toMap) { | ||
| if (exprMap != null && !exprMap.isEmpty()) { | ||
| for (Map.Entry<String, String> entry : exprMap.entrySet()) { | ||
| String expression = entry.getValue(); | ||
| if (isReturnedObjExpression(expression)) { | ||
| continue; | ||
| } | ||
| toMap.put(entry.getKey(), CustomizeExpression.parseExpression(expression, context)); | ||
| } | ||
| if (!spanLogs.isEmpty()) { | ||
| span.log(System.currentTimeMillis(), spanLogs); | ||
| } | ||
| } |
There was a problem hiding this comment.
The evalAndPopulate method correctly skips returnedObj expressions during the beforeMethod phase, ensuring they're only processed in afterMethod. The logic properly iterates through expression maps and populates the target map with evaluated expressions.
| 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 call is missing the objInst parameter that should be passed to the base class method.
Suggested change
| super.beforeMethod(method, allArguments, context); | |
| super.beforeMethod(objInst, method, allArguments, context); |
| Object ret) throws Throwable { | ||
| super.afterMethod(method); | ||
| Object ret, MethodInvocationContext context) throws Throwable { | ||
| super.afterMethod(method, ret, context); |
There was a problem hiding this comment.
The afterMethod call is missing the objInst parameter that should be passed to the base class method.
Suggested change
| super.afterMethod(method, ret, context); | |
| super.afterMethod(objInst, method, ret, context); |
| 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 call is missing the objInst parameter that should be passed to the base class method.
Suggested change
| super.handleMethodException(t); | |
| super.handleMethodException(objInst, method, allArguments, argumentsTypes, t, context); |
Comment on lines
28
to
32
| @Override | ||
| public void beforeMethod(Class clazz, Method method, Object[] allArguments, Class<?>[] parameterTypes, | ||
| MethodInterceptResult result) { | ||
| super.beforeMethod(method, allArguments); | ||
| MethodInvocationContext context) { | ||
| super.beforeMethod(method, allArguments, context); | ||
| } |
There was a problem hiding this comment.
The beforeMethod signature has been updated to include MethodInvocationContext context parameter, but the super call is missing the result parameter that was previously passed. This could break the expected behavior of the interceptor chain.
Suggested change
| @Override | |
| public void beforeMethod(Class clazz, Method method, Object[] allArguments, Class<?>[] parameterTypes, | |
| MethodInterceptResult result) { | |
| super.beforeMethod(method, allArguments); | |
| MethodInvocationContext context) { | |
| super.beforeMethod(method, allArguments, context); | |
| } | |
| public void beforeMethod(Class clazz, Method method, Object[] allArguments, Class<?>[] parameterTypes, | |
| MethodInvocationContext context) { | |
| super.beforeMethod(method, allArguments, context); | |
| } |
Comment on lines
34
to
39
| @Override | ||
| public Object afterMethod(Class clazz, Method method, Object[] allArguments, Class<?>[] parameterTypes, | ||
| Object ret) { | ||
| super.afterMethod(method); | ||
| Object ret, MethodInvocationContext context) { | ||
| super.afterMethod(method, ret, context); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
The afterMethod signature has been updated correctly, but the super call is missing the allArguments parameter that was previously passed. This could affect the evaluation of expressions that depend on method arguments.
Suggested change
| @Override | |
| public Object afterMethod(Class clazz, Method method, Object[] allArguments, Class<?>[] parameterTypes, | |
| Object ret) { | |
| super.afterMethod(method); | |
| Object ret, MethodInvocationContext context) { | |
| super.afterMethod(method, ret, context); | |
| return ret; | |
| } | |
| public Object afterMethod(Class clazz, Method method, Object[] allArguments, Class<?>[] parameterTypes, | |
| Object ret, MethodInvocationContext context) { | |
| super.afterMethod(method, allArguments, ret, context); | |
| return ret; | |
| } |
Author
|
DeepseekV3.1 |
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.
This PR adds support for the
returnedObjexpression in the APM Customize Enhance Plugin, allowing tags and logs to be populated from method return values. It also updates the plugin to use the V2 API for enhanced instrumentation.Changes by File
returnedObjexpression support.ClassInstanceMethodsEnhancePluginDefineandInstanceMethodsInterceptPointwith their V2 counterparts.returnedObjexpressions in afterMethod, adding support for return value context in tags and logs.MethodInvocationContext.returnedObjexpression usage.returnedObj.returnedObjusage.