[All] Entity-centric tracing #1243

ramtinms · 2021-09-02T20:39:49Z

This PR

refactors tracing to be entity centric
- for entities (block, collection, transaction) entityID is going to be set as traceID
- a dummy RootEntity span is also constructed to help keeping spans of an entity generated from the same node together and aligned
- most of new methods returns isSampled to avoid extra computations for tag values
- provides in-house proper entity sampling so we don't have to tune params from Jaeger
removes non-operational traces (e.g. time between two events), those are going to be captured differently in the future PRs
refactors components to pass trace spans using contexts
injects tracer into Finalizer
removes consensusTracingConsumer as it was not in use

…counts

…thub.com:onflow/flow-go into ramtin/allocate-storage-addresses-under-accounts

ramtinms · 2021-09-02T20:44:55Z

module/metrics/execution.go

@@ -484,13 +484,15 @@ func NewExecutionCollector(tracer module.Tracer, registerer prometheus.Registere
 // StartBlockReceivedToExecuted starts a span to trace the duration of a block
 // from being received for execution to execution being finished
 func (ec *ExecutionCollector) StartBlockReceivedToExecuted(blockID flow.Identifier) {
-	ec.tracer.StartSpan(blockID, executionBlockReceivedToExecuted).SetTag("block_id", blockID.String)


we need to define a metric instead of tracing here, e.g. blockExecutionWaitTime

ramtinms · 2021-09-02T20:46:57Z

cmd/scaffold.go

@@ -291,7 +291,8 @@ func (fnb *FlowNodeBuilder) initMetrics() {

 	fnb.Tracer = trace.NewNoopTracer()
 	if fnb.BaseConfig.tracerEnabled {
-		tracer, err := trace.NewTracer(fnb.Logger, fnb.BaseConfig.NodeRole)
+		serviceName := fnb.BaseConfig.NodeRole + "-" + fnb.BaseConfig.nodeIDHex[:8]
+		tracer, err := trace.NewTracer(fnb.Logger, serviceName)


We consider each node role + the first 8 digits of the id as service id since info like consensus-001 is not available here.

…flow-go into ramtin/entity-life-cycle-tracing

engine/consensus/matching/core.go

engine/consensus/sealing/core.go

engine/consensus/compliance/core.go

module/trace/trace.go

model/flow/identifier.go

module/tracer.go

Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>

…flow-go into ramtin/entity-life-cycle-tracing

model/flow/identifier.go

This reverts commit edcd95b.

…flow-go into ramtin/entity-life-cycle-tracing

zhangchiqing

Nice improvement!

m4ksio

I really like the change, but (there is always a but) - I really think we should have some tests for it.
Even adding a tracer for existing tests and asserting is traces matches what we would expect will greatly increase confidence

cmd/scaffold.go

m4ksio · 2021-09-17T22:30:04Z

engine/execution/computation/computer/computer.go


 	e.log.Debug().
 		Hex("tx_id", logging.Entity(txBody)).
 		Msg("executing transaction")

-	txView := collectionView.NewChild()
-
 	tx := fvm.Transaction(txBody, txIndex)


Don't we want to move this below trace span as well?

not sure if I understand? you mean the NewChild() call?

SetTraceSpan won't start a trace, it would only pass the parent span reference to transaction object

engine/execution/state/state.go

model/flow/identifier.go

ramtinms · 2021-09-20T16:54:06Z

@m4ksio this PR is already really big, going to add tests for the tracers in a follow up PR to this one.

Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>

2824: chore(tests): remove unused mock r=SaveTheRbtz a=SaveTheRbtz `MutationContext` was removed in 019f7b4 (part of #1243). PS. `@gomisha` I'm curious how many of these unused mack files we have in the repo. Maybe we can automagically detect them during the lint phase? Co-authored-by: Alexey Ivanov <rbtz@dapperlabs.com>

ramtinms and others added 16 commits July 27, 2021 10:19

allocate storage addresses under account

a39b83c

fix error content

2d90e67

hot fix

079fb45

remove magic number 8

f88acba

rename and adapt

abff40c

start storage index from 1

b4d2e79

add test

375f9ed

Merge branch 'master' into ramtin/allocate-storage-addresses-under-ac…

2fa3e29

…counts

Merge branch 'master' into ramtin/allocate-storage-addresses-under-ac…

6881a50

…counts

add AllocateStorageIndex for scripts env as well.

f088771

Merge branch 'ramtin/allocate-storage-addresses-under-accounts' of gi…

abf87cc

…thub.com:onflow/flow-go into ramtin/allocate-storage-addresses-under-accounts

init

3de6ee6

improve tracing

5954e51

temp comment out

5d944fe

spanID construction

d04dae7

update tracing interface to be entity driven

13563d8

ramtinms requested review from AlexHentschel, janezpodhostnik, jordanschalm, m4ksio, yhassanzadeh13 and zhangchiqing as code owners September 2, 2021 20:39

ramtinms commented Sep 2, 2021

View reviewed changes

ramtinms added 6 commits September 2, 2021 15:36

clean up consensus tracing consumer

019f7b4

update follower engine tracing

d4a445e

clean up the consensus traces

24131fa

hot fix

c144f9b

clean up

537f150

take out details of tx execution from block span

7c71640

ramtinms and others added 5 commits September 16, 2021 09:13

Merge branch 'master' into ramtin/entity-life-cycle-tracing

615342d

clean up exec node traces

abd41fa

Merge branch 'ramtin/entity-life-cycle-tracing' of github.com:onflow/…

90c7095

…flow-go into ramtin/entity-life-cycle-tracing

cleanup

8fff950

Merge branch 'master' into ramtin/entity-life-cycle-tracing

c45ea80

ramtinms requested a review from Kay-Zee September 16, 2021 18:20

zhangchiqing reviewed Sep 17, 2021

View reviewed changes

ramtinms and others added 7 commits September 17, 2021 14:14

Update model/flow/identifier.go

f825051

Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>

applying PR comments

e7f922f

Merge branch 'ramtin/entity-life-cycle-tracing' of github.com:onflow/…

70b5e5a

…flow-go into ramtin/entity-life-cycle-tracing

refactor sampling metod

edcd95b

hot fix

8367a65

hot fix

d36da8e

Merge branch 'master' into ramtin/entity-life-cycle-tracing

acc8f72

zhangchiqing reviewed Sep 17, 2021

View reviewed changes

model/flow/identifier.go Outdated Show resolved Hide resolved

ramtinms added 2 commits September 17, 2021 15:29

Revert "refactor sampling metod"

3d6967c

This reverts commit edcd95b.

Merge branch 'ramtin/entity-life-cycle-tracing' of github.com:onflow/…

9cdb472

…flow-go into ramtin/entity-life-cycle-tracing

zhangchiqing approved these changes Sep 17, 2021

View reviewed changes

m4ksio approved these changes Sep 17, 2021

View reviewed changes

ramtinms and others added 6 commits September 20, 2021 10:06

rename save State

32e6101

cleanup unnecesary data

e665a7a

add chainID as a tag on root span

4ec185b

Update model/flow/identifier.go

a44aade

Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>

hot fix

bd81151

Merge branch 'master' into ramtin/entity-life-cycle-tracing

30d9c66

ramtinms merged commit a783037 into master Sep 20, 2021

ramtinms deleted the ramtin/entity-life-cycle-tracing branch September 20, 2021 18:47

SaveTheRbtz mentioned this pull request Jul 19, 2022

[Testing] remove unused mock #2824

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[All] Entity-centric tracing #1243

[All] Entity-centric tracing #1243

ramtinms commented Sep 2, 2021 •

edited

ramtinms Sep 2, 2021

ramtinms Sep 2, 2021 •

edited

zhangchiqing left a comment

m4ksio left a comment

m4ksio Sep 17, 2021

ramtinms Sep 20, 2021

ramtinms Sep 20, 2021

ramtinms commented Sep 20, 2021

[All] Entity-centric tracing #1243

[All] Entity-centric tracing #1243

Conversation

ramtinms commented Sep 2, 2021 • edited

ramtinms Sep 2, 2021

Choose a reason for hiding this comment

ramtinms Sep 2, 2021 • edited

Choose a reason for hiding this comment

zhangchiqing left a comment

Choose a reason for hiding this comment

m4ksio left a comment

Choose a reason for hiding this comment

m4ksio Sep 17, 2021

Choose a reason for hiding this comment

ramtinms Sep 20, 2021

Choose a reason for hiding this comment

ramtinms Sep 20, 2021

Choose a reason for hiding this comment

ramtinms commented Sep 20, 2021

ramtinms commented Sep 2, 2021 •

edited

ramtinms Sep 2, 2021 •

edited