-
Notifications
You must be signed in to change notification settings - Fork 60
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
Aggregate event logs for unsupported chains #1921
Conversation
Pull Request Test Coverage Report for Build 11035173943Details
💛 - Coveralls |
f91f010
to
3cb2158
Compare
src/domain/hooks/hooks.repository.ts
Outdated
private static readonly HOOK_TYPE = 'hook'; | ||
private static readonly UNSUPPORTED_CHAIN_EVENT = 'unsupported_chain_event'; | ||
public static readonly UNSUPPORTED_EVENTS_LOG_INTERVAL = 60 * 1000; // 1 minute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add this to the configuration if we plan to change it dynamically. I opted to keep it here for now, but I'm open to externalizing the configuration.
src/domain/hooks/hooks.repository.ts
Outdated
MemoizedFunction; | ||
private readonly unsupportedEventsStore: Map<string, number> = new Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we have two instances and this is stored in memory, won't this mean the log is emitted twice each time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this to Redis in aea8544.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for looking into this Aaron! The memory storage was created to alleviate cache usage when receiving a lot of unsupported events in a short period. The log would indeed be emitted N times on each cycle (N being the number of running instances). Still, each instance would print the number of events received by the instance, and monitoring tools can aggregate the numbers if needed.
Sorry, this approach was highly opinionated and probably should have been discussed beforehand, I'm fine with using Redis for storing a unified counter, but I also think we should use an atomic counter (Redis INCR
) instead of a plain hash (as implemented on aea8544) for both consistency (dirty writes) and performance. Wdyt @iamacook @PooyaRaki ?
(Btw, if we think this should be changed, I'd be super happy to tackle it on a separate PR so we can merge this one and unblock the feature 🙂 )
c07d182
to
bc6c46f
Compare
c0d04f8
to
a5628fb
Compare
a5628fb
to
685457f
Compare
Add getCounter and setCounter to ICacheService
The base branch was changed.
@@ -10,7 +10,9 @@ import { EventNotificationsHelper } from '@/domain/hooks/helpers/event-notificat | |||
import { EventCacheHelper } from '@/domain/hooks/helpers/event-cache.helper'; | |||
|
|||
@Injectable() | |||
export class HooksRepositoryWithNotifications implements IHooksRepository { | |||
export class HooksRepositoryWithNotifications | |||
implements IHooksRepository, OnModuleInit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to implement OnModuleInit
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just found out that onModuleInit
lifecycle hook is called by NestJS if a class defines a method called onModuleInit
, regardless of whether the class explicitly implements the OnModuleInit
interface.
So even if the interface is not mandatory for the hook to work, I think it is a good practice to use it for clarity and to ensure your class conforms to the expected structure, so I've explicitly set both HooksRepositoryWithNotifications
and HooksRepository
as OnModuleInit
implementations in 7d695ee
Summary
Derived from this previous work, this PR reduces the number of logs emitted while receiving events for unsupported chains.
Since this event amount could be high in some situations, the errors associated with the reception of these events are grouped by
chainId
and aggregated. A warning log is printed each minute for each affected chain, holding the count of events received associated with the chain.Changes