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.

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

feat(NODE-4810): define the new Logger #3475

Merged
merged 51 commits into from
Dec 15, 2022
Merged

feat(NODE-4810): define the new Logger #3475

merged 51 commits into from
Dec 15, 2022

Conversation

andymina
Copy link
Contributor

@andymina andymina commented Nov 22, 2022

Description

What is changing?

Defines the new Logger class as described by NODE-3974. This PR handles the options parsing and instantiation of the logger.

Is there new documentation needed for these changes?

What is the motivation for this change?

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>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

src/mongo_logger.ts Outdated Show resolved Hide resolved
src/mongo_logger.ts Outdated Show resolved Hide resolved
src/mongo_logger.ts Outdated Show resolved Hide resolved
test/unit/mongo_client.test.js Outdated Show resolved Hide resolved
test/unit/mongo_client.test.js Outdated Show resolved Hide resolved
src/mongo_logger.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
test/unit/mongo_logger.test.ts Outdated Show resolved Hide resolved
test/unit/mongo_logger.test.ts Outdated Show resolved Hide resolved
src/mongo_logger.ts Outdated Show resolved Hide resolved
test/unit/mongo_client.test.js Outdated Show resolved Hide resolved
test/unit/mongo_client.test.js Show resolved Hide resolved
test/unit/mongo_client.test.js Outdated Show resolved Hide resolved
test/unit/mongo_logger.test.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Nov 28, 2022
test/unit/mongo_client.test.js Outdated Show resolved Hide resolved
test/unit/mongo_client.test.js Outdated Show resolved Hide resolved
test/unit/mongo_client.test.js Outdated Show resolved Hide resolved
src/connection_string.ts Outdated Show resolved Hide resolved
src/mongo_logger.ts Outdated Show resolved Hide resolved
test/unit/mongo_logger.test.ts Outdated Show resolved Hide resolved
test/integration/node-specific/feature_flags.test.ts Outdated Show resolved Hide resolved
src/mongo_logger.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

Almost there

src/mongo_logger.ts Outdated Show resolved Hide resolved
src/mongo_logger.ts Outdated Show resolved Hide resolved
test/integration/node-specific/feature_flags.test.ts Outdated Show resolved Hide resolved
test/integration/node-specific/feature_flags.test.ts Outdated Show resolved Hide resolved
test/unit/mongo_logger.test.ts Show resolved Hide resolved
test/unit/mongo_logger.test.ts Outdated Show resolved Hide resolved
test/unit/mongo_logger.test.ts Outdated Show resolved Hide resolved
test/unit/mongo_logger.test.ts Outdated Show resolved Hide resolved
test/unit/mongo_logger.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

Please address the comments in the feature_flags file; otherwise, just a few clean up comments on the mongo_logger.test.ts around the severity helpers and defaults; the testing is exhaustive, and there are improvements that could be made there to condense them by analyzing which cases are redundant (i.e., we want to test each component and each severity level at least once, but testing every component with every severity is overkill; likewise, when testing defaults, just one test case where nothing is set and assertions for the default on each expected thing is enough). If we want to just get these changes in, I am not going to insist on reworking the test cases right now (because they do cover everything), but then we'll need to revisit this and rewrite them in a more maintainable way when we go to implement client option handling for these settings.

test/unit/mongo_logger.test.ts Show resolved Hide resolved
test/unit/mongo_logger.test.ts Outdated Show resolved Hide resolved
test/unit/mongo_logger.test.ts Show resolved Hide resolved
test/unit/mongo_logger.test.ts Outdated Show resolved Hide resolved
dariakp
dariakp previously approved these changes Dec 13, 2022
src/mongo_logger.ts Outdated Show resolved Hide resolved
src/mongo_logger.ts Show resolved Hide resolved
src/mongo_logger.ts Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/connection_string.ts Outdated Show resolved Hide resolved
@dariakp dariakp merged commit 6ef11d7 into main Dec 15, 2022
@dariakp dariakp deleted the NODE-4810 branch December 15, 2022 23:19
@avaly
Copy link
Contributor

avaly commented Feb 6, 2023

I can't find any documentation about this new Logger in the API docs or the v5.0.0 changes doc. Where can we read more about this?

@baileympearson
Copy link
Contributor

Hey @avaly - the new logger is still in development, is currently unused inside the driver and is not intended to be consumed yet. Once development is finished, we'll include information and links to relevant documentation in the release notes for whichever release it is a part of.

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.

6 participants