Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spring Framework Integration #1

Merged
merged 16 commits into from
Dec 19, 2017
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,6 @@
.project
.classpath
.DS_Store

*.iml
/.idea
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick but it may mess up other users' editors - can we add newlines at EOF where missing?

84 changes: 84 additions & 0 deletions aws-xray-recorder-sdk-spring/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>com.amazonaws</groupId>
<artifactId>aws-xray-recorder-sdk-pom</artifactId>
<version>1.2.1</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the trouble but the version has bumped to 1.2.2 since this was posted, would it be possible to increment?

</parent>

<artifactId>aws-xray-recorder-sdk-spring</artifactId>
<version>${parent.version}</version>
<name>AWS X-Ray Recorder SDK for Java - Spring Framework Interceptors</name>

<properties>
<spring.version>4.3.12.RELEASE</spring.version>
</properties>

<dependencies>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-context-support</artifactId>
<version>${spring.version}</version>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-context</artifactId>
<version>${spring.version}</version>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-aspects</artifactId>
<version>${spring.version}</version>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>com.amazonaws</groupId>
<artifactId>aws-xray-recorder-sdk-core</artifactId>
<version>1.2.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using <version>${awsxrayrecordersdk.version}</version> will allow this submodule to directly depend on the latest version of the core module.

<scope>compile</scope>
</dependency>
<dependency>
<groupId>org.aspectj</groupId>
<artifactId>aspectjrt</artifactId>
<version>1.8.11</version>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-commons</artifactId>
<version>2.0.0.RELEASE</version>
<scope>provided</scope>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I'm missing something but it doesn't look like this dependency on javax.servlet.servlet-api is used in the submodule.

<groupId>javax.servlet</groupId>
<artifactId>servlet-api</artifactId>
<version>2.5</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.12</version>
<scope>test</scope>
</dependency>
</dependencies>

<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<compilerVersion>1.8</compilerVersion>
<fork>true</fork>
<source>1.8</source>
<target>1.8</target>
</configuration>
</plugin>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add the Javadoc plugin to align this with other submodule poms?

        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-javadoc-plugin</artifactId>
        <version>2.10.4</version>

Additionally, it may be useful to consider adding another <group> tag in the main pom.xml to ensure these com.amazonaws.xray.spring.* classes are documented together in a group.

</plugins>
</build>
</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
package com.amazonaws.xray.spring.aop;

import com.amazonaws.xray.AWSXRay;
import com.amazonaws.xray.entities.Subsegment;
import com.amazonaws.xray.exceptions.SegmentNotFoundException;
import com.amazonaws.xray.strategy.ContextMissingStrategy;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.aspectj.lang.ProceedingJoinPoint;
import org.aspectj.lang.annotation.Around;
import org.aspectj.lang.annotation.Pointcut;

public abstract class AbstractXRayInterceptor {

private static final Log logger = LogFactory.getLog(AbstractXRayInterceptor.class);

private static ContextMissingStrategy getContextMissingStrategy() {
return AWSXRay.getGlobalRecorder().getContextMissingStrategy();
}

/**
* @param pjp the proceeding join point
* @return the result of the method being wrapped
* @throws Throwable
*/
@Around("xrayTracedClasses() || xrayEnabledClasses()")
public Object traceAroundMethods(ProceedingJoinPoint pjp) throws Throwable {
return this.processXRayTrace(pjp);
}

protected Object processXRayTrace(ProceedingJoinPoint pjp) throws Throwable {
boolean endSegment = false;
try {
if (AWSXRay.getCurrentSegment() != null) {
logger.trace("Current segment exists");
}
} catch (SegmentNotFoundException snfe) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The segment not found cases should be passed to the current recorder's ContextMissingStrategy. We try to protect users from inadvertent tracing context loss (happens most often when trying to trace multi-threaded operations). In order to protect users from losing their tracing context, we must by default throw an exception when a user tries to create a subsegment with no segment in progress. Of course, users may override this if they desire. But the default behavior should pass the exception onto the context missing strategy and allow that strategy to create a default segment, if it so desires.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if my previous comment regarding context missing strategies was unclear. The logic inside the getCurrentSegment() function should reference the context missing strategy as needed - and will only throw the SegmentNotFoundException if the strategy calls for it.

    public Segment getCurrentSegment() {
        Optional<Segment> segment = getCurrentSegmentOptional();
        if (segment.isPresent()) {
            return segment.get();
        }
        contextMissingStrategy.contextMissing("No segment in progress.", SegmentNotFoundException.class);
        return null;
    }

So, there should be no need to catch exceptions here. And the boolean endSegment shouldn't be required either. We can call getSegment() and compare the value against null to determine whether or not a subsegment should be created here.

ContextMissingStrategy contextMissingStrategy = getContextMissingStrategy();
contextMissingStrategy.contextMissing("Context Missing from Spring Interceptor", snfe.getClass());
endSegment = true;
}

try {
Subsegment subsegment = AWSXRay.beginSubsegment(pjp.getSignature().getName());
subsegment.setMetadata(XRayInterceptorUtils.generateMetadata(pjp, subsegment));
return XRayInterceptorUtils.conditionalProceed(pjp);
} catch (Exception e) {
AWSXRay.getCurrentSegment().addException(e);
throw e;
} finally {
logger.trace("Ending Subsegment");
AWSXRay.endSubsegment();
if (endSegment) {
logger.trace("Ending Segment");
AWSXRay.endSegment();
}
}
}


protected abstract void xrayEnabledClasses();

@Pointcut("execution(* XRayTraced+.*(..))")
protected void xrayTracedClasses() {
}

@Pointcut("execution(public !void org.springframework.data.repository.Repository+.*(..))")
protected void springRepositories() {
}

/**
* @param pjp the proceeding join point
* @return the result of the method being wrapped
* @throws Throwable
*/
@Around("springRepositories()")
public Object traceAroundRepositoryMethods(ProceedingJoinPoint pjp) throws Throwable {
logger.trace("Advising repository");
boolean hasClassAnnotation = false;

for (Class<?> i : pjp.getTarget().getClass().getInterfaces()) {
if (i.getAnnotation(XRayEnabled.class) != null) {
hasClassAnnotation = true;
break;
}
}

if (hasClassAnnotation) {
return this.processXRayTrace(pjp);
} else {
return XRayInterceptorUtils.conditionalProceed(pjp);
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package com.amazonaws.xray.spring.aop;

import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;

@Retention(RetentionPolicy.RUNTIME)
public @interface XRayEnabled {


}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package com.amazonaws.xray.spring.aop;

import com.amazonaws.xray.entities.Subsegment;
import org.aspectj.lang.ProceedingJoinPoint;

import java.util.HashMap;
import java.util.Map;

public class XRayInterceptorUtils {

public static Object conditionalProceed(ProceedingJoinPoint pjp) throws Throwable {
if (pjp.getArgs().length == 0) {
return pjp.proceed();
} else {
return pjp.proceed(pjp.getArgs());
}
}

public static Map<String, Map<String, Object>> generateMetadata(ProceedingJoinPoint pjp, Subsegment subsegment) {
final Map<String, Map<String, Object>> metadata = new HashMap<>();
final Map<String, Object> classInfo = new HashMap<>();
classInfo.put("Class", pjp.getTarget().getClass().getSimpleName());
metadata.put("ClassInfo", classInfo);
return metadata;
}


}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package com.amazonaws.xray.spring.aop;

import com.amazonaws.xray.AWSXRay;
import com.amazonaws.xray.entities.Subsegment;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.aspectj.lang.ProceedingJoinPoint;
import org.aspectj.lang.annotation.Around;
import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.annotation.Pointcut;
import org.springframework.beans.factory.annotation.Configurable;

@Aspect
@Configurable
public class XRaySpringDataInterceptor {

private static final Log logger = LogFactory.getLog(XRaySpringDataInterceptor.class);

@Around("queryExecution()")
public Object traceSQL(ProceedingJoinPoint pjp) throws Throwable {
try {
Subsegment subsegment = AWSXRay.beginSubsegment(pjp.getSignature().getName());
XRayInterceptorUtils.generateMetadata(pjp, subsegment);
return XRayInterceptorUtils.conditionalProceed(pjp);
} catch (Exception e) {
logger.error(e.getMessage());
AWSXRay.getCurrentSegment().addException(e);
throw e;
} finally {
logger.trace("Ending Subsegment");
AWSXRay.endSubsegment();
}
}

@Pointcut("execution(public !void java.sql.Statement.execute*(java.lang.String))")
private void queryExecution() {
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
package com.amazonaws.xray.spring.aop;

public interface XRayTraced {
}
1 change: 1 addition & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
<module>aws-xray-recorder-sdk-core</module>
<module>aws-xray-recorder-sdk-sql-mysql</module>
<module>aws-xray-recorder-sdk-sql-postgres</module>
<module>aws-xray-recorder-sdk-spring</module>
</modules>
<distributionManagement>
<repository>
Expand Down