Skip to content

Conversation

dariakp
Copy link
Contributor

@dariakp dariakp commented Jun 17, 2021

Description

NODE-3356

@dariakp dariakp requested a review from nbbeeken June 17, 2021 21:18
@nbbeeken nbbeeken requested review from durran and emadum June 17, 2021 21:23
@nbbeeken nbbeeken added the Team Review Needs review from team label Jun 17, 2021
@nbbeeken nbbeeken marked this pull request as ready for review June 17, 2021 21:23
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

I like this refactoring. It was always expected with the command monitoring spec that a logger itself would be registering itself to all our monitoring if the users wanted easy debug, and this is how we originally implemented it in the Ruby driver at least and I believe there was discussion for the same for Node in the past that just got deprioritised. This puts us in the right direction - nice job.

Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

LGTM but one of the Logger tests is failing in CI.

@dariakp
Copy link
Contributor Author

dariakp commented Jun 21, 2021

LGTM but one of the Logger tests is failing in CI.

Oh that's weird, I could have sworn I saw them pass earlier; I'll look into it, thanks for the catch! @emadum

@dariakp dariakp force-pushed the NODE-3356/4.0/debug-logs branch from 57c1131 to 3031fde Compare June 21, 2021 20:15
@dariakp dariakp requested a review from emadum June 21, 2021 20:31
@nbbeeken nbbeeken merged commit 70cace8 into 4.0 Jun 21, 2021
@nbbeeken nbbeeken deleted the NODE-3356/4.0/debug-logs branch June 21, 2021 20:45
ljhaywar pushed a commit that referenced this pull request Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Team Review Needs review from team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants