Skip to content

Add durations to connection pool events #1166

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 8 commits into from
Aug 25, 2023
Merged

Add durations to connection pool events #1166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bbee638
Add durations to connection pool events
stIncMale Aug 1, 2023
9989a43
Specify the release version
stIncMale Aug 21, 2023
f2c77c1
Address review concerns
stIncMale Aug 24, 2023
83a703b
Apply suggestions from code review
stIncMale Aug 24, 2023
6f93086
Address review concerns
stIncMale Aug 24, 2023
d5437f2
Address review concerns
stIncMale Aug 25, 2023
8ca8ab2
Fix grammar
stIncMale Aug 25, 2023
981c91d
Fix grammar
stIncMale Aug 25, 2023
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
42 changes: 38 additions & 4 deletions driver-core/src/main/com/mongodb/ConnectionString.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@
import com.mongodb.connection.ClusterSettings;
import com.mongodb.connection.ConnectionPoolSettings;
import com.mongodb.connection.SocketSettings;
import com.mongodb.event.ConnectionCheckOutStartedEvent;
import com.mongodb.event.ConnectionCheckedInEvent;
import com.mongodb.event.ConnectionCheckedOutEvent;
import com.mongodb.event.ConnectionCreatedEvent;
import com.mongodb.event.ConnectionReadyEvent;
import com.mongodb.internal.diagnostics.logging.Logger;
import com.mongodb.internal.diagnostics.logging.Loggers;
import com.mongodb.internal.dns.DefaultDnsResolver;
Expand Down Expand Up @@ -132,8 +137,10 @@
* <ul>
* <li>{@code maxPoolSize=n}: The maximum number of connections in the connection pool.</li>
* <li>{@code minPoolSize=n}: The minimum number of connections in the connection pool.</li>
* <li>{@code waitQueueTimeoutMS=ms}: The maximum wait time in milliseconds that a thread may wait for a connection to
* become available.</li>
* <li>{@code waitQueueTimeoutMS=ms}: The maximum duration to wait until either:
* an {@linkplain ConnectionCheckedOutEvent in-use connection} becomes {@linkplain ConnectionCheckedInEvent available},
* or a {@linkplain ConnectionCreatedEvent connection is created} and begins to be {@linkplain ConnectionReadyEvent established}.
* See {@link #getMaxWaitTime()} for more details.</li>
* <li>{@code maxConnecting=n}: The maximum number of connections a pool may be establishing concurrently.</li>
* </ul>
* <p>Write concern configuration:</p>
Expand Down Expand Up @@ -1366,15 +1373,42 @@ public Integer getMinConnectionPoolSize() {
/**
* Gets the maximum connection pool size specified in the connection string.
* @return the maximum connection pool size
* @see ConnectionPoolSettings#getMaxSize()
*/
@Nullable
public Integer getMaxConnectionPoolSize() {
return maxConnectionPoolSize;
}

/**
* Gets the maximum wait time of a thread waiting for a connection specified in the connection string.
* @return the maximum wait time of a thread waiting for a connection
* The maximum duration to wait until either:
* <ul>
* <li>
* an {@linkplain ConnectionCheckedOutEvent in-use connection} becomes {@linkplain ConnectionCheckedInEvent available}; or
* </li>
* <li>
* a {@linkplain ConnectionCreatedEvent connection is created} and begins to be {@linkplain ConnectionReadyEvent established}.
* The time between {@linkplain ConnectionCheckOutStartedEvent requesting} a connection
* and it being created is limited by this maximum duration.
* The maximum time between it being created and {@linkplain ConnectionCheckedOutEvent successfully checked out},
* which includes the time to {@linkplain ConnectionReadyEvent establish} the created connection,
* is affected by {@link SocketSettings#getConnectTimeout(TimeUnit)}, {@link SocketSettings#getReadTimeout(TimeUnit)}
* among others, and is not affected by this maximum duration.
* </li>
* </ul>
* The reasons it is not always possible to create and start establishing a connection
* whenever there is no available connection:
* <ul>
* <li>
* the number of connections per pool is limited by {@link #getMaxConnectionPoolSize()};
* </li>
* <li>
* the number of connections a pool may be establishing concurrently is limited by {@link #getMaxConnecting()}.
* </li>
* </ul>
*
* @return The value of the {@code waitQueueTimeoutMS} option, if specified.
* @see ConnectionPoolSettings#getMaxWaitTime(TimeUnit)
*/
@Nullable
public Integer getMaxWaitTime() {
Expand Down
66 changes: 64 additions & 2 deletions driver-core/src/main/com/mongodb/connection/ConnectionPoolSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
import com.mongodb.ConnectionString;
import com.mongodb.annotations.Immutable;
import com.mongodb.annotations.NotThreadSafe;
import com.mongodb.event.ConnectionCheckOutStartedEvent;
import com.mongodb.event.ConnectionCheckedInEvent;
import com.mongodb.event.ConnectionCheckedOutEvent;
import com.mongodb.event.ConnectionCreatedEvent;
import com.mongodb.event.ConnectionPoolListener;
import com.mongodb.event.ConnectionReadyEvent;
Expand Down Expand Up @@ -119,6 +122,8 @@ public Builder applySettings(final ConnectionPoolSettings connectionPoolSettings
*
* @param maxSize the maximum number of connections in the pool; if 0, then there is no limit.
* @return this
* @see #getMaxSize()
* @see #getMaxWaitTime(TimeUnit)
*/
public Builder maxSize(final int maxSize) {
this.maxSize = maxSize;
Expand All @@ -140,13 +145,38 @@ public Builder minSize(final int minSize) {
}

/**
* <p>The maximum time that a thread may wait for a connection to become available.</p>
* The maximum duration to wait until either:
* <ul>
* <li>
* an {@linkplain ConnectionCheckedOutEvent in-use connection} becomes {@linkplain ConnectionCheckedInEvent available}; or
* </li>
* <li>
* a {@linkplain ConnectionCreatedEvent connection is created} and begins to be {@linkplain ConnectionReadyEvent established}.
* The time between {@linkplain ConnectionCheckOutStartedEvent requesting} a connection
* and it being created is limited by this maximum duration.
* The maximum time between it being created and {@linkplain ConnectionCheckedOutEvent successfully checked out},
* which includes the time to {@linkplain ConnectionReadyEvent establish} the created connection,
* is affected by {@link SocketSettings#getConnectTimeout(TimeUnit)}, {@link SocketSettings#getReadTimeout(TimeUnit)}
* among others, and is not affected by this maximum duration.
* </li>
* </ul>
* The reasons it is not always possible to create and start establishing a connection
* whenever there is no available connection:
* <ul>
* <li>
* the number of connections per pool is limited by {@link #getMaxSize()};
* </li>
* <li>
* the number of connections a pool may be establishing concurrently is limited by {@link #getMaxConnecting()}.
* </li>
* </ul>
*
* <p>Default is 2 minutes. A value of 0 means that it will not wait. A negative value means it will wait indefinitely.</p>
*
* @param maxWaitTime the maximum amount of time to wait
* @param timeUnit the TimeUnit for this wait period
* @return this
* @see #getMaxWaitTime(TimeUnit)
*/
public Builder maxWaitTime(final long maxWaitTime, final TimeUnit timeUnit) {
this.maxWaitTimeMS = MILLISECONDS.convert(maxWaitTime, timeUnit);
Expand Down Expand Up @@ -234,6 +264,7 @@ public Builder connectionPoolListenerList(final List<ConnectionPoolListener> con
* @param maxConnecting The maximum number of connections a pool may be establishing concurrently. Must be positive.
* @return {@code this}.
* @see ConnectionPoolSettings#getMaxConnecting()
* @see #getMaxWaitTime(TimeUnit)
* @since 4.4
*/
public Builder maxConnecting(final int maxConnecting) {
Expand Down Expand Up @@ -298,6 +329,9 @@ public Builder applyConnectionString(final ConnectionString connectionString) {
* <p>Default is 100.</p>
*
* @return the maximum number of connections in the pool; if 0, then there is no limit.
* @see Builder#maxSize(int)
* @see ConnectionString#getMaxConnectionPoolSize()
* @see #getMaxWaitTime(TimeUnit)
*/
public int getMaxSize() {
return maxSize;
Expand All @@ -316,12 +350,38 @@ public int getMinSize() {
}

/**
* <p>The maximum time that a thread may wait for a connection to become available.</p>
* The maximum duration to wait until either:
* <ul>
* <li>
* an {@linkplain ConnectionCheckedOutEvent in-use connection} becomes {@linkplain ConnectionCheckedInEvent available}; or
* </li>
* <li>
* a {@linkplain ConnectionCreatedEvent connection is created} and begins to be {@linkplain ConnectionReadyEvent established}.
* The time between {@linkplain ConnectionCheckOutStartedEvent requesting} a connection
* and it being created is limited by this maximum duration.
* The maximum time between it being created and {@linkplain ConnectionCheckedOutEvent successfully checked out},
* which includes the time to {@linkplain ConnectionReadyEvent establish} the created connection,
* is affected by {@link SocketSettings#getConnectTimeout(TimeUnit)}, {@link SocketSettings#getReadTimeout(TimeUnit)}
* among others, and is not affected by this maximum duration.
* </li>
* </ul>
* The reasons it is not always possible to create and start establishing a connection
* whenever there is no available connection:
* <ul>
* <li>
* the number of connections per pool is limited by {@link #getMaxSize()};
* </li>
* <li>
* the number of connections a pool may be establishing concurrently is limited by {@link #getMaxConnecting()}.
* </li>
* </ul>
*
* <p>Default is 2 minutes. A value of 0 means that it will not wait. A negative value means it will wait indefinitely.</p>
*
* @param timeUnit the TimeUnit for this wait period
* @return the maximum amount of time to wait in the given TimeUnits
* @see Builder#maxWaitTime(long, TimeUnit)
* @see ConnectionString#getMaxWaitTime()
*/
public long getMaxWaitTime(final TimeUnit timeUnit) {
return timeUnit.convert(maxWaitTimeMS, MILLISECONDS);
Expand Down Expand Up @@ -388,6 +448,8 @@ public List<ConnectionPoolListener> getConnectionPoolListeners() {
*
* @return The maximum number of connections a pool may be establishing concurrently.
* @see Builder#maxConnecting(int)
* @see ConnectionString#getMaxConnecting()
* @see #getMaxWaitTime(TimeUnit)
* @since 4.4
*/
public int getMaxConnecting() {
Expand Down
53 changes: 49 additions & 4 deletions driver-core/src/main/com/mongodb/event/ConnectionCheckOutFailedEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,12 @@

package com.mongodb.event;

import com.mongodb.connection.ConnectionPoolSettings;
import com.mongodb.connection.ServerId;

import java.util.concurrent.TimeUnit;

import static com.mongodb.assertions.Assertions.isTrueArgument;
import static com.mongodb.assertions.Assertions.notNull;

/**
Expand Down Expand Up @@ -54,8 +58,25 @@ public enum Reason {

private final ServerId serverId;
private final long operationId;

private final Reason reason;
private final long elapsedTimeNanos;

/**
* Constructs an instance.
*
* @param serverId The server ID. See {@link #getServerId()}.
* @param operationId The operation ID. See {@link #getOperationId()}.
* @param reason The reason the connection check out failed. See {@link #getReason()}.
* @param elapsedTimeNanos The time it took while trying to check out the connection. See {@link #getElapsedTime(TimeUnit)}.
* @since 4.11
*/
public ConnectionCheckOutFailedEvent(final ServerId serverId, final long operationId, final Reason reason, final long elapsedTimeNanos) {
this.serverId = notNull("serverId", serverId);
this.operationId = operationId;
this.reason = notNull("reason", reason);
isTrueArgument("waited time is not negative", elapsedTimeNanos >= 0);
this.elapsedTimeNanos = elapsedTimeNanos;
}

/**
* Construct an instance
Expand All @@ -64,11 +85,12 @@ public enum Reason {
* @param operationId the operation id
* @param reason the reason the connection check out failed
* @since 4.10
* @deprecated Prefer {@link ConnectionCheckOutFailedEvent#ConnectionCheckOutFailedEvent(ServerId, long, Reason, long)}.
* If this constructor is used, then {@link #getElapsedTime(TimeUnit)} is 0.
*/
@Deprecated
public ConnectionCheckOutFailedEvent(final ServerId serverId, final long operationId, final Reason reason) {
this.serverId = notNull("serverId", serverId);
this.operationId = operationId;
this.reason = notNull("reason", reason);
this(serverId, operationId, reason, 0);
}

/**
Expand All @@ -77,6 +99,7 @@ public ConnectionCheckOutFailedEvent(final ServerId serverId, final long operati
* @param serverId the server id
* @param reason the reason the connection check out failed
* @deprecated Prefer {@link #ConnectionCheckOutFailedEvent(ServerId, long, Reason)}
* If this constructor is used, then {@link #getOperationId()} is -1.
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: If expanding out the defaults in the java docs then also include:

and {@link #getElapsedTime(TimeUnit)} is 0.

(This applies to all deprecated constructors with a default operation id)

Copy link
Member Author

Choose a reason for hiding this comment

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

I deliberately did not specify anything about getElapsedTime when documenting this constructor. The idea is that when we deprecate a constructor and introduce a new one, we should only update the documentation for the deprecated constructor and write the documentation for the new one. The reason behind this approach is that we are not going to remember and update all previous constructors each time we introduce a new one, which will lead to weird inconsistent documentation.

When we say Prefer {@link ...(ServerId, long, Reason, long)}. If this constructor is used, then {@link #getElapsedTime(TimeUnit)} is 0., a reader sees that the only difference between the preferred constructor and the one he looks at is a single parameter elapsedTimeNanos, and the documentation explains what value the current constructor uses given that it does not accept elapsedTimeNanos from a user.

Similarly, when we say Prefer {@link ...(ServerId, long, Reason)}. If this constructor is used, then {@link #getOperationId()} is -1., a reader sees that the only difference between the preferred constructor and the one he looks at is a single parameter operationId, and the documentation explains what value the current constructor uses given that it does not accept operationId from a user.

The same goes about the preferred constructors. When we introduce a new constructor, the preferred one specified in all previously deprecated constructors is not replaced with the new constructor. It stays unchanged, thus forming a chain of preference, that leads a reader from any constructor to a more and more preferred one. The chain ends with the non-deprecated constructor.

Given this description of the approach and the reason behind it (both are in bold above), do you still want the docs for all previous constructors to be updated when a new one is introduced?

Copy link
Member

@rozza rozza Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I can live with that 👍

*/
@Deprecated
public ConnectionCheckOutFailedEvent(final ServerId serverId, final Reason reason) {
Expand Down Expand Up @@ -112,13 +135,35 @@ public Reason getReason() {
return reason;
}

/**
* The time it took to check out the connection.
* More specifically, the time elapsed between the {@link ConnectionCheckOutStartedEvent} emitted by the same checking out and this event.
* <p>
* Naturally, if a new connection was not {@linkplain ConnectionCreatedEvent created}
* and {@linkplain ConnectionReadyEvent established} as part of checking out,
* this duration is usually not greater than {@link ConnectionPoolSettings#getMaxWaitTime(TimeUnit)},
* but may occasionally be greater than that, because the driver does not provide hard real-time guarantees.</p>
* <p>
* This duration does not currently include the time to deliver the {@link ConnectionCheckOutStartedEvent}.
* Subject to change.</p>
Comment on lines +147 to +148
Copy link
Collaborator

Choose a reason for hiding this comment

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

getElapsedTime. Just curious, in the spec, what you said about user code (which I think referred to this) seemed right. What is the reason this might change?

Copy link
Member Author

Choose a reason for hiding this comment

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

🤷‍♂️ Maybe users will request for that, maybe something else I can't anticipate comes up. There is an opportunity to allow for a future change, and I prefer to take it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When we say "Subject to change", it seems to negate the documented behaviour immediately preceding, since users cannot rely on it. This reminded me of @Beta. I also thought to search for the string "Subject to change" to see if we take this approach elsewhere, and the one result was:

APIs marked with the `@Beta` annotation at the class or method level are subject to change.

For any API we do not consider Beta, I think we should either avoid documenting anything that is subject to change (in cases where the external behaviour is trivial or irrelevant to users), or commit to a behaviour. My opinion is that committing is preferable, assuming it is reasonable for a user to ask "but does this include user code"?

Copy link
Member Author

Choose a reason for hiding this comment

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

"Subject to change", it seems to negate the documented behaviour immediately preceding, since users cannot rely on it.

We could say "we don't provide any guarantees on whether this duration includes the time to deliver ...". Instead, we give users more information: we document the current behavior (which enables a user to understand why his Thread.sleep(1_000) in a listener does not affect the duration reported in some events), but don't guarantee it will stay the same. Also, because the current behavior is documented, users know that if it is changed, the change will be visible in the documentation.

For any API we do not consider Beta, I think we should either avoid documenting anything that is subject to change

@Beta is about API changes, which is not what we are talking about here.

My opinion is that committing is preferable, assuming it is reasonable for a user to ask "but does this include user code"

  1. This PR implements the spec requirement "The driver MUST document this behavior as well as explicitly warn users that the behavior may change in the future". Are you require the spec to be re-considered? If yes, is this concern a PR blocker? Note that if a DRIVERS ticket is created, and the spec is changed as a result, incorporating such a change in the Java driver will not break compatibility.
  2. assuming it is reasonable for a user to ask "but does this include user code" - I don't understand this part. Could you explain it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Described behaviour is rightly taken by users to be part of the public API. Some public behaviour might be tentative ("subject to change"), but my view is that all such instances in the driver should be marked with @Beta, and that this is done with the expectation that we think the present design is right, and will "soon" be removed from Beta. I do think it is a mistake for any non-beta spec to require drivers to mark public behaviour as tentative.

The original wording was describing public behaviour, however, I believe you are proposing that we might instead state this behaviour but clarify that it is non-public. I don't think we (or others) should describe implementation details, especially without a compelling reason. Otherwise, we might include many internal hints and tips in our docs.

So:

  1. If public behaviour is "subject to change", it should be marked Beta, which is not applicable here.
  2. Non-public behaviour is inherently "subject to change", but I think we should not mention internal behaviour.

My opinion is that you made the right call in excluding user code, and that we should publicly commit to that behaviour. I do not mind merging and changing this later via tickets, if that is more convenient.

*
* @param timeUnit The time unit of the result.
* {@link TimeUnit#convert(long, TimeUnit)} specifies how the conversion from nanoseconds to {@code timeUnit} is done.
* @return The time it took to establish the connection.
* @since 4.11
*/
public long getElapsedTime(final TimeUnit timeUnit) {
return timeUnit.convert(elapsedTimeNanos, TimeUnit.NANOSECONDS);
}

@Override
public String toString() {
return "ConnectionCheckOutFailedEvent{"
+ "server=" + serverId.getAddress()
+ ", clusterId=" + serverId.getClusterId()
+ ", operationId=" + operationId
+ ", reason=" + reason
+ ", elapsedTimeNanos=" + elapsedTimeNanos
+ '}';
}
}
2 changes: 2 additions & 0 deletions driver-core/src/main/com/mongodb/event/ConnectionCheckedInEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

/**
* An event for checking in a connection to the pool.
* Such a connection is considered available until it becomes {@linkplain ConnectionCheckedOutEvent in use}
* or {@linkplain ConnectionClosedEvent closed}.
*
* @since 3.5
*/
Expand Down
Loading