-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add testcase to check for missing labels from the generated metrics #116
Add testcase to check for missing labels from the generated metrics #116
Conversation
metrics/serve_test.go
Outdated
col.Stop(nil) | ||
}) | ||
|
||
time.Sleep((2 * time.Second)) |
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.
Do you mind using some kind of wait e.g. assert.Eventually
instead of sleep - much more efficient and reliable - or even better simply hijack (listen to) c.updateNotifyCh
?
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.
Thanks for the feedback @bwplotka it makes sense , I have updated the test to use the c.upateNotifyCh
for assertions
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.
Thanks, I merged #113 first, hope that's ok, but this test is must-have! Added one suggestion, otherwise LGTM, thanks!
893f13a
to
e003269
Compare
Do you mind rebasing? There are some unrelated changes now in this PR |
Signed-off-by: Prateek <[email protected]>
Signed-off-by: Prateek <[email protected]>
Signed-off-by: Prateek <[email protected]>
Signed-off-by: Prateek <[email protected]>
Signed-off-by: Prateek <[email protected]>
7e2ed92
to
7ccc990
Compare
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.
Thanks!
Just updating the PR title as it adds testcase
We have been using avalanche to test out remote write capability in our distributed metrics architecture, we noticed that avalanche wasn't adding any labels let alone the
--const-labels
to the metrics. The solution in the PR should hopefully fix it.Following are the changes in the PR