pinpoint-apm#1262 added ErrorMarking option for spring bean profile

emeroad · Nov 26, 2015 · 906e5ed · 906e5ed
1 parent bfaa1df
commit 906e5ed
Show file tree

Hide file tree

Showing 17 changed files with 219 additions and 40 deletions.
diff --git a/agent/src/main/resources/pinpoint-real-env-lowoverhead-sample.config b/agent/src/main/resources/pinpoint-real-env-lowoverhead-sample.config
@@ -292,6 +292,7 @@ profiler.spring.beans=true
 profiler.spring.beans.name.pattern=
 profiler.spring.beans.class.pattern=
 profiler.spring.beans.annotation=org.springframework.stereotype.Controller,org.springframework.stereotype.Service,org.springframework.stereotype.Repository
+profiler.spring.beans.mark.error=false
 
 ###########################################################
 # log4j (guide url : https://github.com/naver/pinpoint/blob/master/doc/per-request_feature_guide.md)

diff --git a/agent/src/main/resources/pinpoint.config b/agent/src/main/resources/pinpoint.config
@@ -328,6 +328,7 @@ profiler.spring.beans=true
 profiler.spring.beans.name.pattern=
 profiler.spring.beans.class.pattern=
 profiler.spring.beans.annotation=org.springframework.stereotype.Controller,org.springframework.stereotype.Service,org.springframework.stereotype.Repository
+profiler.spring.beans.mark.error=false
 
 ###########################################################
 # log4j (guide url : https://github.com/naver/pinpoint/blob/master/doc/per-request_feature_guide.md)

diff --git a/agent/src/test/resources/pinpoint-disabled-plugin-test.config b/agent/src/test/resources/pinpoint-disabled-plugin-test.config
@@ -197,6 +197,7 @@ profiler.spring.beans=true
 profiler.spring.beans.name.pattern=ma.*, outer
 profiler.spring.beans.class.pattern=.*Morae
 profiler.spring.beans.annotation=org.springframework.stereotype.Component
+profiler.spring.beans.mark.error=false
 
 ###########################################################
 # log4j

diff --git a/agent/src/test/resources/pinpoint-spring-bean-test.config b/agent/src/test/resources/pinpoint-spring-bean-test.config
@@ -197,6 +197,7 @@ profiler.spring.beans=true
 profiler.spring.beans.name.pattern=ma.*, outer
 profiler.spring.beans.class.pattern=.*Morae
 profiler.spring.beans.annotation=org.springframework.stereotype.Component
+profiler.spring.beans.mark.error=false
 
 ###########################################################
 # log4j

diff --git a/bootstrap-core/src/main/java/com/navercorp/pinpoint/bootstrap/context/SpanEventRecorder.java b/bootstrap-core/src/main/java/com/navercorp/pinpoint/bootstrap/context/SpanEventRecorder.java
@@ -9,6 +9,8 @@ public interface SpanEventRecorder extends FrameAttachment {
 
     void recordException(Throwable throwable);
 
+    void recordException(boolean markError, Throwable throwable);
+
     void recordApi(MethodDescriptor methodDescriptor);
 
     void recordApi(MethodDescriptor methodDescriptor, Object[] args);

diff --git a/bootstrap-core/src/main/java/com/navercorp/pinpoint/bootstrap/context/SpanRecorder.java b/bootstrap-core/src/main/java/com/navercorp/pinpoint/bootstrap/context/SpanRecorder.java
@@ -16,6 +16,8 @@ public interface SpanRecorder extends FrameAttachment {
 
     void recordException(Throwable throwable);
 
+    void recordException(boolean markError, Throwable throwable);
+
     void recordApi(MethodDescriptor methodDescriptor);
 
     void recordApi(MethodDescriptor methodDescriptor, Object[] args);

diff --git a/plugins/spring/pom.xml b/plugins/spring/pom.xml
@@ -18,5 +18,10 @@
             <artifactId>pinpoint-bootstrap-core</artifactId>
             <scope>provided</scope>
         </dependency>
+        <dependency>
+            <groupId>com.navercorp.pinpoint</groupId>
+            <artifactId>pinpoint-test</artifactId>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 </project>
diff --git a/...pring/src/main/java/com/navercorp/pinpoint/plugin/spring/beans/BeanMethodTransformer.java b/...pring/src/main/java/com/navercorp/pinpoint/plugin/spring/beans/BeanMethodTransformer.java
@@ -30,6 +30,8 @@
 import com.navercorp.pinpoint.bootstrap.logging.PLogger;
 import com.navercorp.pinpoint.bootstrap.logging.PLoggerFactory;
 
+import static com.navercorp.pinpoint.common.util.VarArgs.va;
+
 /**
  * @author Jongho Moon
  *
@@ -43,8 +45,14 @@ public class BeanMethodTransformer implements TransformCallback {
 
     private final Object lock = new Object();
     private final AtomicInteger interceptorId = new AtomicInteger(-1);
-
-
+
+    private final boolean markError;
+
+
+    public BeanMethodTransformer(boolean markError) {
+        this.markError = markError;
+    }
+
     @Override
     public byte[] doInTransform(Instrumentor instrumentor, ClassLoader loader, String className, Class<?> classBeingRedefined, ProtectionDomain protectionDomain, byte[] classfileBuffer) throws InstrumentException {
         if (logger.isInfoEnabled()) {
@@ -59,8 +67,8 @@ public byte[] doInTransform(Instrumentor instrumentor, ClassLoader loader, Strin
 
             final List<InstrumentMethod> methodList = target.getDeclaredMethods(METHOD_FILTER);
             for (InstrumentMethod method : methodList) {
-                if (logger.isTraceEnabled()) {
-                    logger.trace("### c={}, m={}, params={}", className, method.getName(), Arrays.toString(method.getParameterTypes()));
+                if (logger.isDebugEnabled()) {
+                    logger.debug("### c={}, m={}, params={}", className, method.getName(), Arrays.toString(method.getParameterTypes()));
                 }
 
                 addInterceptor(method);
@@ -89,7 +97,7 @@ private void addInterceptor(InstrumentMethod targetMethod) throws InstrumentExce
                 return;
             }
 
-            id = targetMethod.addInterceptor("com.navercorp.pinpoint.plugin.spring.beans.interceptor.BeanMethodInterceptor");
+            id = targetMethod.addInterceptor("com.navercorp.pinpoint.plugin.spring.beans.interceptor.BeanMethodInterceptor", va(markError));
             interceptorId.set(id);
         }
     }

diff --git a/...ns/spring/src/main/java/com/navercorp/pinpoint/plugin/spring/beans/SpringBeansPlugin.java b/...ns/spring/src/main/java/com/navercorp/pinpoint/plugin/spring/beans/SpringBeansPlugin.java
@@ -16,6 +16,7 @@
 
 import java.security.ProtectionDomain;
 
+import com.navercorp.pinpoint.bootstrap.config.ProfilerConfig;
 import com.navercorp.pinpoint.bootstrap.instrument.InstrumentClass;
 import com.navercorp.pinpoint.bootstrap.instrument.InstrumentException;
 import com.navercorp.pinpoint.bootstrap.instrument.InstrumentMethod;
@@ -35,6 +36,8 @@
  */
 public class SpringBeansPlugin implements ProfilerPlugin, TransformTemplateAware {
 
+    public static final String SPRING_BEANS_MARK_ERROR = "profiler.spring.beans.mark.error";
+
     private TransformTemplate transformTemplate;
 
     @Override
@@ -43,19 +46,22 @@ public void setup(ProfilerPluginSetupContext context) {
     }
 
     private void addAbstractAutowireCapableBeanFactoryTransformer(final ProfilerPluginSetupContext context) {
+        final ProfilerConfig config = context.getConfig();
+        final boolean errorMark = config.readBoolean(SPRING_BEANS_MARK_ERROR, false);
+
         transformTemplate.transform("org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory", new TransformCallback() {
 
             @Override
             public byte[] doInTransform(Instrumentor instrumentor, ClassLoader loader, String className, Class<?> classBeingRedefined, ProtectionDomain protectionDomain, byte[] classfileBuffer) throws InstrumentException {
                 InstrumentClass target = instrumentor.getInstrumentClass(loader, className, classfileBuffer);
 
-                BeanMethodTransformer beanTransformer = new BeanMethodTransformer();
-                ObjectFactory beanFilterFactory = ObjectFactory.byStaticFactory("com.navercorp.pinpoint.plugin.spring.beans.interceptor.TargetBeanFilter", "of", context.getConfig());
+                final BeanMethodTransformer beanTransformer = new BeanMethodTransformer(errorMark);
+                final ObjectFactory beanFilterFactory = ObjectFactory.byStaticFactory("com.navercorp.pinpoint.plugin.spring.beans.interceptor.TargetBeanFilter", "of", config);
 
-                InstrumentMethod createBeanInstance = target.getDeclaredMethod("createBeanInstance", "java.lang.String", "org.springframework.beans.factory.support.RootBeanDefinition", "java.lang.Object[]");
+                final InstrumentMethod createBeanInstance = target.getDeclaredMethod("createBeanInstance", "java.lang.String", "org.springframework.beans.factory.support.RootBeanDefinition", "java.lang.Object[]");
                 createBeanInstance.addInterceptor("com.navercorp.pinpoint.plugin.spring.beans.interceptor.CreateBeanInstanceInterceptor", va(beanTransformer, beanFilterFactory));
 
-                InstrumentMethod postProcessor = target.getDeclaredMethod("applyBeanPostProcessorsBeforeInstantiation", "java.lang.Class", "java.lang.String");
+                final InstrumentMethod postProcessor = target.getDeclaredMethod("applyBeanPostProcessorsBeforeInstantiation", "java.lang.Class", "java.lang.String");
                 postProcessor.addInterceptor("com.navercorp.pinpoint.plugin.spring.beans.interceptor.PostProcessorInterceptor", va(beanTransformer, beanFilterFactory));
 
                 return target.toBytecode();

diff --git a/...in/java/com/navercorp/pinpoint/plugin/spring/beans/interceptor/BeanMethodInterceptor.java b/...in/java/com/navercorp/pinpoint/plugin/spring/beans/interceptor/BeanMethodInterceptor.java
@@ -35,9 +35,11 @@ public class BeanMethodInterceptor implements ApiIdAwareAroundInterceptor {
     private final boolean isDebug = logger.isDebugEnabled();
 
     private final TraceContext traceContext;
+    private final boolean markError;
 
-    public BeanMethodInterceptor(TraceContext traceContext) {
+    public BeanMethodInterceptor(TraceContext traceContext, boolean markError) {
         this.traceContext = traceContext;
+        this.markError = markError;
     }
 
     @Override
@@ -69,7 +71,7 @@ public void after(Object target, int apiId, Object[] args, Object result, Throwa
         try {
             final SpanEventRecorder recorder = trace.currentSpanEventRecorder();
             recorder.recordApi(new DummyMethodDescriptor(apiId));
-            recorder.recordException(throwable);
+            recorder.recordException(markError, throwable);
         } finally {
             trace.traceBlockEnd();
         }

diff --git a/...ava/com/navercorp/pinpoint/plugin/spring/beans/interceptor/BeanMethodInterceptorTest.java b/...ava/com/navercorp/pinpoint/plugin/spring/beans/interceptor/BeanMethodInterceptorTest.java
@@ -0,0 +1,54 @@
+/*
+ * *
+ *  * Copyright 2014 NAVER Corp.
+ *  * Licensed under the Apache License, Version 2.0 (the "License");
+ *  * you may not use this file except in compliance with the License.
+ *  * You may obtain a copy of the License at
+ *  *
+ *  *     http://www.apache.org/licenses/LICENSE-2.0
+ *  *
+ *  * Unless required by applicable law or agreed to in writing, software
+ *  * distributed under the License is distributed on an "AS IS" BASIS,
+ *  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  * See the License for the specific language governing permissions and
+ *  * limitations under the License.
+ *
+ */
+
+package com.navercorp.pinpoint.plugin.spring.beans.interceptor;
+
+import com.navercorp.pinpoint.bootstrap.context.SpanEventRecorder;
+import com.navercorp.pinpoint.bootstrap.context.Trace;
+import com.navercorp.pinpoint.test.mock.MockTraceContext;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import static org.mockito.Mockito.times;
+
+/**
+ * @author Woonduk Kang(emeroad)
+ */
+public class BeanMethodInterceptorTest {
+
+    @Test
+    public void testBefore() throws Exception {
+
+    }
+
+    @Test
+    public void testAfter() throws Exception {
+        final MockTraceContext traceContext = new MockTraceContext();
+        final Trace trace = Mockito.mock(Trace.class);
+        traceContext.setTrace(trace);
+        final SpanEventRecorder recorder = Mockito.mock(SpanEventRecorder.class);
+        Mockito.when(trace.currentSpanEventRecorder()).thenReturn(recorder);
+
+        final BeanMethodInterceptor beanMethodInterceptor = new BeanMethodInterceptor(traceContext, true);
+
+        Object thisObject = new Object();
+        final Exception throwable = new Exception();
+        beanMethodInterceptor.after(thisObject, 10, null, null, throwable);
+
+        Mockito.verify(recorder, times(1)).recordException(true, throwable);
+    }
+}
diff --git a/profiler/src/main/java/com/navercorp/pinpoint/profiler/context/AbstractRecorder.java b/profiler/src/main/java/com/navercorp/pinpoint/profiler/context/AbstractRecorder.java
@@ -34,17 +34,23 @@ public AbstractRecorder(final TraceContext traceContext) {
         this.traceContext = traceContext;
     }
 
-    public void recordException(Throwable th) {
-        if (th == null) {
+    public void recordException(Throwable throwable) {
+        recordException(true, throwable);
+    }
+
+    public void recordException(boolean markError, Throwable throwable) {
+        if (throwable == null) {
             return;
         }
-        final String drop = StringUtils.drop(th.getMessage(), 256);
+        final String drop = StringUtils.drop(throwable.getMessage(), 256);
         // An exception that is an instance of a proxy class could make something wrong because the class name will vary.
-        final int exceptionId = traceContext.cacheString(th.getClass().getName());
-        setExceptionInfo(exceptionId, drop);
+        final int exceptionId = traceContext.cacheString(throwable.getClass().getName());
+        setExceptionInfo(markError, exceptionId, drop);
     }
 
     abstract void setExceptionInfo(int exceptionClassId, String exceptionMessage);
+
+    abstract void setExceptionInfo(boolean markError, int exceptionClassId, String exceptionMessage);
 
     public void recordApi(MethodDescriptor methodDescriptor) {
         if (methodDescriptor == null) {

diff --git a/profiler/src/main/java/com/navercorp/pinpoint/profiler/context/DefaultSpanRecorder.java b/profiler/src/main/java/com/navercorp/pinpoint/profiler/context/DefaultSpanRecorder.java
@@ -60,9 +60,16 @@ public void recordStartTime(long startTime) {
 
     @Override
     void setExceptionInfo(int exceptionClassId, String exceptionMessage) {
+        setExceptionInfo(true, exceptionClassId, exceptionMessage);
+    }
+
+    @Override
+    void setExceptionInfo(boolean markError, int exceptionClassId, String exceptionMessage) {
         span.setExceptionInfo(exceptionClassId, exceptionMessage);
-        if (!span.isSetErrCode()) {
-            span.setErrCode(1);
+        if (markError) {
+            if (!span.isSetErrCode()) {
+                span.setErrCode(1);
+            }
         }
     }
 

diff --git a/profiler/src/main/java/com/navercorp/pinpoint/profiler/context/WrappedSpanEventRecorder.java b/profiler/src/main/java/com/navercorp/pinpoint/profiler/context/WrappedSpanEventRecorder.java
@@ -122,9 +122,17 @@ public void recordAsyncSequence(short asyncSequence) {
 
     @Override
     void setExceptionInfo(int exceptionClassId, String exceptionMessage) {
+        this.setExceptionInfo(true, exceptionClassId, exceptionMessage);
+    }
+
+    @Override
+    void setExceptionInfo(boolean markError, int exceptionClassId, String exceptionMessage) {
         spanEvent.setExceptionInfo(exceptionClassId, exceptionMessage);
-        if (!spanEvent.getSpan().isSetErrCode()) {
-            spanEvent.getSpan().setErrCode(1);
+        if (markError) {
+            final Span span = spanEvent.getSpan();
+            if (!span.isSetErrCode()) {
+                span.setErrCode(1);
+            }
         }
     }
 

diff --git a/...src/main/java/com/navercorp/pinpoint/profiler/objectfactory/AutoBindingObjectFactory.java b/...src/main/java/com/navercorp/pinpoint/profiler/objectfactory/AutoBindingObjectFactory.java
@@ -49,8 +49,8 @@ public AutoBindingObjectFactory(InstrumentContext pluginContext, ClassLoader cla
     }
 
     public Object createInstance(ObjectFactory objectFactory, ArgumentProvider... providers) {
-        Class<?> type = pluginContext.injectClass(classLoader, objectFactory.getClassName());
-        ArgumentsResolver argumentsResolver = getArgumentResolver(objectFactory, providers);
+        final Class<?> type = pluginContext.injectClass(classLoader, objectFactory.getClassName());
+        final ArgumentsResolver argumentsResolver = getArgumentResolver(objectFactory, providers);
 
         if (objectFactory instanceof ByConstructor) {
             return byConstructor(type, (ByConstructor) objectFactory, argumentsResolver);
@@ -61,18 +61,18 @@ public Object createInstance(ObjectFactory objectFactory, ArgumentProvider... pr
         throw new IllegalArgumentException("Unknown objectFactory type: " + objectFactory);
     }
 
-    private Object byConstructor(Class<?> type, ByConstructor recipe, ArgumentsResolver argumentsResolver) {
-        ConstructorResolver resolver = new ConstructorResolver(type, argumentsResolver);
+    private Object byConstructor(Class<?> type, ByConstructor byConstructor, ArgumentsResolver argumentsResolver) {
+        final ConstructorResolver resolver = new ConstructorResolver(type, argumentsResolver);
 
         if (!resolver.resolve()) {
             throw new PinpointException("Cannot find suitable constructor for " + type.getName());
         }
 
-        Constructor<?> constructor = resolver.getResolvedConstructor();
-        Object[] resolvedArguments = resolver.getResolvedArguments();
+        final Constructor<?> constructor = resolver.getResolvedConstructor();
+        final Object[] resolvedArguments = resolver.getResolvedArguments();
 
         if (isDebug) {
-            logger.debug("Create insatnce by constructor {}, with arguments {}", constructor, Arrays.toString(resolvedArguments));
+            logger.debug("Create instance by constructor {}, with arguments {}", constructor, Arrays.toString(resolvedArguments));
         }
 
         try {
@@ -81,35 +81,35 @@ private Object byConstructor(Class<?> type, ByConstructor recipe, ArgumentsResol
             throw new PinpointException("Fail to invoke constructor: " + constructor + ", arguments: " + Arrays.toString(resolvedArguments), e);
         }
     }
-    
-    private Object byStaticFactoryMethod(Class<?> type, ByStaticFactoryMethod recipe, ArgumentsResolver argumentsResolver) {
-        StaticMethodResolver resolver = new StaticMethodResolver(type, recipe.getFactoryMethodName(), argumentsResolver);
+
+    private Object byStaticFactoryMethod(Class<?> type, ByStaticFactoryMethod staticFactoryMethod, ArgumentsResolver argumentsResolver) {
+        StaticMethodResolver resolver = new StaticMethodResolver(type, staticFactoryMethod.getFactoryMethodName(), argumentsResolver);
 
         if (!resolver.resolve()) {
-            throw new PinpointException("Cannot find suitable factory method " + type.getName() + "." + recipe.getFactoryMethodName());
+            throw new PinpointException("Cannot find suitable factory method " + type.getName() + "." + staticFactoryMethod.getFactoryMethodName());
         }
 
-        Method method = resolver.getResolvedMethod();
-        Object[] resolvedArguments = resolver.getResolvedArguments();
+        final Method method = resolver.getResolvedMethod();
+        final Object[] resolvedArguments = resolver.getResolvedArguments();
 
         if (isDebug) {
-            logger.debug("Create insatnce by static factory method {}, with arguments {}", method, Arrays.toString(resolvedArguments));
+            logger.debug("Create instance by static factory method {}, with arguments {}", method, Arrays.toString(resolvedArguments));
         }
 
         try {
             return method.invoke(null, resolvedArguments);
         } catch (Exception e) {
-            throw new PinpointException("Fail to invoke factory method: " + type.getName() + "." + recipe.getFactoryMethodName() + ", arguments: " + Arrays.toString(resolvedArguments), e);
+            throw new PinpointException("Fail to invoke factory method: " + type.getName() + "." + staticFactoryMethod.getFactoryMethodName() + ", arguments: " + Arrays.toString(resolvedArguments), e);
         }
 
     }
 
-    private ArgumentsResolver getArgumentResolver(ObjectFactory recipe, ArgumentProvider[] providers) {
-        List<ArgumentProvider> merged = new ArrayList<ArgumentProvider>(commonProviders);
+    private ArgumentsResolver getArgumentResolver(ObjectFactory objectFactory, ArgumentProvider[] providers) {
+        final List<ArgumentProvider> merged = new ArrayList<ArgumentProvider>(commonProviders);
         merged.addAll(Arrays.asList(providers));
 
-        if (recipe.getArguments() != null) { 
-            merged.add(new OrderedValueProvider(this, recipe.getArguments()));
+        if (objectFactory.getArguments() != null) {
+            merged.add(new OrderedValueProvider(this, objectFactory.getArguments()));
         }
 
         return new ArgumentsResolver(merged);