Refactoring express route project structure #99
Conversation
azerella
force-pushed
the
az-refactor-routes
branch
from
9d4ddd7
to
9171cc5
Compare
azerella
force-pushed
the
az-refactor-routes
branch
from
9171cc5
to
b535d07
Compare
There was a problem hiding this comment.
Beware that @AndreiEres is bringing changes in parts you touched and I think his code should be merged first. There will likely be a few conflicts afterwards.
|
|
||
| export const metrics: MetricsDefinition = { | ||
| data: { | ||
| balance: 0, |
There was a problem hiding this comment.
I don't think this is a good idea to initialize the balance with 0.
It will lead to prometheus potentially reporting this and trigger alerts about the balance being 0 whereas it currently is not empty.
There was a problem hiding this comment.
Ah this was simply copied from what it already is, I'm not too confident in what else this should be? Might need @AndreiEres to weigh in here
There was a problem hiding this comment.
The current code skips retuning the metrics at all until a real value is known (it does not return 0 unless it is really 0).
Not sure what Prometheus' defaults are but null or undefined would be better.
There was a problem hiding this comment.
Beware that @AndreiEres is bringing changes in parts you touched and I think his code should be merged first. There will likely be a few conflicts afterwards.
I'll wait until these changes are in before working on this restructure further. I'm also sure that @AndreiEres would prefer that the Prometheus defaults are modified in a separate PR
There was a problem hiding this comment.
I would disagree with that, becaue if we merge this PR with the balance initializing at 0, we will get false alarms about the balance being too low. We do not have the issue today so the PR in the current state would introduce a regression.
We don't really need another PR to fix regressions introduced in a PR if we are aware of them.
There was a problem hiding this comment.
We do not have the issue today so the PR in the current state would introduce a regression.
I don't understand that considering this is what the existing code already does? Are you saying we haven't deployed this code yet?
|
Closing in favor of #145, as this is outdated |
Fixes #98