Skip to content

Mongo Bytebuddy and Mongo Async #154

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

Jump to bottom
Merged
merged 2 commits into from
Nov 28, 2017
Merged

Mongo Bytebuddy and Mongo Async #154

merged 2 commits into from
Nov 28, 2017

Conversation

@realark
Copy link
Contributor

@realark realark commented Nov 14, 2017 •
edited
Loading

  • Convert Mongo Instrumentation to Bytebuddy
  • Add support for the mongo async driver
  • Add extensive integration testing by running a local mongod
  • Add relevant classes and versions to versionScan plugin to verify the correct driver versions are instrumented.

@realark realark added the type: enhancement Enhancements and improvements label Nov 14, 2017
@realark realark force-pushed the ark/mongo_bytebuddy branch 2 times, most recently from d7e1d45 to ed8dc1c Compare November 14, 2017 22:46
tylerbenson
tylerbenson suggested changes Nov 16, 2017
View reviewed changes
Copy link
Contributor

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

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

In addition to the rename, I'd suggest also trying to test this with a spring boot project as an executable jar. That is the way most customers are using it and I've found a good way to test the edge cases for classloader issues.

dd-java-agent/integrations/mongo/mongo.gradle Outdated
Copy link
Contributor

@tylerbenson tylerbenson Nov 16, 2017

Choose a reason for hiding this comment

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

Lets update this project name to include the version it is compatible with... (mongo-3?)

Copy link
Contributor Author

@realark realark Nov 16, 2017

Choose a reason for hiding this comment

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

👍 Good call

@realark realark changed the title Convert Mongo instrumentation to ByteBuddy Mongo Bytebuddy and Mongo Async Nov 20, 2017
@realark realark force-pushed the ark/mongo_bytebuddy branch from c3eaa39 to e37c0f5 Compare November 27, 2017 16:40
@realark realark force-pushed the ark/mongo_bytebuddy branch from e37c0f5 to 47b1eec Compare November 27, 2017 17:20
tylerbenson
tylerbenson reviewed Nov 27, 2017
View reviewed changes
...nt-ittests/src/test/java/com/datadoghq/agent/integration/MongoClientInstrumentationTest.java
private static final ListWriter writer = new ListWriter();
private static final DDTracer tracer = new DDTracer(writer);

public static void startLocalMongo() throws Exception {
Copy link
Contributor

@tylerbenson tylerbenson Nov 27, 2017

Choose a reason for hiding this comment

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

I wonder if we could take advantage of actual Cassandra and mongo instances running in CI… rather than having to start them up on the fly. (obviously using this as a fallback for non-CI execution)

Copy link
Contributor Author

@realark realark Nov 27, 2017

Choose a reason for hiding this comment

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

For mongo, I don't see a substantial performance hit running a local instance.

@realark realark merged commit e7869cd into master Nov 28, 2017
@realark realark deleted the ark/mongo_bytebuddy branch November 28, 2017 01:04
tylerbenson
tylerbenson reviewed Nov 28, 2017
View reviewed changes
dd-trace/dd-trace.gradle
testCompile group: 'cglib', name: 'cglib-nodep', version: '3.2.5'

testCompile 'org.openjdk.jmh:jmh-core:1.19'
testCompile 'org.openjdk.jmh:jmh-generator-annprocess:1.19'
Copy link
Contributor

@tylerbenson tylerbenson Nov 28, 2017

Choose a reason for hiding this comment

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

Do these belong here?

tylerbenson
tylerbenson reviewed Nov 28, 2017
View reviewed changes
Copy link
Contributor

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

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

Looks good. Retroactive approval.

@realark realark added this to the 0.2.10 milestone Dec 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@palazzem palazzem Awaiting requested review from palazzem

1 more reviewer

@tylerbenson tylerbenson tylerbenson requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type: enhancement Enhancements and improvements

Projects

None yet

Milestone

0.2.10

Development

Successfully merging this pull request may close these issues.

3 participants

@realark @tylerbenson