Pr#83 branch3 DeepseekV3.1 - #3
Conversation
Sorry, something went wrong.
This comment was marked as outdated.
|
@deeployed-peer-dev review |
Sorry, something went wrong.
|
Walkthrough
This PR migrates the Customize Enhance Plugin to the V2 API and adds support for the returnedObj expression feature, enabling tags and logs to be populated from method return values. Test scenarios are updated to validate these changes. Changes
sequenceDiagram
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
Uplevel your code reviews with Deeployed Peer! |
Sorry, something went wrong.
There was a problem hiding this comment.
- 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.
- CHANGES.md
- docs/en/setup/service-agent/java-agent/Customize-enhance-trace.md
- test/plugin/scenarios/customize-scenario/config/customize_enhance.xml
- 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-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.
Sorry, something went wrong.
| */ | ||
|
|
||
| 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.
| package org.apache.skywalking.apm.agent.core.util; | |
| package org.apache.skywalking.apm.agent.core.util; |
Sorry, something went wrong.
| 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.
| import org.junit.Assert; | |
| import org.junit.Test; | |
| import org.junit.Assert; | |
| import org.junit.Test; |
Sorry, something went wrong.
There was a problem hiding this comment.
FP_Hallucination
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| 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.
| super.beforeMethod(method, allArguments, context); | |
| super.beforeMethod(objInst, method, allArguments, context); |
Sorry, something went wrong.
There was a problem hiding this comment.
FP_nitpick
Sorry, something went wrong.
| 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.
| super.afterMethod(method, ret, context); | |
| super.afterMethod(objInst, method, ret, context); |
Sorry, something went wrong.
There was a problem hiding this comment.
FP_nitpick
Sorry, something went wrong.
| 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.
| super.handleMethodException(t); | |
| super.handleMethodException(objInst, method, allArguments, argumentsTypes, t, context); |
Sorry, something went wrong.
There was a problem hiding this comment.
FP_Hallucination
Sorry, something went wrong.
| @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.
| @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); | |
| } |
Sorry, something went wrong.
There was a problem hiding this comment.
FP_Hallucination
Sorry, something went wrong.
| @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.
| @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; | |
| } |
Sorry, something went wrong.
There was a problem hiding this comment.
FP_Hallucination
Sorry, something went wrong.
|
DeepseekV3.1 |
Sorry, something went wrong.
This PR adds support for the returnedObj expression 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