Dougqh/spring data support #1014

dougqh · 2019-09-24T14:56:50Z

Revised approach to adding spring-data support.

Similar to the prior approach this change uses Spring's AOP interceptors; however, rather than hooking into the TransactionInterceptor -- this change adds a Datadog specific MethodInterceptor into the Repository.

This is accomplished by instrumenting RepositoryFactorySupport constructor to add a post-processor. The post-processor then adds a MethodInterceptor at the beginning of the interceptor chain.

Tweak partially picked from signalfx -- that commit included same other unwanted changes in this file and others (see signalfx/signalfx-java-tracing@636c23e)

There are two main differences... - "hsqldb" vs "spring-data" as the root test service name - an extra seemingly spurious sql span in signalfx

Added some explanatory comments for each span -- might turn these into assertions later Primary aim was to understand the differences from the signalfx fork. They seem to stem deviations in the underlying JDBC integration.

Removing "Modified by SignalFx" comments, since our integration is current the same. Added a comment to SpringJpaTest explaining the differences.

Removing dependencies rendered redundant by clean-up in datadog repo - as per review comment

After experimenting with RepositorySupport$QueryExecutorMethodInterceptor switched to TransactionInterceptor. QueryExecutorMethodInterceptor was too narrowly scoped and didn't capture the UPDATE or DELETE statement. TransactionInterceptor captures everything, but is still a bit more broadly applicable than I'd like.

Renaming advice class to more accurately reflect what is being instrumented

Switching muzzle to target spring-tx rather than spring-data (which is logical given the instrumentation point) Also added extraDependency on aopalliance This restricts the support the Spring 2.5 when spring-tx & spring-aop using aopalliance were introduced.

The SpringDataDecorator is really for Spring transactions including non-DB related transactions. The SpringDataDecorator never really implemented DB or ORM support anyway, since most of the methods just returned null. Changing the decorator to just extend ClientDecorator instead. Also rename to reflect its true purpose.

This revised version targets spring-data RepositoryFactorySupport rather than spring-tx This is accomplished by injecting a RepositoryProxyPostProcessor during construction that adds a datadog specific MethodIntercptor. In the end, this functions similarly to the tx support in that it uses a widely scoped MethodInterceptor. This required some changes to the test as well. The old test set-up the repository before the instrumentation was fully-enabled. Enabling the instrumentation earlier capture extra traces where Spring JPA performs metainfo queries.

Renamed transactionalMethod to just method, since this is no longer tied to spring-tx

Moved spring to spring.jpa to make room for other spring data integrations.

RepositoryProxyPostProcessor has different method sigs depending on the version of spring-data-commons. As stop gap implemented both signatures, but probably need to split spring-data support by version.

tylerbenson

see below.

.../src/main/java/datadog/trace/instrumentation/springdata/SpringRepositoryInstrumentation.java

.../spring-data/src/main/java/datadog/trace/instrumentation/springdata/SpringDataDecorator.java

dd-java-agent/instrumentation/spring-data/src/test/groovy/SpringJpaTest.groovy

dd-java-agent/instrumentation/spring-data/spring-data.gradle

.../spring-data/src/main/java/datadog/trace/instrumentation/springdata/SpringDataDecorator.java

Rather than using a somewhat ugly solution of implementing both postProcess method signatures. I'm restricting the integration to newer versions (1.9 - Sept 2014) of spring-data to start.

Removing unnecessary test code left over from initial port integration

Changing directory name to spring-data-1.9 to reflect version restrictions as per review comments

dd-java-agent/instrumentation/spring-data-1.9/spring-data-1.9.gradle

After seeing the existing elasticsearch 5.3 spring data test, I decided to extend that instead. However, I do think we need to revisit the best place for tests involving multiple pieces of instrumentation.

@shared runs before the spring-data instrumentation is enabled. Switching to more explicitly set-up code to change the timing; however, this change doesn't enabled spring-data instrumentation to keep this non-functional.

This reverts commit 0dc63d7.

To shift the set-up timing without adding a lot of ugly code (as I did previously), I introduced a lazy dynamic proxy around the repository. I'm not really happy with this, but I consider it better than the prior approach. There is also probably a more "groovy" way to do this. As before, this change is itself non-functional. The subsequent test will enable spring-data instrumentation and alter the test.

Enabling the spring-data integration in this test Mostly, this introduces an extra span for the repository; however, it also connects some previously separate traces under the repository action.

dougqh · 2019-10-02T18:27:47Z

...icsearch/transport-5.3/src/test/groovy/springdata/Elasticsearch53SpringRepositoryTest.groovy


  @Shared
-  DocRepository repo = applicationContext.getBean(DocRepository)
+  DocRepository repo = Proxy.newProxyInstance(


I created this lazy proxy because otherwise Spring is configured before the spring-data instrumentation is available, but I don't particularly like it.

I could also change how the test set-up is done, but that was ugly in terms of refactoring the test.

Ultimately, I think it would be better if we enabled instrumentation before @shared set-up is run, but clearly, that would be larger change probably with significant ripple effects.

It's kind of tricky because spock controls when @Shared variables are initialized and we already apply the instrumentation as early as possible. I believe any further changes would need to be done in the test runner.

If nothing else, I would think the instrumentation itself could be enabled via @Shared. I don't love that solution, but it is probably cleaner than what I currently resorted to doing.

...icsearch/transport-5.3/src/test/groovy/springdata/Elasticsearch53SpringRepositoryTest.groovy

dougqh · 2019-10-02T18:31:22Z

dd-java-agent/instrumentation/spring-data-1.9/src/test/groovy/SpringJpaTest.groovy

+class SpringJpaTest extends AgentTestRunner {
+  def "test CRUD"() {
+    // moved inside test -- otherwise, miss the opportunity to instrument
+    def context = new AnnotationConfigApplicationContext(JpaPersistenceConfig)


Perhaps, I should employ the same proxy approach here that I ended up using for elastic.
Here I was at least able to do something simple. Elastic required care to not set-up extra nodes; otherwise, the replication count tag value started to change.

dougqh · 2019-10-02T18:32:02Z

...icsearch/transport-5.3/src/test/groovy/springdata/Elasticsearch53SpringRepositoryTest.groovy

+    DocRepository getOrCreateRepository() {
+      if ( repo != null ) return repo;
+
+      TEST_WRITER.clear()


I think a helper routine for this capture set-up traces and discard idiom might be worth adding.

dougqh · 2019-10-02T18:34:00Z

dd-java-agent/instrumentation/spring-data-1.9/src/test/groovy/SpringJpaTest.groovy

@@ -0,0 +1,160 @@
+// This file includes software developed at SignalFx


I don't like the inconsistency of having the spring-data JPA / JDBC test in the spring-data directory while have the spring-data Elastic test in the elastic directory.

As mentioned in response to prior comments, I think we should consider have separate test-only sub-modules for such combination tests.

tylerbenson

Minor change to rethrow the error and make sure the tests are passing.

tylerbenson · 2019-10-07T09:21:57Z

.../src/main/java/datadog/trace/instrumentation/springdata/SpringRepositoryInstrumentation.java

+
+      boolean isRepositoryOp = Repository.class.isAssignableFrom(clazz);
+      // non-Repository methods including Object methods will also flow through
+      // this interceptor.  Don't create spans for those.


Is this still accurate/needed?

Yes, it is still needed. I might add a test to make sure it doesn't regress.

tylerbenson · 2019-10-07T09:22:36Z

.../src/main/java/datadog/trace/instrumentation/springdata/SpringRepositoryInstrumentation.java

+      try {
+        result = methodInvocation.proceed();
+      } catch (Throwable t) {
+        DECORATOR.onError(scope, t);


Rethrow t?

Yes, that's what I asked in the other comment. I've done that.

I haven't looked carefully, but I didn't see any tests confirming our exception propagation. That seems like something that would be worth adding.

While I don't disagree, I don't think there's a universal way to test this... It'd need to be done for each integration.

Yes, you'd either have to create a helper, test it once, and use it regularly. Or, yes, we'd need to test each time.

I don't think this is really a problem for simple advice where we can trust ByteBuddy to do the right thing. But in other cases, we have error propagation code that is specific to an integration that lack tests.

dougqh · 2019-10-08T08:58:16Z

...icsearch/transport-5.3/src/test/groovy/springdata/Elasticsearch53SpringRepositoryTest.groovy

@@ -69,34 +114,23 @@ class Elasticsearch53SpringRepositoryTest extends AgentTestRunner {
    repo.index(doc) == doc

    and:
-    assertTraces(2) {
-      trace(0, 1) {
+    assertTraces(1) {


Rarely (<5%) of the time, I haven't seen index actions fail.
The reason appears to be that the RefreshAction happens early and is placed into a separate trace.

I'd like to address this is possible rather than introducing additional instability into the tests.

Added a test case that confirms that no span is creating for Object methods on the repository

tylerbenson

🎉

DataDog/dd-trace-java#1014 DataDog/dd-trace-java#1054

Ryan Fitzpatrick and others added 20 commits September 10, 2019 15:13

Initial spring data instrumentation

5f7e060

Merge branch 'master' into dougqh/spring-data

6d38831

Tweak partially picked from signalfx -- that commit included same other unwanted changes in this file and others (see signalfx/signalfx-java-tracing@636c23e)

Fixing mismatches in spans collected by signalfx fork & datadog

1335296

There are two main differences... - "hsqldb" vs "spring-data" as the root test service name - an extra seemingly spurious sql span in signalfx

Added explanatory comments to test

7a366f0

Added some explanatory comments for each span -- might turn these into assertions later Primary aim was to understand the differences from the signalfx fork. They seem to stem deviations in the underlying JDBC integration.

Removing "Modified by SignalFx"

60e9cc2

Removing "Modified by SignalFx" comments, since our integration is current the same. Added a comment to SpringJpaTest explaining the differences.

Removing "Modified by SignalFx"

ab52988

Putting in alphabetical order

5f5605f

Removing unneecessary dependencies

d2ba6af

Removing dependencies rendered redundant by clean-up in datadog repo - as per review comment

Switching to simple concat for efficiency

122533f

Google Format

85389b4

Renaming advice class

e43baf8

Renaming advice class to more accurately reflect what is being instrumented

Updating muzzle spec

172ff74

Switching muzzle to target spring-tx rather than spring-data (which is logical given the instrumentation point) Also added extraDependency on aopalliance This restricts the support the Spring 2.5 when spring-tx & spring-aop using aopalliance were introduced.

googleJavaFormat

8a89547

codeNarc - removed unused import of @shared

a242e08

Changing muzzle spec to depend on spring-data again

680c442

Renamed variable

02c7e5b

Renamed transactionalMethod to just method, since this is no longer tied to spring-tx

Adding license clause

64e3ebc

dougqh requested a review from a team as a code owner September 24, 2019 14:56

dougqh added 6 commits September 25, 2019 15:59

spring -> spring.jpa

d0cf564

Moved spring to spring.jpa to make room for other spring data integrations.

PersistenceConfig -> JpaPersistenceConfig

682cc40

Prefix with Jpa

59416bf

Tinkering with version compatibility

64a5fd6

RepositoryProxyPostProcessor has different method sigs depending on the version of spring-data-commons. As stop gap implemented both signatures, but probably need to split spring-data support by version.

Fixed typo

1e8af55

Simplify @bean annotation

3b14a1c

tylerbenson suggested changes Sep 25, 2019

View reviewed changes

dougqh added 2 commits September 26, 2019 09:29

Adjusting support version of spring-data

eb05fc7

Rather than using a somewhat ugly solution of implementing both postProcess method signatures. I'm restricting the integration to newer versions (1.9 - Sept 2014) of spring-data to start.

/ -> .

45f6edf

dougqh added 2 commits September 30, 2019 14:21

Removing unnecessary test code

8b42d28

Removing unnecessary test code left over from initial port integration

spring-data -> spring-data-1.9

02c2d43

Changing directory name to spring-data-1.9 to reflect version restrictions as per review comments

tylerbenson reviewed Oct 1, 2019

View reviewed changes

dd-java-agent/instrumentation/spring-data-1.9/spring-data-1.9.gradle Outdated Show resolved Hide resolved

dougqh added 5 commits October 1, 2019 14:07

Cleaning up gradle file

ec9e220

After seeing the existing elasticsearch 5.3 spring data test, I decided to extend that instead. However, I do think we need to revisit the best place for tests involving multiple pieces of instrumentation.

Changing test set-up timing

0dc63d7

@shared runs before the spring-data instrumentation is enabled. Switching to more explicitly set-up code to change the timing; however, this change doesn't enabled spring-data instrumentation to keep this non-functional.

Revert "Changing test set-up timing"

85240a9

This reverts commit 0dc63d7.

Enabling spring-data integration in test

dfcf9c6

Enabling the spring-data integration in this test Mostly, this introduces an extra span for the repository; however, it also connects some previously separate traces under the repository action.

dougqh commented Oct 2, 2019

View reviewed changes

...icsearch/transport-5.3/src/test/groovy/springdata/Elasticsearch53SpringRepositoryTest.groovy Show resolved Hide resolved

dougqh commented Oct 2, 2019

View reviewed changes

codenarc

001d1d8

tylerbenson suggested changes Oct 7, 2019

View reviewed changes

dougqh added 2 commits October 8, 2019 09:38

Merge branch 'master' into dougqh/spring-data-support

b01bef7

Propagating exception

5edff68

dougqh commented Oct 8, 2019

View reviewed changes

dougqh added 4 commits October 8, 2019 13:47

Clarifying comment

01b9220

Test - no span for Object methods

e696aa7

Added a test case that confirms that no span is creating for Object methods on the repository

Fixing latestDepTest

3e6419d

Simplifying latestDepTest dependencies

63ef181

tylerbenson approved these changes Oct 8, 2019

View reviewed changes

dougqh merged commit 7dbad68 into master Oct 8, 2019

dougqh deleted the dougqh/spring-data-support branch October 8, 2019 16:02

tylerbenson added this to the 0.34.0 milestone Oct 11, 2019

dougqh added a commit to DataDog/documentation that referenced this pull request Oct 30, 2019

Adding Spring Data to integrations list

af47eae

DataDog/dd-trace-java#1014 DataDog/dd-trace-java#1054

dougqh mentioned this pull request Oct 30, 2019

Java tracer v0.36.0 DataDog/documentation#5793

Closed

dougqh added a commit to DataDog/documentation that referenced this pull request Oct 30, 2019

Adding Spring Data to integrations list

8f62fb4

DataDog/dd-trace-java#1014 DataDog/dd-trace-java#1054

dougqh mentioned this pull request Oct 30, 2019

Dougqh/java tracer v0.36.0 DataDog/documentation#5797

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dougqh/spring data support #1014

Dougqh/spring data support #1014

dougqh commented Sep 24, 2019

tylerbenson left a comment

dougqh Oct 2, 2019

tylerbenson Oct 7, 2019

dougqh Oct 8, 2019

dougqh Oct 2, 2019

dougqh Oct 2, 2019

dougqh Oct 2, 2019

tylerbenson left a comment

tylerbenson Oct 7, 2019

dougqh Oct 8, 2019

tylerbenson Oct 7, 2019

dougqh Oct 8, 2019

tylerbenson Oct 8, 2019

dougqh Oct 8, 2019

dougqh Oct 8, 2019

tylerbenson left a comment

		@@ -0,0 +1,160 @@
		// This file includes software developed at SignalFx

Dougqh/spring data support #1014

Dougqh/spring data support #1014

Conversation

dougqh commented Sep 24, 2019

tylerbenson 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

tylerbenson 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

Choose a reason for hiding this comment

tylerbenson left a comment

Choose a reason for hiding this comment