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: add heap monitoring #369

Merged
merged 21 commits into from
Jun 5, 2024
Merged

feat: add heap monitoring #369

merged 21 commits into from
Jun 5, 2024

Conversation

peternhale
Copy link
Collaborator

@peternhale peternhale commented May 15, 2024

What does this PR do?

add heap monitoring and class instrumentation

What issues does this PR fix or reference?

Instrumentation only

@W-15760648@

@W-15760648@ add heap monitoring and class instrumentation
Copy link
Contributor

@CristiCanizales CristiCanizales left a comment

Choose a reason for hiding this comment

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

lgtm! Just a question :)

src/reporters/tapFormatTransform.ts Outdated Show resolved Hide resolved
@CristiCanizales
Copy link
Contributor

QA doc here

Copy link
Contributor

@CristiCanizales CristiCanizales left a comment

Choose a reason for hiding this comment

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

LGTM!

:shipit:

@W-15724695@
@W-15760648@

Replace json stringify with bfj module
Add tuning properties to streams
Add env vars to adjust buffer size and json format indent
@peternhale
Copy link
Collaborator Author

@CristiCanizales I am trying to resolve the broken tests, but the code should be near complete if you want to have another look.

src/utils/heapMonitor.ts Outdated Show resolved Hide resolved
src/utils/heapMonitor.ts Show resolved Hide resolved
src/reporters/junitFormatTransformer.ts Show resolved Hide resolved
package.json Show resolved Hide resolved
src/reporters/junitFormatTransformer.ts Show resolved Hide resolved
src/reporters/tapFormatTransform.ts Show resolved Hide resolved
src/reporters/tapReporter.ts Show resolved Hide resolved
src/utils/heapMonitor.ts Show resolved Hide resolved
@@ -26,11 +28,19 @@ export class HumanFormatTransform extends Readable {
super(options);
this.testResult = testResult;
this.detailedCoverage ??= false;
this.logger = Logger.childFromRoot('HumanFormatTransform');
Copy link
Member

Choose a reason for hiding this comment

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

what are the concerns to have this logger, given that we have HeapMonitor to log?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No concerns. It is the same root logger that the other places where logging is done in the module.

src/tests/asyncTests.ts Show resolved Hide resolved
src/tests/codeCoverage.ts Show resolved Hide resolved
src/reporters/junitFormatTransformer.ts Show resolved Hide resolved
src/reporters/tapFormatTransform.ts Show resolved Hide resolved
src/reporters/tapFormatTransform.ts Show resolved Hide resolved
src/reporters/tapReporter.ts Show resolved Hide resolved
src/utils/heapMonitor.ts Show resolved Hide resolved
Copy link
Contributor

@CristiCanizales CristiCanizales left a comment

Choose a reason for hiding this comment

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

LGTM! Just a nit in the date in the copyright of heapMonitor file

@@ -0,0 +1,152 @@
/*
* Copyright (c) 2021, salesforce.com, inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2021, salesforce.com, inc.
* Copyright (c) 2024, salesforce.com, inc.

Copy link
Member

@mingxuanzhangsfdx mingxuanzhangsfdx left a comment

Choose a reason for hiding this comment

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

Thanks for the change! LGTM

import { NamespaceInfo } from './types';
import type { QueryResult } from '@jsforce/jsforce-node';

const DEFAULT_BUFFER_SIZE = 256;
const MIN_BUFFER_SIZE = 256;
const MAX_BUFFER_SIZE = 32_768;
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. I didn't know about _ used to improve the readability of large numbers 🙂

Copy link
Contributor

@CristiCanizales CristiCanizales left a comment

Choose a reason for hiding this comment

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

Thanks for adding these validations :) Please just address xNuts tests failing before merging so we can :shipit:

@peternhale peternhale merged commit ee6fe4b into main Jun 5, 2024
16 checks passed
@peternhale peternhale deleted the phale/W-15760648-heap-monitor branch June 5, 2024 20:10
gilgourevitch pushed a commit to gilgourevitch/salesforcedx-apex that referenced this pull request Jun 6, 2024
* feat: add heap monitoring

@W-15760648@ add heap monitoring and class instrumentation

* chore: fix ut

* chore: bump core and jsforce versions

* chore: move log trace calls

* chore: improve instrumentation

* chore: allow only one heap monitor

* chore: refactor interval assignment to c’tor

* chore: bump core

* chore: wip

* fix: use module bfj for json string and perf tuning

@W-15724695@
@W-15760648@

Replace json stringify with bfj module
Add tuning properties to streams
Add env vars to adjust buffer size and json format indent

* chore: apply review comments

* chore: fix windows ut for table

* chore: fix another windows test

* chore: add comments on setImmediate calls

* chore: bump kit

* chore: address review comments

* chore: add limits to new env var values

* chore: bump core to latest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants