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

perf(NODE-6616): shortcircuit logging ejson.stringify #4377

Merged
merged 19 commits into from
Jan 24, 2025

Conversation

W-A-James
Copy link
Contributor

Description

What is changing?

Add logic to short-circuit the EJSON.stringify call when logging.
Note that the logic here only accounts for types that are cheap to check the length of, so we notably don't account for the following BSON types

  • Long
  • Decimal128
  • Code (with scope)
  • BSONSymbol (deprecated, so removed handling to reduce number of branches/case statements)
Is there new documentation needed for these changes?

What is the motivation for this change?

Improve performance of Command logging

Release Highlight

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

Copy link

There is an existing patch(es) for this commit SHA:

Please note that the status that is posted is not in the context of this PR but rather the (latest) existing patch and that may affect some tests that may depend on the particular PR. If your tests do not rely on any PR-specific values (like base or head branch name) then your tests will report the same status. If you would like a patch to run in the context of this PR and abort the other(s), comment 'evergreen retry'.

@nbbeeken nbbeeken self-requested a review January 23, 2025 22:01
@nbbeeken nbbeeken self-assigned this Jan 23, 2025
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jan 23, 2025
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

should be some quick fixes, looks good

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
src/mongo_logger.ts Show resolved Hide resolved
src/mongo_logger.ts Outdated Show resolved Hide resolved
src/mongo_logger.ts Outdated Show resolved Hide resolved
src/mongo_logger.ts Show resolved Hide resolved
test/unit/mongo_logger.test.ts Outdated Show resolved Hide resolved
@W-A-James W-A-James requested a review from nbbeeken January 23, 2025 23:15
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Req Changes for viz

I think we just need to correct the Uint8Array case

@W-A-James W-A-James requested a review from nbbeeken January 24, 2025 19:20
@nbbeeken nbbeeken removed the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jan 24, 2025
@nbbeeken nbbeeken added the Team Review Needs review from team label Jan 24, 2025
@nbbeeken nbbeeken merged commit c1bcf0d into main Jan 24, 2025
27 of 30 checks passed
@nbbeeken nbbeeken deleted the NODE-6616-ejson-max-len branch January 24, 2025 20:17
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.

3 participants