Skip to content

Dougqh/spring data support #1014

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.

Sign up for GitHub

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

Merged
merged 52 commits into from
Oct 8, 2019
Merged

Dougqh/spring data support #1014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
5f7e060
Initial spring data instrumentation
Jun 10, 2019
6d38831
Merge branch 'master' into dougqh/spring-data
dougqh Sep 10, 2019
1335296
Fixing mismatches in spans collected by signalfx fork & datadog
dougqh Sep 10, 2019
7a366f0
Added explanatory comments to test
dougqh Sep 11, 2019
60e9cc2
Removing "Modified by SignalFx"
dougqh Sep 11, 2019
ab52988
Removing "Modified by SignalFx"
dougqh Sep 11, 2019
5f5605f
Putting in alphabetical order
dougqh Sep 11, 2019
d2ba6af
Removing unneecessary dependencies
dougqh Sep 11, 2019
122533f
Switching to simple concat for efficiency
dougqh Sep 16, 2019
0c3e057
Switching to instrumenting transaction support
dougqh Sep 17, 2019
85389b4
Google Format
dougqh Sep 17, 2019
e43baf8
Renaming advice class
dougqh Sep 17, 2019
172ff74
Updating muzzle spec
dougqh Sep 17, 2019
7c5e74b
Cleaning-up decorator
dougqh Sep 17, 2019
6b5acc8
New Approach - instrument RepositoryFactorySupport
dougqh Sep 23, 2019
8a89547
googleJavaFormat
dougqh Sep 23, 2019
a242e08
codeNarc - removed unused import of @Shared
dougqh Sep 23, 2019
680c442
Changing muzzle spec to depend on spring-data again
dougqh Sep 23, 2019
02c7e5b
Renamed variable
dougqh Sep 23, 2019
64e3ebc
Adding license clause
dougqh Sep 24, 2019
d0cf564
spring -> spring.jpa
dougqh Sep 25, 2019
682cc40
PersistenceConfig -> JpaPersistenceConfig
dougqh Sep 25, 2019
59416bf
Prefix with Jpa
dougqh Sep 25, 2019
64a5fd6
Tinkering with version compatibility
dougqh Sep 25, 2019
1e8af55
Fixed typo
dougqh Sep 25, 2019
3b14a1c
Simplify @Bean annotation
dougqh Sep 25, 2019
eb05fc7
Adjusting support version of spring-data
dougqh Sep 26, 2019
45f6edf
/ -> .
dougqh Sep 26, 2019
4d287c6
googleJavaFormat
dougqh Sep 26, 2019
56cebb1
Switched to extending JpaRepository
dougqh Sep 26, 2019
5cb0cc7
Preparing for ElasticSearch test
dougqh Sep 26, 2019
d192811
Fixing muzzle version
dougqh Sep 26, 2019
ac557ed
operationName -> resourceName
dougqh Sep 26, 2019
b91648d
googleJavaFormat
dougqh Sep 27, 2019
48b7018
googleJavaFormat
dougqh Sep 27, 2019
1b1cc34
Switching setResourceName -> setTag
dougqh Sep 27, 2019
3780cec
Adding setAsync
dougqh Sep 30, 2019
aeef4f4
Removing unused dependency
dougqh Sep 30, 2019
8b42d28
Removing unnecessary test code
dougqh Sep 30, 2019
02c2d43
spring-data -> spring-data-1.9
dougqh Sep 30, 2019
ec9e220
Cleaning up gradle file
dougqh Oct 1, 2019
0dc63d7
Changing test set-up timing
dougqh Oct 1, 2019
85240a9
Revert "Changing test set-up timing"
dougqh Oct 1, 2019
78f2036
Introduced lazy proxy around DocRepository
dougqh Oct 1, 2019
dfcf9c6
Enabling spring-data integration in test
dougqh Oct 2, 2019
001d1d8
codenarc
dougqh Oct 2, 2019
b01bef7
Merge branch 'master' into dougqh/spring-data-support
dougqh Oct 8, 2019
5edff68
Propagating exception
dougqh Oct 8, 2019
01b9220
Clarifying comment
dougqh Oct 8, 2019
e696aa7
Test - no span for Object methods
dougqh Oct 8, 2019
3e6419d
Fixing latestDepTest
dougqh Oct 8, 2019
63ef181
Simplifying latestDepTest dependencies
dougqh Oct 8, 2019
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
227 changes: 168 additions & 59 deletions ...earch/transport-5.3/src/test/groovy/springdata/Elasticsearch53SpringRepositoryTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,52 @@ import com.anotherchrisberry.spock.extensions.retry.RetryOnFailure
import datadog.trace.agent.test.AgentTestRunner
import datadog.trace.api.DDSpanTypes
import io.opentracing.tag.Tags
import org.springframework.context.ApplicationContext
import org.springframework.context.annotation.AnnotationConfigApplicationContext
import spock.lang.Shared
import java.lang.reflect.Proxy

import java.lang.reflect.InvocationHandler
import java.lang.reflect.Method

import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace

@RetryOnFailure(times = 3, delaySeconds = 1)
class Elasticsearch53SpringRepositoryTest extends AgentTestRunner {
@Shared
ApplicationContext applicationContext = new AnnotationConfigApplicationContext(Config)
// Setting up appContext & repo with @Shared doesn't allow
// spring-data instrumentation to applied.
// To change the timing without adding ugly checks everywhere -
// use a dynamic proxy. There's probably a more "groovy" way to do this.

@Shared
DocRepository repo = applicationContext.getBean(DocRepository)
DocRepository repo = Proxy.newProxyInstance(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

getClass().getClassLoader(),
[DocRepository] as Class[],
new LazyProxyInvoker())

static class LazyProxyInvoker implements InvocationHandler {
def repo

DocRepository getOrCreateRepository() {
if (repo != null) {
return repo
}

TEST_WRITER.clear()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

runUnderTrace("setup") {
def applicationContext = new AnnotationConfigApplicationContext(Config)
repo = applicationContext.getBean(DocRepository)
}
TEST_WRITER.waitForTraces(1)
TEST_WRITER.clear()

return repo
}

@Override
Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
return method.invoke(getOrCreateRepository(), args)
}
}

def setup() {
TEST_WRITER.clear()
Expand All @@ -36,13 +69,25 @@ class Elasticsearch53SpringRepositoryTest extends AgentTestRunner {

and:
assertTraces(1) {
trace(0, 1) {
trace(0, 2) {
span(0) {
operationName "repository.operation"
resourceName "CrudRepository.findAll"
tags {
"$Tags.COMPONENT.key" "spring-data"
"$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
defaultTags()
}
}

span(1) {
serviceName "elasticsearch"
resourceName "SearchAction"
operationName "elasticsearch.query"
spanType DDSpanTypes.ELASTICSEARCH
errored false
childOf(span(0))

tags {
"$Tags.COMPONENT.key" "elasticsearch-java"
"$Tags.DB_TYPE.key" "elasticsearch"
Expand All @@ -69,34 +114,23 @@ class Elasticsearch53SpringRepositoryTest extends AgentTestRunner {
repo.index(doc) == doc

and:
assertTraces(2) {
trace(0, 1) {
assertTraces(1) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

trace(0, 3) {
span(0) {
resourceName "IndexAction"
operationName "elasticsearch.query"
spanType DDSpanTypes.ELASTICSEARCH
resourceName "ElasticsearchRepository.index"
operationName "repository.operation"
tags {
"$Tags.COMPONENT.key" "elasticsearch-java"
"$Tags.DB_TYPE.key" "elasticsearch"
"$Tags.COMPONENT.key" "spring-data"
"$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
"elasticsearch.action" "IndexAction"
"elasticsearch.request" "IndexRequest"
"elasticsearch.request.indices" indexName
"elasticsearch.request.write.type" "doc"
"elasticsearch.request.write.version"(-3)
"elasticsearch.response.status" 201
"elasticsearch.shard.replication.failed" 0
"elasticsearch.shard.replication.successful" 1
"elasticsearch.shard.replication.total" 2
defaultTags()
}
}
}
trace(1, 1) {
span(0) {

span(1) {
resourceName "RefreshAction"
operationName "elasticsearch.query"
spanType DDSpanTypes.ELASTICSEARCH
childOf(span(0))
tags {
"$Tags.COMPONENT.key" "elasticsearch-java"
"$Tags.DB_TYPE.key" "elasticsearch"
Expand All @@ -110,6 +144,28 @@ class Elasticsearch53SpringRepositoryTest extends AgentTestRunner {
defaultTags()
}
}

span(2) {
resourceName "IndexAction"
operationName "elasticsearch.query"
spanType DDSpanTypes.ELASTICSEARCH
childOf(span(0))
tags {
"$Tags.COMPONENT.key" "elasticsearch-java"
"$Tags.DB_TYPE.key" "elasticsearch"
"$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
"elasticsearch.action" "IndexAction"
"elasticsearch.request" "IndexRequest"
"elasticsearch.request.indices" indexName
"elasticsearch.request.write.type" "doc"
"elasticsearch.request.write.version"(-3)
"elasticsearch.response.status" 201
"elasticsearch.shard.replication.failed" 0
"elasticsearch.shard.replication.successful" 1
"elasticsearch.shard.replication.total" 2
defaultTags()
}
}
}
}
TEST_WRITER.clear()
Expand All @@ -119,12 +175,23 @@ class Elasticsearch53SpringRepositoryTest extends AgentTestRunner {

and:
assertTraces(1) {
trace(0, 1) {
trace(0, 2) {
span(0) {
resourceName "CrudRepository.findById"
operationName "repository.operation"
tags {
"$Tags.COMPONENT.key" "spring-data"
"$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
defaultTags()
}
}

span(1) {
serviceName "elasticsearch"
resourceName "GetAction"
operationName "elasticsearch.query"
spanType DDSpanTypes.ELASTICSEARCH
childOf(span(0))
tags {
"$Tags.COMPONENT.key" "elasticsearch-java"
"$Tags.DB_TYPE.key" "elasticsearch"
Expand All @@ -150,12 +217,40 @@ class Elasticsearch53SpringRepositoryTest extends AgentTestRunner {
repo.findById("1").get() == doc

and:
assertTraces(3) {
trace(0, 1) {
assertTraces(2) {
trace(0, 3) {
span(0) {
resourceName "ElasticsearchRepository.index"
operationName "repository.operation"
tags {
"$Tags.COMPONENT.key" "spring-data"
"$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
defaultTags()
}
}
span(1) {
resourceName "RefreshAction"
operationName "elasticsearch.query"
spanType DDSpanTypes.ELASTICSEARCH
childOf(span(0))
tags {
"$Tags.COMPONENT.key" "elasticsearch-java"
"$Tags.DB_TYPE.key" "elasticsearch"
"$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
"elasticsearch.action" "RefreshAction"
"elasticsearch.request" "RefreshRequest"
"elasticsearch.request.indices" indexName
"elasticsearch.shard.broadcast.failed" 0
"elasticsearch.shard.broadcast.successful" 5
"elasticsearch.shard.broadcast.total" 10
defaultTags()
}
}
span(2) {
resourceName "IndexAction"
operationName "elasticsearch.query"
spanType DDSpanTypes.ELASTICSEARCH
childOf(span(0))
tags {
"$Tags.COMPONENT.key" "elasticsearch-java"
"$Tags.DB_TYPE.key" "elasticsearch"
Expand All @@ -173,31 +268,23 @@ class Elasticsearch53SpringRepositoryTest extends AgentTestRunner {
}
}
}
trace(1, 1) {
trace(1, 2) {
span(0) {
resourceName "RefreshAction"
operationName "elasticsearch.query"
spanType DDSpanTypes.ELASTICSEARCH
resourceName "CrudRepository.findById"
operationName "repository.operation"
tags {
"$Tags.COMPONENT.key" "elasticsearch-java"
"$Tags.DB_TYPE.key" "elasticsearch"
"$Tags.COMPONENT.key" "spring-data"
"$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
"elasticsearch.action" "RefreshAction"
"elasticsearch.request" "RefreshRequest"
"elasticsearch.request.indices" indexName
"elasticsearch.shard.broadcast.failed" 0
"elasticsearch.shard.broadcast.successful" 5
"elasticsearch.shard.broadcast.total" 10
defaultTags()
}
}
}
trace(2, 1) {
span(0) {

span(1) {
serviceName "elasticsearch"
resourceName "GetAction"
operationName "elasticsearch.query"
spanType DDSpanTypes.ELASTICSEARCH
childOf(span(0))
tags {
"$Tags.COMPONENT.key" "elasticsearch-java"
"$Tags.DB_TYPE.key" "elasticsearch"
Expand All @@ -222,12 +309,41 @@ class Elasticsearch53SpringRepositoryTest extends AgentTestRunner {
!repo.findAll().iterator().hasNext()

and:
assertTraces(3) {
trace(0, 1) {
assertTraces(2) {
trace(0, 3) {
span(0) {
resourceName "CrudRepository.deleteById"
operationName "repository.operation"
tags {
"$Tags.COMPONENT.key" "spring-data"
"$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
defaultTags()
}
}

span(1) {
resourceName "RefreshAction"
operationName "elasticsearch.query"
spanType DDSpanTypes.ELASTICSEARCH
childOf(span(0))
tags {
"$Tags.COMPONENT.key" "elasticsearch-java"
"$Tags.DB_TYPE.key" "elasticsearch"
"$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
"elasticsearch.action" "RefreshAction"
"elasticsearch.request" "RefreshRequest"
"elasticsearch.request.indices" indexName
"elasticsearch.shard.broadcast.failed" 0
"elasticsearch.shard.broadcast.successful" 5
"elasticsearch.shard.broadcast.total" 10
defaultTags()
}
}
span(2) {
resourceName "DeleteAction"
operationName "elasticsearch.query"
spanType DDSpanTypes.ELASTICSEARCH
childOf(span(0))
tags {
"$Tags.COMPONENT.key" "elasticsearch-java"
"$Tags.DB_TYPE.key" "elasticsearch"
Expand All @@ -244,31 +360,24 @@ class Elasticsearch53SpringRepositoryTest extends AgentTestRunner {
}
}
}
trace(1, 1) {

trace(1, 2) {
span(0) {
resourceName "RefreshAction"
operationName "elasticsearch.query"
spanType DDSpanTypes.ELASTICSEARCH
resourceName "CrudRepository.findAll"
operationName "repository.operation"
tags {
"$Tags.COMPONENT.key" "elasticsearch-java"
"$Tags.DB_TYPE.key" "elasticsearch"
"$Tags.COMPONENT.key" "spring-data"
"$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
"elasticsearch.action" "RefreshAction"
"elasticsearch.request" "RefreshRequest"
"elasticsearch.request.indices" indexName
"elasticsearch.shard.broadcast.failed" 0
"elasticsearch.shard.broadcast.successful" 5
"elasticsearch.shard.broadcast.total" 10
defaultTags()
}
}
}
trace(2, 1) {
span(0) {

span(1) {
serviceName "elasticsearch"
resourceName "SearchAction"
operationName "elasticsearch.query"
spanType DDSpanTypes.ELASTICSEARCH
childOf(span(0))
tags {
"$Tags.COMPONENT.key" "elasticsearch-java"
"$Tags.DB_TYPE.key" "elasticsearch"
Expand Down
1 change: 1 addition & 0 deletions dd-java-agent/instrumentation/elasticsearch/transport-5.3/transport-5.3.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ dependencies {

testCompile project(':dd-java-agent:instrumentation:apache-httpasyncclient-4')
testCompile project(':dd-java-agent:instrumentation:netty-4.1')
testCompile project(':dd-java-agent:instrumentation:spring-data-1.9')

testCompile group: 'org.apache.logging.log4j', name: 'log4j-core', version: '2.11.0'
testCompile group: 'org.apache.logging.log4j', name: 'log4j-api', version: '2.11.0'
Expand Down
6 changes: 3 additions & 3 deletions dd-java-agent/instrumentation/hibernate/core-4.3/src/test/groovy/SpringJpaTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import datadog.trace.agent.test.AgentTestRunner
import org.springframework.context.annotation.AnnotationConfigApplicationContext
import spock.lang.Shared
import spring.Customer
import spring.CustomerRepository
import spring.PersistenceConfig
import spring.jpa.Customer
import spring.jpa.CustomerRepository
import spring.jpa.PersistenceConfig


/**
Expand Down
2 changes: 1 addition & 1 deletion ...re-4.3/src/test/java/spring/Customer.java → ....3/src/test/java/spring/jpa/Customer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package spring;
package spring.jpa;

import javax.persistence.Entity;
import javax.persistence.GeneratedValue;
Expand Down
2 changes: 1 addition & 1 deletion .../test/java/spring/CustomerRepository.java → ...t/java/spring/jpa/CustomerRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package spring;
package spring.jpa;

import java.util.List;
import org.springframework.data.repository.CrudRepository;
Expand Down
Loading