-
Notifications
You must be signed in to change notification settings - Fork 67
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: allow a bulk subscription, to get all metrics at once. #134
Conversation
I'll add any applicable docs/tests if this is a change you are interested in :) |
@zachdaniel I think I fixed up the Travis build, but you'll need to rebase your changes on 6e22ec8 to get your PR to pass. |
Added some documentation. |
I can add some tests, but won't have time until tomorrow evening most likely. |
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.
This looks good! Will give it another look once you have a change to add tests.
@zachdaniel, just wanted to follow-up regarding test coverage. Once that's in place, I think we're in good shape to merge this. |
Sorry, dropped the ball on this one, been pretty busy. I'll add this to my list and get it done soon :) |
@jparise Alright, I added a test for this. Sorry for the hold up, I've been very busy! |
@zachdaniel it looks like there are a few test failures. |
Huh, they all passed on my machine :) I'll check it out. |
It is especially interesting because it succeeded on one of those jobs but not the others. This would lead me to believe that there is some kind of race condition involved. Perhaps creating a subscription for multiple metrics at once takes a bit longer than the others and so this test fails sometimes? The only thing I can think of to do is add a wait :/ |
Going to try it with a function that will wait a bit if the subscription doesn't exist yet and see how that goes. https://github.com/pinterest/elixometer/pull/134/files#diff-82d59e151908e3896650c999ba68dc0cR108 |
Eek. more passed this time, but not all of them! |
🤦♂ It would help if I actually used the wait time. |
I must be missing something. I have it set up where it will wait up to 5 seconds for the subscription to be created. Unless the build boxes are running on potatoes, that should be plenty. As I increase the amount of time it will potentially wait, more seem to pass though... Maybe the metrics list is coming back in a different order, actually. Let me try that. |
Nope. Taking out the wait and just ensuring that they are sorted made it so that all of them pass. I guess I'll just make it so that it will wait a very long time and see if that works. Sorry for all of the spam, this has just been a surprisingly weird problem. |
Alright, I think I'm at a loss here. I can't get the test to fail on my machine. I've got it waiting way longer than could possibly be necessary, its failing in some but not all of the builds and which ones it is failing in change every time. |
Oh, actually I think I know what it is. One of the tests is setting |
Co-Authored-By: Jon Parise <jon@indelible.org>
whew that was it :) Thought I was going crazy there. Since exunit randomizes the test order, some tests would pass because the Resetting it after the test runs here: https://github.com/pinterest/elixometer/pull/134/files#diff-82d59e151908e3896650c999ba68dc0cR437 resolved the issue. |
@jparise ready for you now :) |
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.
Great!
If you want to avoid having to aggregate each different datapoint for each metric, you can use
report_bulk: true
in your reporters, like so:But in order for that to work, the subscription for each datapoint needs to be given at one time, as opposed to separate subscriptions. This allows that, by allowing the configuration option
bulk_subscribe: true