Skip to content

Conversation

michaelpigg
Copy link

@michaelpigg michaelpigg commented Jun 1, 2020

Adding support for Scala Phantom library https://github.com/outworkers/phantom

Strategy is to add a completion listener to the Future returned from the future method.

TODO:

@michaelpigg michaelpigg requested a review from a team as a code owner June 1, 2020 14:03
@tylerbenson tylerbenson added the tag: community Community contribution label Jun 1, 2020
Copy link
Contributor

@devinsba devinsba left a comment

Choose a reason for hiding this comment

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

We definitely want to add in latestDepTest and clean up commented out code but otherwise this is looking good. Thanks


where:
id | cmd | cql | ec
UUID.randomUUID() | insertBook | "UPDATE books.books SET title = 'Programming in Scala', author = 'Odersky' WHERE id = " + id + ";" | scala.concurrent.ExecutionContext$.MODULE$.global()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the cassandra instrumentation adding in the query parameters here, or is this the phantom instrumentation? If it's phantom we we would not want to do that by default. We don't want users potentially sending secrets to us without realizing it

Copy link
Author

Choose a reason for hiding this comment

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

I was wondering about this. Is there something in the cassandra instrumentation that avoids this issue?

testCompile deps.scala
testCompile group: 'org.scala-lang', name: 'scala-reflect', version: '2.11.12'
testCompile group: 'org.cassandraunit', name: 'cassandra-unit', version: '3.11.2.0'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally we like to have a latestDepTest task that would test the latest version of phantom that this instrumentation supports

apply from: "${rootDir}/gradle/test-with-scala.gradle"

apply plugin: 'org.unbroken-dome.test-sets'

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I missed this. We also need a muzzle check. This gives us a test that the instrumentation activates when we expect it to, and optionally doesn't activate when we expect it not to

@michaelpigg
Copy link
Author

@devinsba Can you help me understand why I get the error
java.lang.AssertionError: Span is active before test has started
in the test? It seems that the span from the previous test is being found, but I'm not clear why it would still be active.

https://app.circleci.com/pipelines/github/DataDog/dd-trace-java/1216/workflows/e6917aa6-9bf9-4ec1-923f-ae320e29a966/jobs/64994/tests

Copy link
Contributor

@randomanderson randomanderson left a comment

Choose a reason for hiding this comment

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

  • Can't use a static port or it's fail in CI
  • Run ./gradlew googleJavaFormat to fix formatting

@Shared
Cluster cluster
@Shared
int port = 9142
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
int port = 9142
int port = PortUtils.randomOpenPort();

Copy link
Author

Choose a reason for hiding this comment

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

Updated to use random port

@tylerbenson
Copy link
Contributor

@michaelpigg Do you need help finishing this up?

@michaelpigg
Copy link
Author

@michaelpigg Do you need help finishing this up?

@tylerbenson I think my main issue is #1515 (comment). I can't figure out why the phantom span doesn't seem to finish cleanly. The scope manager thinks it's still active when the test tries to close the "parent" span.

@tylerbenson
Copy link
Contributor

@michaelpigg thanks for the update. @devinsba will take a look at this next week.

@devinsba
Copy link
Contributor

devinsba commented Jul 6, 2020

@michaelpigg checking this morning

@tylerbenson
Copy link
Contributor

@michaelpigg can we close this in favor of #1665 since that seems to be the underlying problem?

@michaelpigg
Copy link
Author

@tylerbenson Closing for now. May want to bring it back at some point.

@tylerbenson tylerbenson added this to the Closed milestone Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag: community Community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants