Spring Framework Integration #1

andrewpowell · 2017-12-05T21:27:34Z

This integration for the Spring Framework enables the usage of aspects to trace requests down a call stack. Classes can either implement an interface or be annotated to identify themselves as available to the aspect for tracing.

jamesdbowman

Thanks for the PR Andrew. I've added a few comments around some desired functionality changed. I'd be happy to work with you to add some of these changes as well.

jamesdbowman · 2017-12-15T00:01:05Z

...recorder-sdk-spring/src/main/java/com/amazonaws/xray/spring/aop/AbstractXRayInterceptor.java

+import org.aspectj.lang.annotation.Around;
+import org.aspectj.lang.annotation.Pointcut;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;


As an SDK convention we have been relying on org.apache.commons.logging.Log for our log framework, only because it is bundled with the AWS SDK already, and keeps the logging consistent between the two. I'd be open to switching to slf4j across the SDK if a good case could be made for diverging from the AWS SDK, though.

jamesdbowman · 2017-12-15T00:05:20Z

...recorder-sdk-spring/src/main/java/com/amazonaws/xray/spring/aop/AbstractXRayInterceptor.java

+            if (AWSXRay.getCurrentSegment() != null) {
+                LOGGER.trace("Current segment exists");
+            }
+        } catch (SegmentNotFoundException snfe) {


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.

jamesdbowman · 2017-12-15T00:06:33Z

...recorder-sdk-spring/src/main/java/com/amazonaws/xray/spring/aop/AbstractXRayInterceptor.java

+            subsegment.setMetadata(this.generateMetadata(pjp, subsegment));
+            return XRayInterceptorUtils.conditionalProceed(pjp);
+        } catch (Exception e) {
+            LOGGER.error(e.getMessage());


I think we should avoid re-logging any exceptions that may pass through customer code, as they likely will be logging it already.

jamesdbowman · 2017-12-15T00:12:20Z

...ray-recorder-sdk-spring/src/main/java/com/amazonaws/xray/spring/aop/XRayJDBCInterceptor.java

+        classInfo.put("Class", pjp.getTarget().getClass().getSimpleName());
+        metadata.put("ClassInfo", classInfo);
+        String arg = (String) pjp.getArgs()[0];
+        subsegment.putSql("SQL Statement",arg);


We've reserved the keyword sanitized_query in our segment schema for such cases, would it be possible to sanitize the query of any literal values and use that keyword here? Perhaps we could also provide a query only option (non-sanitized) but we should draw user's attention to the fact that they are adding sensitive information to their traces.

andrewpowell · 2017-12-15T12:25:57Z

Thx for the feedback. I've updated the code with the suggestions and committed for review.

jamesdbowman

Just a few comments around the pom and the context missing behavior.

jamesdbowman · 2017-12-15T22:26:13Z

aws-xray-recorder-sdk-spring/pom.xml

+                    <source>1.8</source>
+                    <target>1.8</target>
+                </configuration>
+            </plugin>


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.

jamesdbowman · 2017-12-15T22:27:38Z

aws-xray-recorder-sdk-spring/pom.xml

+            <version>2.0.0.RELEASE</version>
+            <scope>provided</scope>
+        </dependency>
+        <dependency>


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

jamesdbowman · 2017-12-15T22:39:42Z

aws-xray-recorder-sdk-spring/pom.xml

+    <parent>
+        <groupId>com.amazonaws</groupId>
+        <artifactId>aws-xray-recorder-sdk-pom</artifactId>
+        <version>1.2.1</version>


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

jamesdbowman · 2017-12-15T22:40:12Z

.gitignore

@@ -5,3 +5,6 @@
 .project
 .classpath
 .DS_Store
+
+*.iml
+/.idea


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

jamesdbowman · 2017-12-15T22:40:51Z

aws-xray-recorder-sdk-spring/pom.xml

+        <dependency>
+            <groupId>com.amazonaws</groupId>
+            <artifactId>aws-xray-recorder-sdk-core</artifactId>
+            <version>1.2.0</version>


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

jamesdbowman · 2017-12-15T22:52:01Z

...recorder-sdk-spring/src/main/java/com/amazonaws/xray/spring/aop/AbstractXRayInterceptor.java

+            if (AWSXRay.getCurrentSegment() != null) {
+                logger.trace("Current segment exists");
+            }
+        } catch (SegmentNotFoundException snfe) {


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.

andrewpowell · 2017-12-16T16:50:41Z

Incorporated latest round of feedback and merged the upstream repo

# Conflicts: # aws-xray-recorder-sdk/pom.xml

jamesdbowman

Sorry just one comment about the creation of segments. Looks ready to go otherwise.

jamesdbowman · 2017-12-19T00:02:26Z

aws-xray-recorder-sdk-apache-http/pom.xml

@@ -1,5 +1,5 @@
-<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
-  xsi:schemaLocation="http://mapven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+<project xmlns="http://maven.apache.org/POM/4.0.0"


+1 thanks for fixing these across the board!! 😁

jamesdbowman · 2017-12-19T00:19:03Z

...recorder-sdk-spring/src/main/java/com/amazonaws/xray/spring/aop/AbstractXRayInterceptor.java

+    protected Object processXRayTrace(ProceedingJoinPoint pjp) throws Throwable {
+        Segment segment = getCurrentSegment();
+        if (segment == null) {
+            AWSXRay.beginSegment(pjp.getSignature().getName());


I noticed that this beginSegment remains while the endSegment logic was taken out. I'm still not sure we can get away with beginning the segments if one doesn't exist - I think we'd have to leave that up to the SegmentContext / ContextMissingStrategy chosen by the programmer.

fixed and pushed

* Adding Spring modules for X-Ray. * Adding POM description * Fixing issues highlighted from PR #1 * Removing GPG maven plugin override * Reformatting code * Homologizing logging declarations * abstracting spring version to property and removing SLF4J dependency * referencing parent version for module version * removing redundant groupId from POM * adding newline at EOF * Incorporating feedback from PR #1 * Adding newline at EOF * updating POMs * Adding module into main pom * Removing beginSegment() per comments on PR #1 * Bumping version to 1.3.0 for release of Spring integration * Instating the ability for developers to override generateMetadata to create custom metadata outside of the base metadata provoided by XRayInterceptorUtils.generateMetadata.

andrewpowell added 2 commits December 5, 2017 16:16

Adding Spring modules for X-Ray.

4bbc49b

Adding POM description

3129859

jamesdbowman reviewed Dec 15, 2017

View reviewed changes

andrewpowell added 2 commits December 15, 2017 07:22

Fixing issues highlighted from PR aws#1

1c0e4f7

Removing GPG maven plugin override

52a55cc

andrewpowell added 5 commits December 15, 2017 07:27

Reformatting code

9d29090

Homologizing logging declarations

cc58326

abstracting spring version to property and removing SLF4J dependency

96ba6af

referencing parent version for module version

b7a04c4

removing redundant groupId from POM

3549c7b

jamesdbowman reviewed Dec 15, 2017

View reviewed changes

andrewpowell added 3 commits December 16, 2017 11:37

adding newline at EOF

1a68dbf

Incorporating feedback from PR aws#1

3256748

Adding newline at EOF

ceb7267

andrewpowell added 3 commits December 16, 2017 11:55

updating POMs

011947a

Merge remote-tracking branch 'upstream/master'

be2c0f0

# Conflicts: # aws-xray-recorder-sdk/pom.xml

Adding module into main pom

3317bb2

jamesdbowman reviewed Dec 19, 2017

View reviewed changes

Removing beginSegment() per comments on PR aws#1

440fec1

jamesdbowman merged commit 01d52ae into aws:master Dec 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spring Framework Integration #1

Spring Framework Integration #1

andrewpowell commented Dec 5, 2017

jamesdbowman left a comment

jamesdbowman Dec 15, 2017

jamesdbowman Dec 15, 2017

jamesdbowman Dec 15, 2017

jamesdbowman Dec 15, 2017

andrewpowell commented Dec 15, 2017

jamesdbowman left a comment

jamesdbowman Dec 15, 2017

jamesdbowman Dec 15, 2017

jamesdbowman Dec 15, 2017

jamesdbowman Dec 15, 2017

jamesdbowman Dec 15, 2017

jamesdbowman Dec 15, 2017

andrewpowell commented Dec 16, 2017 •

edited

jamesdbowman left a comment

jamesdbowman Dec 19, 2017

jamesdbowman Dec 19, 2017

andrewpowell Dec 19, 2017

Spring Framework Integration #1

Spring Framework Integration #1

Conversation

andrewpowell commented Dec 5, 2017

jamesdbowman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewpowell commented Dec 15, 2017

jamesdbowman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewpowell commented Dec 16, 2017 • edited

jamesdbowman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewpowell commented Dec 16, 2017 •

edited