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.

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

feat(NODE-4720): Add log messages to CMAP spec #3645

Merged
merged 37 commits into from May 8, 2023
Merged

feat(NODE-4720): Add log messages to CMAP spec #3645

merged 37 commits into from May 8, 2023

Conversation

W-A-James
Copy link
Contributor

@W-A-James W-A-James commented Apr 14, 2023 •
edited

Description

What is changing?

CMAP Logging updates
  • Added emitAndLog method to TypedEventEmitter
  • Added logging the following events:
    • ConnectionPoolCreatedEvent
    • ConnectionPoolReadyEvent
    • ConnectionPoolClearedEvent
    • ConnectionPoolClosedEvent
    • ConnectionCreatedEvent
    • ConnectionReadyEvent
    • ConnectionClosedEvent
    • ConnectionCheckOutStartedEvent
    • ConnectionCheckOutFailedEvent
    • ConnectionCheckedOutEvent
    • ConnectionCheckedInEvent
Changes to Topology
  • Changed Topology constructor to accept MongoClient as first argument to guarantee that it always has a valid reference to the owning client
Changes to Server
  • Moved pool and topology fields from fields on Server.s to top level fields of Server and updated all references to reflect this
Testing changes
  • CMAP logging
    • Pulled in CMAP logging tests and fix from NODE-5212
  • Topology
    • Converted test/integration/node-specific/topology.test.js to typescript and refactored test to get rid of flakiness introduced by other changes
    • Removed TestConfiguration.newTopology as it was unused
    • Updated unit tests making use of old Topology constructor to now make use of new topologyWithPlaceHolderClient helper function
  • Unified Test Runner
    • Implemented close operation for client entities in Unified Test Runner and refactored this functionality to be less nested
Is there new documentation needed for these changes?

No

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@W-A-James W-A-James marked this pull request as ready for review April 28, 2023 13:53
baileympearson
baileympearson requested changes Apr 28, 2023
View reviewed changes
test/tools/unified-spec-runner/operations.ts Outdated Show resolved Hide resolved
src/sdam/server.ts Outdated Show resolved Hide resolved
test/tools/utils.ts Show resolved Hide resolved
@W-A-James W-A-James requested a review from nbbeeken May 1, 2023 17:33
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label May 1, 2023
@nbbeeken nbbeeken self-assigned this May 1, 2023
nbbeeken
nbbeeken requested changes May 1, 2023
View reviewed changes
test/tools/runner/config.ts Outdated Show resolved Hide resolved
test/unit/sdam/server.test.ts Outdated Show resolved Hide resolved
test/tools/utils.ts Show resolved Hide resolved
test/tools/unified-spec-runner/operations.ts Outdated Show resolved Hide resolved
...integration/connection-monitoring-and-pooling/connection_monitoring_and_pooling.spec.test.ts Outdated Show resolved Hide resolved
src/mongo_client.ts Outdated Show resolved Hide resolved
src/sdam/server.ts Outdated Show resolved Hide resolved
src/cmap/connection_pool.ts Outdated Show resolved Hide resolved
src/cmap/connection_pool.ts Outdated Show resolved Hide resolved
W-A-James
W-A-James commented May 3, 2023
View reviewed changes
src/mongo_client.ts Outdated Show resolved Hide resolved
@W-A-James W-A-James requested a review from nbbeeken May 3, 2023 14:38
nbbeeken
nbbeeken requested changes May 3, 2023
View reviewed changes
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

LGTM, just need small CI fixes

@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels May 3, 2023
nbbeeken
nbbeeken previously approved these changes May 4, 2023
View reviewed changes
@dariakp dariakp self-requested a review May 5, 2023 14:05
dariakp
dariakp requested changes May 5, 2023
View reviewed changes
src/mongo_types.ts Show resolved Hide resolved
src/cmap/connection_pool.ts Outdated Show resolved Hide resolved
src/sdam/server.ts Show resolved Hide resolved
@dariakp dariakp added Blocked Blocked on other work and removed Blocked Blocked on other work labels May 5, 2023
@W-A-James W-A-James requested a review from dariakp May 8, 2023 15:50
@nbbeeken nbbeeken merged commit b27f385 into main May 8, 2023
11 of 21 checks passed
@nbbeeken nbbeeken deleted the NODE-4720/add_log_messages_to_cmap_spec branch May 8, 2023 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baileympearson baileympearson baileympearson requested changes

@nbbeeken nbbeeken nbbeeken approved these changes

@dariakp dariakp dariakp approved these changes

Assignees

@nbbeeken nbbeeken

Labels
Team Review Needs review from team
Projects
None yet
Milestone
No milestone
4 participants
@W-A-James @nbbeeken @baileympearson @dariakp