Migrating to mongo client 4.x #2493

jonatan-ivanov · 2021-03-10T00:46:22Z

Resolves #2338

…odb.driver.pool.waitqueuesize

onobc

@jonatan-ivanov I added a few comments. Thx for doing this!

.../src/main/java/io/micrometer/core/instrument/binder/mongodb/MongoMetricsCommandListener.java

...in/java/io/micrometer/core/instrument/binder/mongodb/MongoMetricsConnectionPoolListener.java

.../test/java/io/micrometer/core/instrument/binder/mongodb/MongoMetricsCommandListenerTest.java

onobc · 2021-03-10T02:45:39Z

...ava/io/micrometer/core/instrument/binder/mongodb/MongoMetricsConnectionPoolListenerTest.java

@@ -68,7 +73,8 @@ public void clusterOpening(ClusterOpeningEvent event) {

        assertThat(registry.get("mongodb.driver.pool.size").tags(tags).gauge().value()).isEqualTo(2);
        assertThat(registry.get("mongodb.driver.pool.checkedout").gauge().value()).isZero();
-        assertThat(registry.get("mongodb.driver.pool.waitqueuesize").gauge().value()).isZero();
+        // TODO: waitQueueEntered and waitQueueExited were removed, how can we provide mongodb.driver.pool.waitqueuesize?


Since Mongo driver stopped emitting these - and it was labeled as "going to be removed in future release in Mongo 3.x" I think we don't provide it. I think instead we can provide the metrics for these new events.

Do you happen to know anything from the Mongo team we could reference in our release notes? I asked about it in #2493 (comment) before I saw your review comments.

Not from the Mongo team per-se. However, this deprecation list (posted in 3.12) was super helpful when we migrated our apps to 4.x.

And drilling down directly into the event in question you see this loud warning:

⚠️ Deprecated. In the next major release, this event will no longer be published

The driver 4.0 upgrade docs coupled w/ the above info (which also references the 3.12 deprecation list) should suffice.

I see. We can certainly reference that. I was hoping for any reasoning why it was deprecated and removed, but lacking that, we can use the deprecation list you linked. As a user, did you ever use the wait queue metrics? Is there removal an issue for you or do you think it might be for other users?
Edit: I should check all the updates before commenting. I see you've answered my question already in #2493 (comment)

That JIRA issue is in connection with what I found: https://github.com/mongodb/specifications/blob/master/source/connection-monitoring-and-pooling/connection-monitoring-and-pooling.rst#why-are-waitqueuesize-and-waitqueuemultiple-deprecated

I'm going to ask the mongo community; ConcurrentPool has public getCount, getInUseCount, getAvailableCount but its interface does not and getting a reference could also be tricky.

Between the deprecated list, release notes, and this spec outlining why the waitQueue metrics are deprecated it feels like the waitQueue metrics can be omitted.

I meant to ask this earlier. How are docs handled in Micrometer? Do we need to open a "documentation ticket" for the removal of waitqueue metric?

Depending on the outcome of the thread in the mongo forum, this can go to javadoc/release-notes/micrometer-docs.

Good call in posting on that forum @jonatan-ivanov - looks like we got some direction. It does sound like this will give roughly the same as waitqueue. I understand metrics are APIs and its nice to not break consumers. However, I feel like we should deprecate that metric as Mongo does not emit it anymore. What are your thoughts Jonatan and @shakuzen ?

Also, what is that process like for Micrometer metrics?

I don't think any of the metrics backends we support have a concept of a deprecated metric (though it is an interesting idea), so unfortunately we're limited by what we can do to inform users. Release notes are typically where we would call out a removal. I agree we should consider whether it is an important metric for users or not. It seems the Mongo team thinks it isn't something to be tracked, judging from their removal of it. If there are Micrometer and MongoDB users out there that find it useful, we'd like to hear about that. We could drop an ask in the release notes and remove it in the next minor version if we don't hear people are using it.

...in/java/io/micrometer/core/instrument/binder/mongodb/MongoMetricsConnectionPoolListener.java

shakuzen · 2021-03-10T02:56:17Z

...in/java/io/micrometer/core/instrument/binder/mongodb/MongoMetricsConnectionPoolListener.java

@@ -79,6 +79,11 @@ public void connectionPoolClosed(ConnectionPoolClosedEvent event) {
        waitQueueSize.remove(serverId);
    }

+//    @Override
+//    public void connectionPoolCleared(ConnectionPoolClearedEvent event) {
+//        // TODO: Do we need to implement this?


Is the ConnectionClosedEvent called for each connection when this happens? If so, perhaps we don't need to do anything with this event. The pool is still open and usable after it is cleared, right?

Good questions @shakuzen . Seeing this plus your other comment about the other new event types I am in favor of moving these "new event support" to a follow on issue. A little bit of digging on the new events needs to be done.

This ticket is really to move from 3.x -> 4.x and this current proposal puts us in parity w/ 3.x sans "waitqueuesize".

It is called when the pool is invalidated. This happens in error scenarios but nothing particular happens with the pool in this case. If the connection happens to be closed after this, as @shakuzen said a ConnectionClosedEvent will be emitted so we seems to be ok not implementing this.

If the connection happens to be closed

Does it get closed after? I mean, that makes sense that it would, I just did not see it when glancing through the code (I did not pull down locally though either). Either way though, I think its fine to omit it now. If someone wants it they will file an issue.

This happens in error scenarios (e.g.: not able to connect) and what happens depends on when do you get the error:

It happens before the connection checked out:
This means the connection was not in use, there is a retry, eventually there is a timeout and the connection will be closed

It happens after the connection checked out:
This means that the connection was in use, there will be a check in and then a close which is good since it takes care about the events we need

Check out the ConcurrentPool (release, prune, close) and the maintenance task in DefaultConnectionPool they orchestrate this logic.

...ava/io/micrometer/core/instrument/binder/mongodb/MongoMetricsConnectionPoolListenerTest.java

dependencies.gradle

See micrometer-metricsgh-2493

See gh-2493

Migrating to mongo client 4.x and remove (hopefully temporarily) mong…

7094aa9

…odb.driver.pool.waitqueuesize

jonatan-ivanov self-assigned this Mar 10, 2021

jonatan-ivanov linked an issue Mar 10, 2021 that may be closed by this pull request

Upgrade to Mongo 4 java driver #2338

Closed

onobc reviewed Mar 10, 2021

View reviewed changes

shakuzen reviewed Mar 10, 2021

View reviewed changes

...ava/io/micrometer/core/instrument/binder/mongodb/MongoMetricsConnectionPoolListenerTest.java Outdated Show resolved Hide resolved

shakuzen reviewed Mar 10, 2021

View reviewed changes

dependencies.gradle Show resolved Hide resolved

jonatan-ivanov added 3 commits March 12, 2021 13:14

Lists.newArrayList -> Collections.singletonList

e2716fc

Removing waitQueueSize hopefully just temporarily.

f80cd56

It seems we are ok without this

b22e9b9

micrometer-metrics deleted a comment from onobc Mar 13, 2021

MOst probably we need to let waitqueuesize go

c4caf81

onobc mentioned this pull request Mar 13, 2021

Possibly add metrics for newly added Mongo4 connection pool events #2499

Open

micrometer-metrics deleted a comment from onobc Mar 13, 2021

jonatan-ivanov added 3 commits March 14, 2021 20:35

Adding back waitqueuesize using checkedout events

ada3019

author

fe34ee4

Merge branch 'master' into mongo-4.x

bc0e603

jonatan-ivanov marked this pull request as ready for review March 15, 2021 03:42

jonatan-ivanov requested a review from shakuzen March 15, 2021 03:43

shakuzen approved these changes Mar 15, 2021

View reviewed changes

jonatan-ivanov merged commit 30b3a33 into master Mar 15, 2021

jonatan-ivanov deleted the mongo-4.x branch March 15, 2021 18:04

izeye added a commit to izeye/micrometer that referenced this pull request Mar 23, 2021

Polish "Migrating to mongo client 4.x"

b8c71f0

See micrometer-metricsgh-2493

izeye mentioned this pull request Mar 23, 2021

Polish "Migrating to mongo client 4.x" #2523

Merged

shakuzen pushed a commit that referenced this pull request Mar 24, 2021

Polish "Migrating to mongo client 4.x" (#2523)

f7ac7f3

See gh-2493

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrating to mongo client 4.x #2493

Migrating to mongo client 4.x #2493

jonatan-ivanov commented Mar 10, 2021 •

edited by shakuzen

Loading

onobc left a comment

onobc Mar 10, 2021 •

edited

Loading

shakuzen Mar 10, 2021

onobc Mar 10, 2021 •

edited

Loading

shakuzen Mar 10, 2021 •

edited

Loading

jonatan-ivanov Mar 12, 2021 •

edited

Loading

onobc Mar 13, 2021

onobc Mar 13, 2021

jonatan-ivanov Mar 13, 2021

onobc Mar 15, 2021

shakuzen Mar 15, 2021

shakuzen Mar 10, 2021

onobc Mar 10, 2021 •

edited

Loading

jonatan-ivanov Mar 12, 2021

onobc Mar 13, 2021

jonatan-ivanov Mar 13, 2021 •

edited

Loading

Migrating to mongo client 4.x #2493

Migrating to mongo client 4.x #2493

Conversation

jonatan-ivanov commented Mar 10, 2021 • edited by shakuzen Loading

onobc left a comment

Choose a reason for hiding this comment

onobc Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onobc Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

shakuzen Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

jonatan-ivanov Mar 12, 2021 • edited Loading

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

onobc Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatan-ivanov Mar 13, 2021 • edited Loading

Choose a reason for hiding this comment

jonatan-ivanov commented Mar 10, 2021 •

edited by shakuzen

Loading

onobc Mar 10, 2021 •

edited

Loading

onobc Mar 10, 2021 •

edited

Loading

shakuzen Mar 10, 2021 •

edited

Loading

jonatan-ivanov Mar 12, 2021 •

edited

Loading

onobc Mar 10, 2021 •

edited

Loading

jonatan-ivanov Mar 13, 2021 •

edited

Loading