-
Notifications
You must be signed in to change notification settings - Fork 166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extended events logging port to v0.20 #1058
Conversation
cmd/verification/main.go
Outdated
@@ -170,7 +170,7 @@ func main() { | |||
rt := fvm.NewInterpreterRuntime() | |||
vm := fvm.NewVirtualMachine(rt) | |||
vmCtx := fvm.NewContext(node.Logger, node.FvmOptions...) | |||
chunkVerifier := chunks.NewChunkVerifier(vm, vmCtx) | |||
chunkVerifier := chunks.NewChunkVerifier(vm, vmCtx, node.Logger.With().Str("component", "chunk_verifier").Logger()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be more readable if we just pass the logger here and do the rest internally. Otherwise, we always should make sure that the caller takes care of this part. As seen in the rest of PR, we need to replicate this pattern at least three times on all invocations, which literally makes code less DRY (Don't Repeat Yourself).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chunkVerifier := chunks.NewChunkVerifier(vm, vmCtx, node.Logger.With().Str("component", "chunk_verifier").Logger()) | |
chunkVerifier := chunks.NewChunkVerifier(vm, vmCtx, node.Logger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right, this will be better, let me refactor it
module/chunks/chunkVerifier.go
Outdated
@@ -41,6 +44,7 @@ func NewChunkVerifier(vm VirtualMachine, vmCtx fvm.Context) *ChunkVerifier { | |||
fvm.WithServiceEventCollectionEnabled(), | |||
fvm.WithTransactionProcessors(fvm.NewTransactionInvocator(vmCtx.Logger)), | |||
), | |||
logger: logger, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger: logger, | |
logger: logger.With().Str("component", "chunk_verifier").Logger(), |
engine/testutil/nodes.go
Outdated
@@ -828,7 +828,7 @@ func VerificationNode(t testing.TB, | |||
fvm.WithBlocks(blockFinder), | |||
) | |||
|
|||
chunkVerifier := chunks.NewChunkVerifier(vm, vmCtx) | |||
chunkVerifier := chunks.NewChunkVerifier(vm, vmCtx, node.Log.With().Str("component", "chunk_verifier").Logger()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chunkVerifier := chunks.NewChunkVerifier(vm, vmCtx, node.Log.With().Str("component", "chunk_verifier").Logger()) | |
chunkVerifier := chunks.NewChunkVerifier(vm, vmCtx, node.Log) |
@@ -191,7 +191,7 @@ func executeBlockAndVerify(t *testing.T, txs [][]*flow.TransactionBody) *executi | |||
_, chdps, er, err := execution.GenerateExecutionResultAndChunkDataPacks(prevResultId, initialCommit, computationResult) | |||
require.NoError(t, err) | |||
|
|||
verifier := chunks.NewChunkVerifier(vm, fvmContext) | |||
verifier := chunks.NewChunkVerifier(vm, fvmContext, logger.With().Str("component", "chunk_verifier").Logger()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
verifier := chunks.NewChunkVerifier(vm, fvmContext, logger.With().Str("component", "chunk_verifier").Logger()) | |
verifier := chunks.NewChunkVerifier(vm, fvmContext, logger) |
Codecov Report
@@ Coverage Diff @@
## v0.20 #1058 +/- ##
==========================================
+ Coverage 53.29% 53.41% +0.11%
==========================================
Files 318 318
Lines 21513 21505 -8
==========================================
+ Hits 11466 11487 +21
+ Misses 8483 8450 -33
- Partials 1564 1568 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
1088: Back-port `v0.20` fixes for `canary7` to `master` r=jordanschalm a=jordanschalm Backport fixes on the `v0.20` branch to `master`: - [x] #1036 - [x] #1057 - [x] #1058 - [x] #1080 - [x] #1090 - [x] #1096 - [x] #1124 - [x] #1125 Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Maks Pawlak <120831+m4ksio@users.noreply.github.com> Co-authored-by: Khalil Claybon <kclaybon1@gmail.com>
No description provided.