Skip to content

GODRIVER-2085 Add serverConnectionId to command monitoring events #724

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

Merged
merged 7 commits into from
Sep 16, 2021
Merged

GODRIVER-2085 Add serverConnectionId to command monitoring events #724

merged 7 commits into from
Sep 16, 2021

Conversation

benjirewis
Copy link
Contributor

GODRIVER-2085

Adds a ServerConnectionID field to CommandStartedEvent, CommandSucceededEvent and CommandFailedEvent to track the serverConnectionId returned in the server's hello or legacy hello response (on 4.2+). Syncs command monitoring spec tests. Bumps supported schema version of the unified test runner to 1.6.

@benjirewis benjirewis requested a review from kevinAlbs August 30, 2021 18:36
@benjirewis benjirewis marked this pull request as ready for review August 30, 2021 18:36
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Overall looking good! I have not fully reviewed yet, but I think the connection ID may need to be set from the connection's server description instead of the selected server.

@benjirewis benjirewis requested a review from kevinAlbs August 31, 2021 15:55
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Apologies for the delayed review. I have a question about storing connectionID on description.Server, but otherwise looks great!

Copy link
Contributor Author

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

I've removed ServerConnectionID from description.Server and added it to HandshakeInformation.

I also required a new field serverConnectionID on topology.Connection and a new method on the Connection interface: ServerConnectionID. The new ServerConnectionID method means that any user implementing driver.Connection will see an error (this interface is under x, though).

Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for addressing the concerns about exposing connectionId as part of server.Description.

Copy link
Collaborator

@matthewdale matthewdale left a comment

Choose a reason for hiding this comment

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

👍 pending resolving the comment about rebasing on master.

@benjirewis
Copy link
Contributor Author

Failing tests are just due to GODRIVER-2155.

@benjirewis benjirewis merged commit 2e76982 into mongodb:master Sep 16, 2021
@benjirewis benjirewis deleted the serverConnectionID.2085 branch September 16, 2021 17:29
faem pushed a commit to kubedb/mongo-go-driver that referenced this pull request Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants