feat: exit on error with some exception #66

ratik · 2022-11-25T12:26:31Z

As discussed we don't need to resubmit msg as described in the task instead we exit on every error but ones defined in exception.
So default behavior: it will exit on all errors but contract errors

swelf19 · 2022-11-25T13:08:30Z

internal/txprocessor/txprocessor.go

+		// check error with regexp
+		if !r.ignoreErrorsRegexp.MatchString(err.Error()) {
+			r.logger.Error("failed to submit tx proof", zap.Error(err))
+			os.Exit(3)


the only thing is bothering me

We ignore all our graceful shutdown

I reuse ProcessAndsubmit https://github.com/neutron-org/neutron-query-relayer/blob/feat/resubmit-failed-txs/internal/webserver/webserver.go#L87 which in case of network/gas error just stops the daemon with os.Exit(3)

I would consider to pass a critical error up on the stack

absolutely so, we shouldn't just os.Exit() here

zavgorodnii · 2022-11-29T10:54:48Z

this PR also required a comprehensive description in the docs (with motivation, examples & suggestions)

ratik · 2022-11-30T11:45:40Z

this PR also required a comprehensive description in the docs (with motivation, examples & suggestions)

neutron-org/neutron-docs#36
@oopcode @swelf19

swelf19 · 2022-12-01T13:46:59Z

internal/relay/relayer.go

@@ -80,6 +80,7 @@ func (r *Relayer) Run(
 			if err != nil {
 				r.logger.Error("could not process message", zap.Uint64("query_id", query.Id), zap.Error(err))
 				neutronmetrics.AddFailedRequest(string(query.QueryType), time.Since(start).Seconds())
+				return err


Looks like we stop the app in case of any error in the processMessageKV and processMessageTX, regardless is the error happends during tx submition(we want to stop), or during some KV query on the remote chain rpc(we dont want to stop). We need to detect the error is critical and stop only this case

in addition, this will stop the app if any error in the whole processMessageTX has happened (e.g. if buildTxQuery failed)

@sotnikov-s ok, thanks. added this error into ignore pattern

@swelf19 fixed

internal/txprocessor/txprocessor.go

sotnikov-s · 2022-12-06T09:33:42Z

internal/relay/relayer.go

@@ -80,6 +80,7 @@ func (r *Relayer) Run(
 			if err != nil {
 				r.logger.Error("could not process message", zap.Uint64("query_id", query.Id), zap.Error(err))
 				neutronmetrics.AddFailedRequest(string(query.QueryType), time.Since(start).Seconds())
+				return err


in addition, this will stop the app if any error in the whole processMessageTX has happened (e.g. if buildTxQuery failed)

internal/txprocessor/txprocessor.go

sotnikov-s · 2022-12-06T10:03:06Z

internal/txprocessor/txprocessor.go

-	if err != nil {
-		return fmt.Errorf("failed to store tx: %w", err)
+	// check error with regexp
+	if !r.ignoreErrorsRegexp.MatchString(err.Error()) {


IMO TxProcessor should have nothing to do with this new piece of logic. It's Relayer who decides to ignore err (just log it) or to crash. TxProcessor logic should remain intact in this scope.

Instead, enhance Relayer.processMessageTX and Relayer.processMessageKV methods the way they return a custom error, say, SubmissionError, which contains error severity level:

critical for errors that came from TxProcessor.ProcessAndSubmit and didn't match regexp;

info for all other errors.
Then, the Relayer switches on errors severity (here) and decides whether to stop

I made some changes and they are look like what you described except info thing. Could you please check and let me know if you are happy with these ones?

this is better, but still critical error handling logic is spread between two entities: relayer and txprocessor. I think it would be better to move all critical-noncritical error handling logic to the relayer

internal/config/config.go

sotnikov-s · 2022-12-06T10:12:26Z

.env.example

@@ -31,6 +31,7 @@ RELAYER_QUERIES_TASK_QUEUE_CAPACITY=10000
 RELAYER_CHECK_SUBMITTED_TX_STATUS_DELAY=10s
 RELAYER_INITIAL_TX_SEARCH_OFFSET=0
 RELAYER_PROMETHEUS_PORT=9999
+RELAYER_IGNORE_ERRORS_REGEX=(execute wasm contract failed)


please don't forget to update readme and documentation

sure, as soon as we merge, will update docs

internal/txprocessor/txprocessor.go

sotnikov-s · 2022-12-06T12:39:07Z

internal/txprocessor/txprocessor.go

-	if err != nil {
-		return fmt.Errorf("failed to store tx: %w", err)
+	// check error with regexp
+	if !r.ignoreErrorsRegexp.MatchString(err.Error()) {


this is better, but still critical error handling logic is spread between two entities: relayer and txprocessor. I think it would be better to move all critical-noncritical error handling logic to the relayer

ratik · 2022-12-06T13:20:43Z

this is better, but still critical error handling logic is spread between two entities: relayer and txprocessor. I think it would be better to move all critical-noncritical error handling logic to the relayer

@sotnikov-s can't get how can you manage flow in txProcessor and depend on configuration when you move logic to the relayer

sotnikov-s · 2022-12-06T14:50:27Z

this is better, but still critical error handling logic is spread between two entities: relayer and txprocessor. I think it would be better to move all critical-noncritical error handling logic to the relayer

@sotnikov-s can't get how can you manage flow in txProcessor and depend on configuration when you move logic to the relayer

okay, I agree that it's not good. also: I wouldn't like the critical error to be confounded with an ordinary error, e.g. I don't like the strings comparison as the check for this case. I decided to try to write it myself instead of describing my thoughts here. please take a look: #67

fix: tiny fixes for the crit errors implementation

[NTRN-194] feat: add critical tx submit error

sotnikov-s

tested locally: regex and critical error handling work as expected

feat: exit on error with some exception

fb3b39a

ratik requested a review from zavgorodnii November 25, 2022 12:26

swelf19 reviewed Nov 25, 2022

View reviewed changes

feat: graceful shutdown instead of exit

b9b4ab3

ratik requested a review from swelf19 November 30, 2022 10:00

ratik mentioned this pull request Nov 30, 2022

feat: RELAYER_IGNORE_ERRORS_REGEX neutron-org/neutron-docs#36

Merged

zavgorodnii previously approved these changes Nov 30, 2022

View reviewed changes

swelf19 reviewed Dec 1, 2022

View reviewed changes

swelf19 reviewed Dec 5, 2022

View reviewed changes

internal/txprocessor/txprocessor.go Outdated Show resolved Hide resolved

sotnikov-s requested changes Dec 6, 2022

View reviewed changes

fix: typo

6c8d2b4

ratik dismissed zavgorodnii’s stale review via 6c8d2b4 December 6, 2022 10:30

ratik added 4 commits December 6, 2022 11:35

fix: measure failed tx processing even in case the error is critical

5d52987

fix: exit on critical error

24f9533

fix: ignore build tx query string errors

12b789e

refactor: move to fn

8bfa52b

sotnikov-s reviewed Dec 6, 2022

View reviewed changes

chore: comment

a4151af

add hard crit error for tx submission

e9e0eff

sotnikov-s mentioned this pull request Dec 6, 2022

[NTRN-194] feat: add critical tx submit error #67

Merged

Andrei Zavgorodnii and others added 5 commits December 7, 2022 13:40

fix: tiny fixes for the crit errors implementation

54435d8

prettify SubmitTxProof error handling

740ca21

move critical tx msgs handling to tx processing scope

26cbdbd

Merge pull request #68 from neutron-org/fix/tiny-fixes-for-crit-errors

d0f130b

fix: tiny fixes for the crit errors implementation

Merge pull request #67 from neutron-org/feat/add-crit-tx-submit-error

b55f721

[NTRN-194] feat: add critical tx submit error

ratik requested a review from sotnikov-s December 9, 2022 10:06

ratik requested review from swelf19 and zavgorodnii December 9, 2022 10:06

sotnikov-s approved these changes Dec 9, 2022

View reviewed changes

zavgorodnii approved these changes Dec 13, 2022

View reviewed changes

zavgorodnii merged commit 1cc1354 into main Dec 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: exit on error with some exception #66

feat: exit on error with some exception #66

ratik commented Nov 25, 2022 •

edited by sotnikov-s

Loading

swelf19 Nov 25, 2022

zavgorodnii Nov 29, 2022

zavgorodnii commented Nov 29, 2022

ratik commented Nov 30, 2022 •

edited

Loading

swelf19 Dec 1, 2022

sotnikov-s Dec 6, 2022

ratik Dec 6, 2022

ratik Dec 6, 2022

sotnikov-s Dec 6, 2022

sotnikov-s Dec 6, 2022

ratik Dec 6, 2022

sotnikov-s Dec 6, 2022

sotnikov-s Dec 6, 2022

ratik Dec 6, 2022

sotnikov-s Dec 6, 2022

ratik commented Dec 6, 2022

sotnikov-s commented Dec 6, 2022

sotnikov-s left a comment

feat: exit on error with some exception #66

feat: exit on error with some exception #66

Conversation

ratik commented Nov 25, 2022 • edited by sotnikov-s Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zavgorodnii commented Nov 29, 2022

ratik commented Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ratik commented Dec 6, 2022

sotnikov-s commented Dec 6, 2022

sotnikov-s left a comment

Choose a reason for hiding this comment

ratik commented Nov 25, 2022 •

edited by sotnikov-s

Loading

ratik commented Nov 30, 2022 •

edited

Loading