-
Notifications
You must be signed in to change notification settings - Fork 153
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: implement dump metrics #200
Conversation
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.
Please, generate change files with rush change -b origin/master
Done |
Added rpc requests metrics |
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.
Please, avoid code style changes.
to: assertNotNull(lastBlock) | ||
} | ||
const chunk = getNextChunk(blockRange.from, blockRange.to) | ||
chunk.transactDir('.', async fs => { |
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.
missing await
}); | ||
|
||
this.rpcRequestsGauge = new Gauge({ | ||
name: 'sqd_rpc_requests_count', |
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.
General pattern for metric names in the SDK is sqd_{app}_{name}
.
Since there could be multiple RPC connections used from a single process, url
label should be added as well.
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 see currently only one url. But ok, will add this
collect() { | ||
const metrics = rpc.getMetrics(); | ||
this.set({ | ||
kind: 'successful', |
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.
May be success | failure
instead of successful | failed
? Sounds better to me.
No description provided.