-
Notifications
You must be signed in to change notification settings - Fork 68
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: Moved sdkClient reset metrics into the metricService #3120
base: main
Are you sure you want to change the base?
feat: Moved sdkClient reset metrics into the metricService #3120
Conversation
labels with prometheus counters. Signed-off-by: ebadiere <[email protected]>
Quality Gate passedIssues Measures |
🚨 Memory Leak Detected 🚨A potential memory leak has been detected in the test titled Details📊 Memory Leak Detection Report 📊 GC Type: MarkSweepCompact Heap Statistics (before vs after executing the test):
Heap Space Statistics (before vs after executing the test):
RecommendationsPlease investigate the memory allocations in this test, focusing on objects that are not being properly deallocated. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3120 +/- ##
==========================================
- Coverage 83.19% 83.14% -0.05%
==========================================
Files 63 63
Lines 4242 4267 +25
Branches 830 830
==========================================
+ Hits 3529 3548 +19
- Misses 470 476 +6
Partials 243 243
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Few comments
* @readonly | ||
* @private | ||
*/ | ||
private readonly hapiService: HAPIService; |
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.
This class property is not actually used so you can remove it
mirrorNodeClient: MirrorNodeClient, | ||
hbarLimiter: HbarLimit, | ||
register: Registry, | ||
eventEmitter: EventEmitter, | ||
) { | ||
this.logger = logger; | ||
this.sdkClient = sdkClient; | ||
this.hapiService = hapiService; |
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 remove the class property this line should be removed we only need the hapiService instance to get the sdk client in the line below
mirrorNodeClient: MirrorNodeClient, | ||
hbarLimiter: HbarLimit, | ||
register: Registry, | ||
eventEmitter: EventEmitter, | ||
) { | ||
this.logger = logger; | ||
this.sdkClient = sdkClient; | ||
this.hapiService = hapiService; | ||
this.sdkClient = hapiService.getSDKClient(); |
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.
But do we really need this, not sure we need the metric service to depend on the sdkClient. The sdkClient is used here to get details from the transaction record in the captureTransactionMetrics. However, arent they passed when emitting the event in the sdkClient?
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.
This is a really good point! Perhaps we can passed the transactionRecordMetrics
in when emitting the event along with other arguments, this way we can make the metric service to be independent from the sdkClient. But I guess it's gonna be out of the scope for this PR we can create a ticket for it. Great point!
@@ -220,6 +275,20 @@ export default class MetricService { | |||
this.captureMetrics(executionMode, txConstructorName, status, cost, gasUsed, callerName, interactingEntity); | |||
}; | |||
|
|||
public incrementConsensusClientDurationResets = ({ duration }: IExecuteConsenusClientResetDurationPayload): void => { |
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.
let's try to include JSDocs for these methods
this.consensusClientResetsDuration.inc(1); | ||
}; | ||
|
||
public incrementConsensusClientErrorResets = ({ errorCodes }: IExecuteConsenusClientResetErrorPayload): void => { |
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.
let's try to include JSDocs for these methods
this.consensusClientResetsError.inc(1); | ||
}; | ||
|
||
public incrementConsensusClientTransactionResets = ({ |
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.
let's try to include JSDocs for these methods
* @param {Registry} register | ||
* @returns {Counter} Consensus node client reset metric | ||
*/ | ||
private initConsensusClientResetDurationMetric(register: Registry): Counter { |
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.
The methods initConsensusClientResetDurationMetric()
, initConsensusClientResetErrorCodeMetric()
, and initConsensusClientResetTransactionMetric()
are essentially the same. It might be better to group them into a more general method to avoid code repetition. We could place this general method in ./packages/relay/src/utils.ts
, making it reusable in other areas as well.
@@ -123,6 +163,21 @@ export default class MetricService { | |||
this.eventEmitter.on(constants.EVENTS.EXECUTE_QUERY, (args: IExecuteQueryEventPayload) => { | |||
this.addExpenseAndCaptureMetrics(args); | |||
}); | |||
|
|||
this.eventEmitter.on(constants.EVENTS.RESET_CLIENT_DURATION, (args: IExecuteConsenusClientResetDurationPayload) => { | |||
this.incrementConsensusClientDurationResets(args); |
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.
Also let's make sure to add tests to cover these execution
}); | ||
|
||
this.eventEmitter.on(constants.EVENTS.RESET_CLIENT_ERRORS, (args: IExecuteConsenusClientResetErrorPayload) => { | ||
this.incrementConsensusClientErrorResets(args); |
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.
Also let's make sure to add tests to cover these execution
this.eventEmitter.on( | ||
constants.EVENTS.RESET_CLIENT_TRANSACTIONS, | ||
(args: IExecuteConsenusClientResetTransactionPayload) => { | ||
this.incrementConsensusClientTransactionResets(args); |
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.
Also let's make sure to add tests to cover these execution
Description:
Moved sdkClient reset metrics into the metricService and replaced labels with prometheus counters.
Related issue(s):
Fixes #3075
Notes for reviewer:
The old client reset counter was not used and was problematic.
Labels Are Not Meant for Numerical Data: In Prometheus, labels are designed for categorical data used to identify or group metrics (e.g., by host, region, or service). They are not intended for storing numerical data that you want to plot or perform calculations on.
Inability to Perform Calculations: Since label values are strings, you cannot perform arithmetic operations on them in Prometheus queries.
Visualization Challenges: Grafana relies on numerical data points over time to generate graphs. With the current setup, it's difficult to extract and plot the numerical values from labels.
This change replaces the labels with counters.
Checklist