Skip to content
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.

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

MongoDB 4 driver instrumentation #2043

Merged
merged 1 commit into from Nov 2, 2020
Merged

Conversation

mcculls
Copy link
Contributor

@mcculls mcculls commented Oct 30, 2020

Covers both mongodb-driver-sync and mongodb-driver-reactivestreams clients

@mcculls mcculls added the inst: others All other instrumentations label Oct 30, 2020
@mcculls mcculls marked this pull request as ready for review October 30, 2020 15:38
@mcculls mcculls requested a review from a team as a code owner October 30, 2020 15:38
private static final BsonValue HIDDEN_CHAR = new BsonString("?");

private static BsonDocument scrub(final BsonDocument origin) {
final BsonDocument scrub = new BsonDocument();
Copy link
Member

Choose a reason for hiding this comment

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

I know we do this elsewhere, but I know from experience with this driver thatBsonDocument (having maintained a private fork) is quite heavy - every value is wrapped in a BsonValue. What we could be doing here is piping the document into a BsonWriter which writes the scrubbed value into a StringBuilder:

ScrubbingBsonWriter scrubber = new ScrubbingBsonWriter(new StringBuilder()); // implements org.bson.BsonWriter
scrubber.pipe(new BsonDocumentReader(statement));
CharSequence scrubbed = scrubber.getOutput()

I have an example of how to do this sort of thing (which just prints out the attributed weight of a BSON document) here. This will be quite handy for getting the overhead of this instrumentation down.

Copy link
Contributor Author

@mcculls mcculls Oct 30, 2020

Choose a reason for hiding this comment

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

cool - yes, this approach comes from the old MongoDB 3 instrumentation - thanks for the link

Copy link
Member

@richardstartin richardstartin left a comment

Choose a reason for hiding this comment

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

Looks reasonable, perhaps we could revisit the performance of the scrubbing for all instrumentation versions in another PR.

@richardstartin
Copy link
Member

One small request - could you fixup the commit which changes the Java version for the tests so we don't have a commit for which tests couldn't pass?

Covers both sync and reactivestreams clients
pass {
group = "org.mongodb"
module = "mongodb-driver-core"
versions = "[3.7,)"
Copy link
Contributor

Choose a reason for hiding this comment

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

This version number ideally should align with the project name. Any reason for the mismatch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comes down to how MongoDB evolved MongoClientOptions into MongoClientSettings - the v3 instrumentation works with early v3 drivers and the latest legacy drivers (both v3 and v4) that use MongoClientOptions. Whereas the v4 instrumentation works with the newer sync and reactivestreams drivers that use MongoClientSettings - but is also compatible with early releases of those drivers back to 3.7.

Since v3 based applications tend to use the legacy driver, and v4 based applications typically use the sync or reactivestreams drivers I decided it was simpler to mark the new instrumentation as v4, but declare in the muzzle spec that this will also work with earlier releases (when using the newer client API that uses MongoClientSettings).

Also because no Mongo client uses both MongoClientOptions and MongoClientSettings there's no risk of double instrumentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mcculls would you mind adding a comment in the gradle file describing your reasoning here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inst: others All other instrumentations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants