fix(NODE-5818): Add feature flagging to server selection logging

[aditi-khare-mongoDB]

Description

Add feature flagging to server selection logging, so that event instances are only created when willLog returns true for the logging configuration.

EVG Perf Analysis Results

Upon comparing the perf results of NODE-5818's changes to the commit before server selection logging was merged (b93d405, there are no significant regressions through this commit.
Both commits were rerun at the same time to ensure no ci changes influence the results.

Here is the Spreadsheet Link.

What is changing?

Same as above.

Is there new documentation needed for these changes?

No.

What is the motivation for this change?

Before: (Due to server selection logging merge NODE-4687)
findOne - 6% throughput reduction
runCommand - 9% throughput reduction
smallDocInsertOne - 6.5% throughput reduction

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

[durran]

Am I correct in analysing this that, for example, findOne on these changes is slower than the mainline?

[aditi-khare-mongoDB]

Bookkeeping:

With newest rebase to main (includes the logging runtime error handling changes), this is a perf improvement / stabalization

[aditi-khare-mongoDB]

Perf Analysis Results

Evergreen Trends
Upon comparing the perf results of NODE-5818's changes to the commit before server selection logging was merged (b93d405, there are no significant regressions through this commit.
Both commits were rerun at the same time to ensure no ci changes influence the results.

Here is the Spreadsheet Link.

[alenakhineika]

The PR is approved by the team, but blocked by DEVPROD-4523 and can be merged only when the waterfall is fixed.

Also flaky tests: NODE-5897, NODE-5498, NODE-5803

[durran]

I think we just need to rebase now, remove the willLog checks, and then make all calls to the mongologger use topology.client.mongoLogger?.debug() now.

[aditi-khare-mongoDB]

@durran what is your reasoning for removing the willLog / feature flagging checks?

[durran]

@durran what is your reasoning for removing the willLog / feature flagging checks?

@aditi-khare-mongoDB The logger instantiation PR refactored the log method in the client logger to check willLog first and return if it won't log. debug is bound to log so it's going to check that first as well - I don't see the need for the redundant check to willLog. That's why I thought this PR would just go away.

[aditi-khare-mongoDB]

@durran
I see. The log method always checked willLog or some similar method (compareSeverities) before logging the message. See this PR (a random logging PR from before the logger instantiation PR), for reference.

The focus of this PR is so that a ServerSelectionEvent instance is only created if willLog returns true, and the log call only occurs after the ServerSelectionEvent instance is created. This is why, I am of the opinion that we should add a willLog check before we call MongoLogger?.debug or MongoLogger?.info.

[durran]

@durran I see. The log method always checked willLog or some similar method (compareSeverities) before logging the message. See this PR (a random logging PR from before the logger instantiation PR), for reference.

The focus of this PR is so that a ServerSelectionEvent instance is only created if willLog returns true, and the log call only occurs after the ServerSelectionEvent instance is created. This is why, I am of the opinion that we should add a willLog check before we call MongoLogger?.debug or MongoLogger?.info.

Ah ok, thanks for clarifying that. Then I think we can proceed here.