feat: added support for adding labels to default metrics #374
Conversation
test/defaultMetricsTest.js
Outdated
| @@ -71,6 +71,21 @@ describe('collectDefaultMetrics', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| it('should apply labels to metrics when configured', () => { | |||
| const labels = { | |||
There was a problem hiding this comment.
can you add a expect.assertions(5) (or whatever the number is)?
There was a problem hiding this comment.
Can you please elaborate on what this type of assertion is supposed to do? I'm not actually familiar with it, and it appears to produce different results when running locally vs. CI (this commit's tests do not fail for me locally on my mac using node v12.16.1, though they appear to be failing in CI). I'm guessing I'm just using it wrong, but I don't see any other examples of it in this repo for help
There was a problem hiding this comment.
https://jestjs.io/docs/en/expect#expectassertionsnumber - it ensures that that many assertions are made, which makes sure tests don't falsely pass if some bug causes the async callback not to run.
There was a problem hiding this comment.
do you know why this function produces inconsistent results / doesn't always have the same number of assertions? i can actually reproduce the inconsistencies locally:
the first run has 47 assertions, but the second only has 45 (i didn't change any code between each run)
There was a problem hiding this comment.
probably something async we're not properly waiting for
There was a problem hiding this comment.
ok, what would you like to do here? again, this type of assertion isn't used anywhere else in the codebase, so i might just have to close this PR and just use our own fork if it'll be blocked without it. i unfortunately don't have time to debug the issue
There was a problem hiding this comment.
went with a slightly different approach
rkoval
force-pushed
the
master
branch
3 times, most recently
from
8bb8aa9 to
863ae67
Compare
test/defaultMetricsTest.js
Outdated
| @@ -84,6 +84,7 @@ describe('collectDefaultMetrics', () => { | |||
| expect(metricValue.labels).toMatchObject(labels); | |||
| }); | |||
| }); | |||
| expect.assertions(45); | |||
There was a problem hiding this comment.
(I'm not sure if it matters, but usually this is the first line of the it function.)
|
I like this approach! Sorry for the hassle but can you rebase on master please? |
|
@zbjornson this has been rebased on the current master |
|
@zbjornson @siimon PTAL |
|
👍 |
I believe this resolves #224. I was also needing a mechanism like this for applying labels to worker processes when running in a clustered environment.
I did it a bit more generically than just the suggested
collectDefaultMetrics({includeWorkerId: true})because our project setup prevents us from using lib/cluster.js. We're using a pm2 wrapper around our nodejs parent process which has some nuances when using the nodejsclustermodule directly from lib/cluster.js. As such, this should appease a wider audience of users who just want to generically apply labels to default metrics (including myself)