search

librato / librato-metrics

Ruby wrapper to make it easy to interact with Librato's API.
https://librato.com
Other
108 stars 51 forks source link

Add warning message before test runs - production data pruned #127

Open mariuspod opened 7 years ago

mariuspod commented 7 years ago

Hi,

I've just checked out the repo so I could contribute, ran the tests and had 53 failures without touching the code. The rspec output showed always the same error, saying that there was a missing TEST_API_USER and TEST_API_KEY ENV variable so I went to the librato UI of my production account and created a new token for testing and passed this token to the rspec run. The tests started to run and I've noticed a decline in my production metrics in librato UI.

Unfortunately, the test run deleted all my production metrics within seconds due to this line: https://github.com/librato/librato-metrics/blob/727268d4ffd86b183c7c20b1d5861f491689657c/spec/spec_helper.rb#L20 This is really frustrating and it just doesn't look right to me that this can happen at all and that no TEST environment has been evaluated prior to deleting the metrics.

Shouldn't there be at least a big warning sign on the public github repo warning devs about running this in their non-test accounts ? Or even stating that there's a need for an extra test account in order to run the tests would have saved me. One could also include a warning box in spec_helper that runs in a before(:suite) hook with a sleep of 10 seconds so people have a chance to ctrl-c the run ? I wasn't aware of the consequences at all and I would like others to not nuke their production data like I did by running the simple command:

bundle exec rspec

IMHO, this is really not how it should work.

nextmat commented 7 years ago

@standardout - You are absolutely correct. This definitely isn't how it should work. We typically run tests with rake which automatically skips these tests if an account isn't specified. Historically the integration tests exist primarily for internal use.

By running with rspec directly (a very reasonable action) it looks like you ran into the errors produced by the tests that would normally be skipped with rake. It looks like the possibility of doing this was introduced when we upgraded rspec in the last month, as the default runner behavior has changed.

My profound apologies that this was even possible and that your data was affected. I know our support team is working now to restore the affected data. I believe they are already in contact with you but if not or if you need additional information you can reach them at support@librato.com or via the in-app help.

We will also add additional safeguards in this library to make sure this doesn't happen again. Apologies again and we appreciate your patience as we get your data restored.

mariuspod commented 7 years ago

Hi @nextmat , your support team is doing a great job, more and more of my data is showing up again! 👍 Thanks for the explanation what went wrong. For now, I'll create a separate librato test account for running my local tests.

nextmat commented 7 years ago

@standardout - Happy to hear it! Apologies again for the trouble & thank you for helping to improve the library.

nextmat commented 7 years ago

@standardout - One other side note that might be useful: because of small delays in given eventual consistency at our current scale we've been seeing more flapping in our integration suite recently. We've been exploring moving to testing these differently but if you see some inconsistent failures this the likely cause.