2831 use metered logging instead rollbar

[akaspin]

trello

[gburanov]

This approach completely removes rollar even for critical cases (that should be immediately reported). Why? Original ticket was only about some errors

[akaspin]

@gburanov Do you mean https://github.com/remerge/rex/pull/21/files#diff-39bc10f00ee6210b92b42aae65670f40R60. This rollbar reporting catches panic and not crashes the app and floods rollbar. In comparison with old approach in this PR rollbar replaced with alert-log.

[gburanov]

why not 1.11?)

[akaspin]

Good point. 👍

[gburanov]

But it make sense to be notified using rollbar when u have panic in your app. And if panic happens so often that is SPAMS, maybe it needs to be fixed.

[gburanov]

Again, the original story IMHO was about error that happen very often (because of network or because of 3rd party problem) - but it will not panic in this case. Or?

[akaspin]

@gburanov It's not a panic actually ;-) It's just lazy way to catch errors during request.

[gburanov]

Ok, so now we catch ALL errors and send NOTHING to rollbar (only to log and to metrics).

Question: Why? We supposed to send ONLY repetitive errors to metrics instead of rollbar IMHO

[akaspin]

@gburanov Rex (and Act) are deprecated. It's pain from first sight. But right now it's a opportunity to find approach. Rollbar is expensive solution. We have a lot of options:

1. Logging with targeted appenders: namespaces, levels ... Example: all errors goes to log with metrics, all critical errors goes to rollbar synchronously.
2. Own error tracking solution: Sentry, APM.
3. ???

In current "catch all" approach divide errors (and panics) is pure pain. But the point is that app remains running after and may flood Rollbar ("your account will be upgraded" ... AAA!!!). This PR fixes issue.

[itszootime]

I agree with @gburanov - we should only use the log+metric approach for these repetitive errors we expect, e.g. "invalid URL escape", "appnexus preview asset not found". We should still report unexpected errors to Rollbar. The Rollbar flooding issue can be mitigated with rate limiting (already set) and disabling the auto plan upgrade.

[akaspin]

@gburanov @itszootime Ok. Done. But note very interesting Rollbar behaviour in https://rollbar.com/remerge/act/items/12900/. Actually it's not "invalid url escape" but "read tcp 207.244.122.35:443->52.4.254.244:37530: use of closed network connection". This PR affects only Gin errors. ... Hmm. I was young and stupid. Sec.

UPD: Done again. Now rex reporting to rollbar only on panic inside handler. Panics are our issues - they ours. Other errors are related to other side.

[gburanov]

skill why do we change it here from rollbar to logs ?

[gburanov]

@akaspin but again, why?

• @itszootime wrote "we should ONLY use the log+metric approach for these repetitive errors we expect"

• And u wrote later "Now rex reporting to rollbar only on panic inside handler"

These are 2 different and controversial statements as for me

I think we need to use logs ONLY for the repetative errors specified in the tickect. ONLY. All the other errors should be sent to rolbar as they are now (no change here)