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-4847): Add config error handling to logging #3970

Merged
merged 7 commits into from
Jan 25, 2024

Conversation

aditi-khare-mongoDB
Copy link
Contributor

@aditi-khare-mongoDB aditi-khare-mongoDB commented Jan 19, 2024

Description

Essentially, these changes ensure that the logger does not crash the user's application at runtime, at any point after options are parsed.

During options parsing, if the argument passed in is of incorrect type we want to throw for client options, but not for an incorrect environment option (set to default in this case).

What is changing?

To ensure we don't crash the user's application through the logger, unless there is an invalid type client option passed in to the MongoClient / ConnectionString.

Invalid argument types are checked through the transform function for an element of the OPTIONS array. The OPTIONS array is used in parseOptions (which is called by MongoClient and ConnectionString), so it resolves the argument types for both classes.

See all unit tests defined under config error handling in Kickoff Document,

Is there new documentation needed for these changes?

After NODE-5672 (Release Logger)

What is the motivation for this change?

This ticket is a subtask of NODE-4816 (Add Error Handling to the Logger).

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

@aditi-khare-mongoDB aditi-khare-mongoDB changed the title config handling ready for review NODE(4847): Add config error handling to logging Jan 19, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title NODE(4847): Add config error handling to logging feat(NODE-4847): Add config error handling to logging Jan 19, 2024
@alenakhineika alenakhineika self-requested a review January 22, 2024 16:15
@alenakhineika alenakhineika added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jan 22, 2024
@alenakhineika
Copy link
Contributor

(housekeeping): double check the checkboxes in the pr description

@alenakhineika
Copy link
Contributor

Could we be more specific in the PR title and description, and maybe AC to the ticket about what we expect from this PR? The current description is mostly about the parent ticket and also "Most of the mongodbLogPath stream error handling must be done at runtime, so is in a different subtask.". It is unclear what we are aiming to handle as part of this PR. Can we keep the link to the parent ticket in the What is the motivation for this change? section and put into Description and What is changing? more relevant to the current PR details? From the code, I see only changes made to ConnectionString options to validate those for incorrect values. Based on the kick-off doc, the same edge cases should be already handled by MongoClient.parseOptions() for MongoClientOptions. But I don't see the logic described in "Logging Error Handling Mini Design".

@aditi-khare-mongoDB
Copy link
Contributor Author

Updated the PR description and Logging Mini Design!

@alenakhineika alenakhineika added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jan 23, 2024
alenakhineika
alenakhineika previously approved these changes Jan 23, 2024
test/unit/mongo_client.test.js Outdated Show resolved Hide resolved
@alenakhineika
Copy link
Contributor

alenakhineika commented Jan 25, 2024

The failing test is reported in NODE-5852 and the flaky test in NODE-5789.

@alenakhineika alenakhineika merged commit 8f7bb59 into main Jan 25, 2024
23 of 27 checks passed
@alenakhineika alenakhineika deleted the NODE-4847/config-error-handling-logging branch January 25, 2024 14:29
@alenakhineika alenakhineika removed the Team Review Needs review from team label Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants