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-4814): implement remaining severity logging methods #3629

Merged

Conversation

W-A-James
Copy link
Contributor

@W-A-James W-A-James commented Apr 6, 2023

Description

What is changing?

Adds in the remainder of the severity loggers whose base designs were implemented in NODE-5169

Is there new documentation needed for these changes?

No

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

@W-A-James W-A-James force-pushed the NODE-4814/implement_remaining_severity_logging_methods branch 3 times, most recently from 23694d0 to 08638b4 Compare April 11, 2023 17:05
@W-A-James W-A-James changed the base branch from main to NODE-4813/Implement_severity_logging_methods April 11, 2023 17:06
@W-A-James W-A-James force-pushed the NODE-4814/implement_remaining_severity_logging_methods branch 2 times, most recently from 1d7b3dc to 40034c3 Compare April 14, 2023 19:55
Base automatically changed from NODE-4813/Implement_severity_logging_methods to main April 14, 2023 19:58
@W-A-James W-A-James force-pushed the NODE-4814/implement_remaining_severity_logging_methods branch from ab7b362 to c2324c9 Compare April 14, 2023 20:19
@W-A-James W-A-James marked this pull request as ready for review April 14, 2023 20:19
@W-A-James
Copy link
Contributor Author

Question: Do we want to remove the entries in the SeverityLevel enum that correspond to the severities that we are not using?

These would be:

  • emergency
  • alert
  • critical
  • notice

@W-A-James
Copy link
Contributor Author

Question: Do we want to have the helper that corresponds with the warn severity be named warn or warning? Doing the former allows for easy accessing on the logger object by using the severity name directly as a property, so I skew towards that approach.

@baileympearson
Copy link
Contributor

baileympearson commented Apr 17, 2023

Question: Do we want to have the helper that corresponds with the warn severity be named warn or warning? Doing the former allows for easy accessing on the logger object by using the severity name directly as a property, so I skew towards that approach.

Agreed. I also think that these methods make the most sense in the verb form (i.e., "warn" instead of "warning")

Question: Do we want to remove the entries in the SeverityLevel enum that correspond to the severities that we are not using?
These would be:

  • emergency
  • alert
  • critical
  • notice

I lean towards leaving all severity levels, for consistency with the spec. If we TS support to help enforce the lack of helpers for emergency, alert, critical, notice, we could consider defining severity level something like this? This would constrain severity level to only allow supported severity levels. Not sure if it's worthwhile though.

export type SeverityLevel = (typeof SeverityLevel)[keyof typeof SeverityLevel] & 
	('log' | 'debug' .... );

Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

Tiny change

) {
const severity = SEVERITY_LEVEL_MAP.getSeverityLevelName(l);
for (let i = index; i >= 0; i--) {
const severity = severities[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

if we type severities as SeverityLevel[], we should be able to remove the cast in line 683

@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Apr 17, 2023
@baileympearson baileympearson added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Apr 18, 2023
@baileympearson
Copy link
Contributor

CI failures are unrelated.

src/mongo_logger.ts Outdated Show resolved Hide resolved
@W-A-James W-A-James requested a review from nbbeeken April 18, 2023 18:48
src/mongo_logger.ts Outdated Show resolved Hide resolved
src/mongo_logger.ts Outdated Show resolved Hide resolved
@W-A-James W-A-James requested a review from nbbeeken April 18, 2023 19:43
nbbeeken
nbbeeken previously approved these changes Apr 18, 2023
@baileympearson baileympearson merged commit d7a8079 into main Apr 19, 2023
14 of 21 checks passed
@baileympearson baileympearson deleted the NODE-4814/implement_remaining_severity_logging_methods branch April 19, 2023 18:16
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
3 participants