feat(NODE-4687): Add logging to server selection #3946
Conversation
nbbeeken
changed the title
|
Reference to the failing tests: there are two spec tests with the same description 'server-selection-logging', which cause a MochaError. Anyone know a work around? |
@dariakp I am wondering if this should be changed in the spec itself? |
We shouldn't be duplicating all the unified tests here. I'd update the name of one of the duplicates here in this PR, open a DRIVERS ticket to change the name, and point to this PR as the reference implementation for the change. |
|
I found a related ticket in the logging epic filed recently about potentially removing Load Balancer mode in server selection logging, this would also mean removing one of the duplicate test descriptions. I messaged in the drivers-1204-logging channel for more information: |
alenakhineika
added
the
Primary Review
alenakhineika
added
Team Review
aditi-khare-mongoDB
force-pushed
the
NODE-4687/log-server-selection-with-operation
branch
2 times, most recently
from
a5da0d6
to
82a5e60
Compare
aditi-khare-mongoDB
force-pushed
the
NODE-4687/log-server-selection-with-operation
branch
from
82a5e60
to
fc5bca0
Compare
src/mongo_logger.ts
Outdated
| @@ -369,11 +383,17 @@ export function stringifyWithMaxLen( | |||
| maxDocumentLength: number, | |||
| options: EJSONOptions = {} | |||
| ): string { | |||
| const ejson = EJSON.stringify(value, options); | |||
| maxDocumentLength = 20; | |||
There was a problem hiding this comment.
Why are we overwriting maxDocumentLength here? This would make us non-compliant with the spec that states that our default maxDocumentLength should be 1000 (characters in our case) and also override the value passed in to defaultLogTransform
There was a problem hiding this comment.
This was part of debugging and I forgot to remove it before pushing. It's removed now.
| log: Record<string, any>, | ||
| event: ConnectionPoolMonitoringEvent | ServerOpeningEvent | ServerClosedEvent | ||
| ) { | ||
| function attachConnectionFields(log: Record<string, any>, event: any) { |
There was a problem hiding this comment.
Why did we broaden the type of the event parameter?
There was a problem hiding this comment.
We use this function for all SDAM Events, too. We used to use it for server selection events as well before serverHost and serverPort were defined directly on the loggable server selection event classes.
src/operations/execute_operation.ts
Outdated
| @@ -251,7 +254,10 @@ async function retryOperation< | |||
| } | |||
|
|
|||
| // select a new server, and attempt to retry the operation | |||
| const server = await topology.selectServerAsync(selector, { session }); | |||
| const server = await topology.selectServerAsync(selector, { | |||
| session: session, | |||
There was a problem hiding this comment.
| session: session, | |
| session, |
There was a problem hiding this comment.
Didn't catch that, should be changed now.
aditi-khare-mongoDB
force-pushed
the
NODE-4687/log-server-selection-with-operation
branch
from
4617695
to
5322cf6
Compare
|
Ah, I see, I'll be more mindful of running unit tests before I push in the future. |
Description
Add logging to server selection.
What is changing?
When logging is enabled at the debug level, users will be able to see "server selection started, succeeded and failed events". When logging is enabled at the info level, users will be able to see "waiting for suitable server events".
Is there new documentation needed for these changes?
Not until logging is released (NODE-5672)
What is the motivation for this change?
To help users see their server selection events, depending on their logging level enabled.
Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript