Add expired tx logging #484

dstepanov-yadro · 2023-06-29T07:37:35Z

dstepanov-yadro commented

2023-06-29 07:37:35 +00:00

Relates #457

There were errors in the logs related to expired transactions. I want to add transaction logging for further investigation.

Relates https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/457 There were errors in the logs related to expired transactions. I want to add transaction logging for further investigation.

dstepanov-yadro force-pushed fix/notary_prep_logger from 6f2990a9b0 to 1f91a5361c

2023-06-29 07:38:22 +00:00

Compare

dstepanov-yadro force-pushed fix/notary_prep_logger from 1f91a5361c to 82e69bdf0f

2023-06-29 07:41:05 +00:00

Compare

dstepanov-yadro requested review from storage-core-committers 2023-06-29 07:42:25 +00:00

dstepanov-yadro requested review from storage-core-developers 2023-06-29 07:42:25 +00:00

acid-ant approved these changes 2023-06-29 08:04:21 +00:00

aarifullin reviewed 2023-06-29 09:11:45 +00:00

pkg/morph/event/stringer.go Outdated

					
				@ -0,0 +12,4 @@

				func (s *txStringer) String() string {

					if s == nil {

						return "stringer is null"

aarifullin commented

2023-06-29 09:11:45 +00:00

It means that we'll see in logs zapped message like

{ ..., "main_tx": "stringer is null", "fallback_tx": "transaction is null" ... }

Is it really convinient? For example, for message parsing.
I'd like to return just "null" or "nil" in both statements. WDYT?

It means that we'll see in logs zapped message like ``` { ..., "main_tx": "stringer is null", "fallback_tx": "transaction is null" ... } ``` Is it really convinient? For example, for message parsing. I'd like to return just "null" or "nil" in both statements. WDYT?

fyrchik commented

2023-06-29 15:04:57 +00:00

Is this line even reachable?

dstepanov-yadro commented

2023-06-30 06:41:24 +00:00

fixed

aarifullin approved these changes 2023-06-29 09:18:00 +00:00

fyrchik reviewed 2023-06-29 15:06:26 +00:00

pkg/morph/event/stringer.go Outdated

					
				@ -0,0 +17,4 @@

					if s.tx == nil {

						return "transaction is null"

					}

					data, err := s.tx.MarshalJSON()

fyrchik commented

2023-06-29 15:06:26 +00:00

Wow, this could be A LOT OF text, script size can reach 64 KiB. Do we need it?

dstepanov-yadro commented

2023-06-30 06:42:55 +00:00

Fixed logging. Now only heights will be logged, because .Hash() may cause panic, there is already unit test for this behaviour.

Fixed logging. Now only heights will be logged, because `.Hash()` may cause panic, there is already unit test for this behaviour.

fyrchik marked this conversation as resolved

dstepanov-yadro force-pushed fix/notary_prep_logger from 82e69bdf0f to 05c5ed3aa7

2023-06-30 06:40:57 +00:00

Compare

fyrchik reviewed 2023-06-30 12:45:02 +00:00

pkg/morph/event/listener.go Outdated

					
				@ -370,3 +370,3 @@

						case errors.Is(err, ErrMainTXExpired):

							l.log.Warn(logs.EventSkipExpiredMainTXNotaryEvent,

								zap.String("error", err.Error()),

								zap.Error(err),

fyrchik commented

2023-06-30 12:42:00 +00:00

I believe initial reasons for using String were to avoid having stacktrace for not-so-rare warnings.
Anyway, seems unrelated to the commit, move?

I believe initial reasons for using `String` were to avoid having stacktrace for not-so-rare warnings. Anyway, seems unrelated to the commit, move?

dstepanov-yadro commented

2023-06-30 13:30:35 +00:00

ok, fixed

pkg/morph/event/notary_preparator.go Outdated

					
				@ -308,6 +318,10 @@ func (p Preparator) validateExpiration(fbTX *transaction.Transaction) error {

					}

					if currBlock >= nvb.Height {

						p.logger.Warn(logs.MorphExpiredTransactionReceived,