Skip to content

Commit 1895a15

Browse files
jrheajrhea
committed
Ensure that all spans return ok when there are no errors
1 parent 571e915 commit 1895a15

File tree

4 files changed

+49
-34
lines changed

4 files changed

+49
-34
lines changed

‎eth/catalyst/api.go‎

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -615,32 +615,45 @@ func (api *ConsensusAPI) GetBlobsV2(hashes []common.Hash) ([]*engine.BlobAndProo
615615
// Helper for NewPayload* methods.
616616
var invalidStatus = engine.PayloadStatusV1{Status: engine.INVALID}
617617

618-
// startNewPayloadSpan starts a tracing span for a new payload.
619-
func startNewPayloadSpan(ctx context.Context, name string, params engine.ExecutableData) (context.Context, trace.Span) {
618+
// startNewPayloadSpan starts a tracing span for a new payload and returns a cleanup
619+
// function that should be deferred. spanEnd() will record errors and set
620+
// span status based on the error value at the time of execution.
621+
func startNewPayloadSpan(ctx context.Context, name string, params engine.ExecutableData) (context.Context, func(*error)) {
622+
parentSpan := trace.SpanFromContext(ctx)
620623
tracer := otel.Tracer("")
621624
ctx, span := tracer.Start(ctx, name)
622625
span.SetAttributes(
623626
attribute.Int64("block.number", int64(params.Number)),
624627
attribute.String("block.hash", params.BlockHash.Hex()),
625628
attribute.Int("tx.count", len(params.Transactions)),
626629
)
627-
return ctx, span
630+
spanEnd := func(err *error) {
631+
if *err != nil {
632+
span.RecordError(*err)
633+
span.SetStatus(codes.Error, (*err).Error())
634+
parentSpan.SetStatus(codes.Error, (*err).Error())
635+
} else {
636+
span.SetStatus(codes.Ok, "")
637+
}
638+
span.End()
639+
}
640+
return ctx, spanEnd
628641
}
629642

630643
// NewPayloadV1 creates an Eth1 block, inserts it in the chain, and returns the status of the chain.
631-
func (api *ConsensusAPI) NewPayloadV1(ctx context.Context, params engine.ExecutableData) (engine.PayloadStatusV1, error) {
632-
ctx, span := startNewPayloadSpan(ctx, "engine.newPayloadV1", params)
633-
defer span.End()
644+
func (api *ConsensusAPI) NewPayloadV1(ctx context.Context, params engine.ExecutableData) (result engine.PayloadStatusV1, err error) {
645+
ctx, spanEnd := startNewPayloadSpan(ctx, "engine.newPayloadV1", params)
646+
defer spanEnd(&err)
634647
if params.Withdrawals != nil {
635648
return invalidStatus, paramsErr("withdrawals not supported in V1")
636649
}
637650
return api.newPayload(ctx, params, nil, nil, nil, false)
638651
}
639652

640653
// NewPayloadV2 creates an Eth1 block, inserts it in the chain, and returns the status of the chain.
641-
func (api *ConsensusAPI) NewPayloadV2(ctx context.Context, params engine.ExecutableData) (engine.PayloadStatusV1, error) {
642-
ctx, span := startNewPayloadSpan(ctx, "engine.newPayloadV2", params)
643-
defer span.End()
654+
func (api *ConsensusAPI) NewPayloadV2(ctx context.Context, params engine.ExecutableData) (result engine.PayloadStatusV1, err error) {
655+
ctx, spanEnd := startNewPayloadSpan(ctx, "engine.newPayloadV2", params)
656+
defer spanEnd(&err)
644657
var (
645658
cancun = api.config().IsCancun(api.config().LondonBlock, params.Timestamp)
646659
shanghai = api.config().IsShanghai(api.config().LondonBlock, params.Timestamp)
@@ -661,9 +674,9 @@ func (api *ConsensusAPI) NewPayloadV2(ctx context.Context, params engine.Executa
661674
}
662675

663676
// NewPayloadV3 creates an Eth1 block, inserts it in the chain, and returns the status of the chain.
664-
func (api *ConsensusAPI) NewPayloadV3(ctx context.Context, params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash) (engine.PayloadStatusV1, error) {
665-
ctx, span := startNewPayloadSpan(ctx, "engine.newPayloadV3", params)
666-
defer span.End()
677+
func (api *ConsensusAPI) NewPayloadV3(ctx context.Context, params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash) (result engine.PayloadStatusV1, err error) {
678+
ctx, spanEnd := startNewPayloadSpan(ctx, "engine.newPayloadV3", params)
679+
defer spanEnd(&err)
667680
switch {
668681
case params.Withdrawals == nil:
669682
return invalidStatus, paramsErr("nil withdrawals post-shanghai")
@@ -682,9 +695,9 @@ func (api *ConsensusAPI) NewPayloadV3(ctx context.Context, params engine.Executa
682695
}
683696

684697
// NewPayloadV4 creates an Eth1 block, inserts it in the chain, and returns the status of the chain.
685-
func (api *ConsensusAPI) NewPayloadV4(ctx context.Context, params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash, executionRequests []hexutil.Bytes) (engine.PayloadStatusV1, error) {
686-
ctx, span := startNewPayloadSpan(ctx, "engine.newPayloadV4", params)
687-
defer span.End()
698+
func (api *ConsensusAPI) NewPayloadV4(ctx context.Context, params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash, executionRequests []hexutil.Bytes) (result engine.PayloadStatusV1, err error) {
699+
ctx, spanEnd := startNewPayloadSpan(ctx, "engine.newPayloadV4", params)
700+
defer spanEnd(&err)
688701
switch {
689702
case params.Withdrawals == nil:
690703
return invalidStatus, paramsErr("nil withdrawals post-shanghai")
@@ -702,7 +715,7 @@ func (api *ConsensusAPI) NewPayloadV4(ctx context.Context, params engine.Executa
702715
return invalidStatus, unsupportedForkErr("newPayloadV4 must only be called for prague/osaka payloads")
703716
}
704717
requests := convertRequests(executionRequests)
705-
if err := validateRequests(requests); err != nil {
718+
if err = validateRequests(requests); err != nil {
706719
return engine.PayloadStatusV1{Status: engine.INVALID}, engine.InvalidParams.With(err)
707720
}
708721
return api.newPayload(ctx, params, versionedHashes, beaconRoot, requests, false)

‎eth/catalyst/api_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -503,11 +503,11 @@ func setupBlocks(t *testing.T, ethservice *eth.Ethereum, n int, parent *types.He
503503

504504
envelope := getNewEnvelope(t, api, parent, w, h)
505505

506-
// NOTE: This span is for the test harness only. Engine oot spans are created
506+
// NOTE: This span is for the test harness only. Engine root spans are created
507507
// in NewPayloadV* entrypoints. This test calls newPayload() directly.
508-
ctx, span := startNewPayloadSpan(context.Background(), "engine.api_test.setupBlocks", *envelope.ExecutionPayload)
509-
defer span.End()
508+
ctx, spanEnd := startNewPayloadSpan(context.Background(), "engine.api_test.setupBlocks", *envelope.ExecutionPayload)
510509
execResp, err := api.newPayload(ctx, *envelope.ExecutionPayload, []common.Hash{}, h, envelope.Requests, false)
510+
spanEnd(&err)
511511
if err != nil {
512512
t.Fatalf("can't execute payload: %v", err)
513513
}

‎eth/catalyst/simulated_beacon.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -258,9 +258,11 @@ func (c *SimulatedBeacon) sealBlock(withdrawals []*types.Withdrawal, timestamp u
258258

259259
// NOTE: This span is for the simulated beacon harness only. Normal tracing
260260
// of engine_newPayload* is performed at the Engine API entrypoints.
261-
ctx, span := startNewPayloadSpan(context.Background(), "engine.simulatedBeacon.sealBlock", *payload)
262-
defer span.End()
261+
ctx, spanEnd := startNewPayloadSpan(context.Background(), "engine.simulatedBeacon.sealBlock", *payload)
262+
263+
// Mark the payload as canon
263264
_, err = c.engineAPI.newPayload(ctx, *payload, blobHashes, beaconRoot, requests, false)
265+
spanEnd(&err)
264266
if err != nil {
265267
return err
266268
}

‎eth/catalyst/witness.go‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -87,18 +87,20 @@ func (api *ConsensusAPI) ForkchoiceUpdatedWithWitnessV3(update engine.Forkchoice
8787

8888
// NewPayloadWithWitnessV1 is analogous to NewPayloadV1, only it also generates
8989
// and returns a stateless witness after running the payload.
90-
func (api *ConsensusAPI) NewPayloadWithWitnessV1(params engine.ExecutableData) (engine.PayloadStatusV1, error) {
90+
func (api *ConsensusAPI) NewPayloadWithWitnessV1(params engine.ExecutableData) (result engine.PayloadStatusV1, err error) {
91+
ctx, spanEnd := startNewPayloadSpan(context.Background(), "engine.newPayloadWithWitnessV1", params)
92+
defer spanEnd(&err)
9193
if params.Withdrawals != nil {
9294
return engine.PayloadStatusV1{Status: engine.INVALID}, engine.InvalidParams.With(errors.New("withdrawals not supported in V1"))
9395
}
94-
ctx, span := startNewPayloadSpan(context.Background(), "engine.newPayloadWithWitnessV1", params)
95-
defer span.End()
9696
return api.newPayload(ctx, params, nil, nil, nil, true)
9797
}
9898

9999
// NewPayloadWithWitnessV2 is analogous to NewPayloadV2, only it also generates
100100
// and returns a stateless witness after running the payload.
101-
func (api *ConsensusAPI) NewPayloadWithWitnessV2(params engine.ExecutableData) (engine.PayloadStatusV1, error) {
101+
func (api *ConsensusAPI) NewPayloadWithWitnessV2(params engine.ExecutableData) (result engine.PayloadStatusV1, err error) {
102+
ctx, spanEnd := startNewPayloadSpan(context.Background(), "engine.newPayloadWithWitnessV2", params)
103+
defer spanEnd(&err)
102104
var (
103105
cancun = api.config().IsCancun(api.config().LondonBlock, params.Timestamp)
104106
shanghai = api.config().IsShanghai(api.config().LondonBlock, params.Timestamp)
@@ -115,14 +117,14 @@ func (api *ConsensusAPI) NewPayloadWithWitnessV2(params engine.ExecutableData) (
115117
case params.BlobGasUsed != nil:
116118
return invalidStatus, paramsErr("non-nil blobGasUsed pre-cancun")
117119
}
118-
ctx, span := startNewPayloadSpan(context.Background(), "engine.newPayloadWithWitnessV2", params)
119-
defer span.End()
120120
return api.newPayload(ctx, params, nil, nil, nil, true)
121121
}
122122

123123
// NewPayloadWithWitnessV3 is analogous to NewPayloadV3, only it also generates
124124
// and returns a stateless witness after running the payload.
125-
func (api *ConsensusAPI) NewPayloadWithWitnessV3(params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash) (engine.PayloadStatusV1, error) {
125+
func (api *ConsensusAPI) NewPayloadWithWitnessV3(params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash) (result engine.PayloadStatusV1, err error) {
126+
ctx, spanEnd := startNewPayloadSpan(context.Background(), "engine.newPayloadWithWitnessV3", params)
127+
defer spanEnd(&err)
126128
switch {
127129
case params.Withdrawals == nil:
128130
return invalidStatus, paramsErr("nil withdrawals post-shanghai")
@@ -137,14 +139,14 @@ func (api *ConsensusAPI) NewPayloadWithWitnessV3(params engine.ExecutableData, v
137139
case !api.checkFork(params.Timestamp, forks.Cancun):
138140
return invalidStatus, unsupportedForkErr("newPayloadV3 must only be called for cancun payloads")
139141
}
140-
ctx, span := startNewPayloadSpan(context.Background(), "engine.newPayloadWithWitnessV3", params)
141-
defer span.End()
142142
return api.newPayload(ctx, params, versionedHashes, beaconRoot, nil, true)
143143
}
144144

145145
// NewPayloadWithWitnessV4 is analogous to NewPayloadV4, only it also generates
146146
// and returns a stateless witness after running the payload.
147-
func (api *ConsensusAPI) NewPayloadWithWitnessV4(params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash, executionRequests []hexutil.Bytes) (engine.PayloadStatusV1, error) {
147+
func (api *ConsensusAPI) NewPayloadWithWitnessV4(params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash, executionRequests []hexutil.Bytes) (result engine.PayloadStatusV1, err error) {
148+
ctx, spanEnd := startNewPayloadSpan(context.Background(), "engine.newPayloadWithWitnessV4", params)
149+
defer spanEnd(&err)
148150
switch {
149151
case params.Withdrawals == nil:
150152
return invalidStatus, paramsErr("nil withdrawals post-shanghai")
@@ -162,11 +164,9 @@ func (api *ConsensusAPI) NewPayloadWithWitnessV4(params engine.ExecutableData, v
162164
return invalidStatus, unsupportedForkErr("newPayloadV4 must only be called for prague/osaka payloads")
163165
}
164166
requests := convertRequests(executionRequests)
165-
if err := validateRequests(requests); err != nil {
167+
if err = validateRequests(requests); err != nil {
166168
return engine.PayloadStatusV1{Status: engine.INVALID}, engine.InvalidParams.With(err)
167169
}
168-
ctx, span := startNewPayloadSpan(context.Background(), "engine.newPayloadWithWitnessV4", params)
169-
defer span.End()
170170
return api.newPayload(ctx, params, versionedHashes, beaconRoot, requests, true)
171171
}
172172

0 commit comments

Comments
 (0)