chore: metrics todo and naming #95
Conversation
mfw78
added
enhancement
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
mfw78
force-pushed
the
fix-drop-incompatible-orders
branch
from
4c2a197
to
4adace1
Compare
mfw78
force-pushed
the
fix-drop-incompatible-orders
branch
3 times, most recently
from
6c726bc
to
034e35b
Compare
mfw78
force-pushed
the
chore-metrics
branch
2 times, most recently
from
7e80376
to
e7b3d4e
Compare
src/domain/addContract.ts
Outdated
| newContractsTotal, | ||
| singleOrdersTotal, | ||
| totalActiveOrders, | ||
| totalActiveOwners, |
There was a problem hiding this comment.
unsure about this, but should all of these have Metric.
It makes it more verbose, but in the other hand, this instrumentation is mixing a bit with the bussniess logic, hard to differenciate just by name.
There was a problem hiding this comment.
Personally, I'd prefer referencing the module as we've already called it metrics, so using metric.newContractsTotal etc. Let's look at refactoring this later 😃
src/utils/metrics.ts
Outdated
| errorHandler?: (err: any) => T, | ||
| errorMetric?: client.Counter | ||
| ): T { | ||
| const timer = durationMetric.labels(...labels).startTimer(); |
There was a problem hiding this comment.
should we raise if the labels don't match the metric labels? (the size)
There was a problem hiding this comment.
From looking at the API on prom-client, I don't think the labels may only be internal within the type, and not able to be checked against?
Description
This PR addresses issues around naming conventions for the prometheus metrics. Best efforts have been followed to adhere to best practices.
Changes
How to test
http://127.0.0.1:8080/metricsthere respective metrics changing.Related Issues
Related #78, #70