Don't Report RabbitMQ Close Errors on Staging #1325
Conversation
| @@ -176,6 +176,10 @@ RabbitMQ.prototype.close = function (cb) { | |||
| } | |||
| this.hermesClient.close(function () { | |||
| log.trace('RabbitMQ.prototype.close complete') | |||
| if (process.env.NODE_ENV === 'staging') { | |||
| // Ignore errors on staging close (to avoid alerting) | |||
| return cb.call(this) | |||
|
it seems that it tried to close rabbitmq connection in rabbitmq and failed because connection was already closed. So maybe a better fix would be to handle that in hermes? If |
|
@podviaznikov - While that is true, we also never want any errors to be reported in rollbar for staging hermes close. This is, then, a blanket fix to ensure we don't have staging reporting for this particular use case. |
|
Do we even want to report to rollbar on staging? |
|
What about scenario when you testing something on staging and you want to see rollbar errors? |
|
We don't, but that is a bit of a bigger PR. This fix will help us in the short term. |
|
This LGTM, I'd love to see if we could nix reporting entirely on staging... As it's really not useful. I can't see a case where reporting to rollbar when testing something couldn't be done by reading the logs manually. |
|
@podviaznikov - I am very skeptical that you would be using rollbar for error tracking in the staging environment. My guess is that you would be testing (hopefully soon in isolation) and would be tracking errors in the logs themselves. Rollbar is a monitoring tool that allows you to understand the error behavior of a relatively stable system (such as a production server, etc.) over time. Thus it makes very little sense to use it for real-time testing in a staging environment. |
|
what about testing branches over time. E.x. you have a branch and you run some automated tests over that branch during night. Or you do perf tests and want to see in rollbar if anything wrong happened |
|
@podviaznikov - Do you currently perform this sort of testing? |
|
@podviaznikov - Furthermore, if you needed to do that you should be able to change the reporting behavior in your branch. |
|
@podviaznikov - Just a thought on that, I think we could have a env variable that you could override in your runnable config to tell it whether or not to report staging errors to rollbar. So the default would be to not do so, and then if you needed to keep something more long running you could always switch it back on for your branch. #compromise #politics #thegreatdebate |
I'm not against of this PR (especially because it's a small one) but I do have few concerns:
Because 2, 3 this PR seems like not here not there to me. Unless we are loosing a lot of money on rollbar I'd rather see full fix. But again, this is just and opinion and I'm not against merging it |
|
Let me refer you to... Exhibit A: Exhibit B: |
|
Note how we don't reset on usage until the end of the month 😢 |
|
That is why I said My concern is that you are introducing temporary code. Who is going to remove that code? Do we have JIRA task to have proper implementation and remove this piece you are adding now |
|
Okay, I agree. Let's work a plan about fixing this for all issues. Closing. |
This PR removes error reporting in the
RabbitMQ.prototype.closewhen the server is running withNODE_ENV=staging.The method was reporting quite a few (~6k / week) errors when called on staging. It appears that the server was never started and then a close was executed (via a
SIGINTorSIGTERM, as far as I can tell).While we will need to keep error reporting here so as to indicate a problem with our workers in production, it is a bit superfluous to have all this reporting for staging (where we just restart the container if it is mucking up).
Reviewers: