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

Poster

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

Poster

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

Poster

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,