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

chore: add more logs for OOM search #164

Merged
merged 9 commits into from
Dec 3, 2024
Merged

chore: add more logs for OOM search #164

merged 9 commits into from
Dec 3, 2024

Conversation

shoom3301
Copy link
Contributor

This code doesn't change the logic, it only adds more logs and refactors the code a bit.

  1. Added more logs
  2. Removed excessive exports
  3. Got rid of excessive ComposableCoW contract instances
  4. Added "Conditional order params are equal but not the same" log

The last point is my suspect. I assume that comparation here is not valid, we should not use shallow comparison for the objects.

@shoom3301 shoom3301 requested a review from a team December 2, 2024 09:22
Copy link

github-actions bot commented Dec 2, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@shoom3301
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Dec 2, 2024
src/domain/events/index.ts Outdated Show resolved Hide resolved
src/domain/events/index.ts Outdated Show resolved Hide resolved
src/domain/events/index.ts Outdated Show resolved Hide resolved
@@ -95,27 +96,32 @@ export interface RegistryBlock {
hash: string;
}

type OrdersPerOwner = Map<Owner, Set<ConditionalOrder>>;

const logger = getLogger("Registry");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why registry if this is in model.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

I have no idea :)
I just moved it from class in order to use in loadOwnerOrders()

Copy link
Contributor

Choose a reason for hiding this comment

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

Its a similar comment as above. You took it out of the registry and has still a very generic name.
I don't think getting loggers should be the memory leak (i hope)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got you.
Created a separate logger:

const loadOwnerOrdersLogger = getLogger("loadOwnerOrders");

@shoom3301 shoom3301 merged commit 2eef69c into main Dec 3, 2024
4 checks passed
@shoom3301 shoom3301 deleted the fix/more-logs branch December 3, 2024 11:01
@github-actions github-actions bot locked and limited conversation to collaborators Dec 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants