-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
core/services: fix ocrWrapper saveError contexts #13139
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"chainlink": patch | ||
--- | ||
|
||
core/services: fix ocrWrapper saveError contexts #internal |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,6 @@ import ( | |
|
||
ocr "github.com/smartcontractkit/libocr/offchainreporting2plus" | ||
|
||
commonlogger "github.com/smartcontractkit/chainlink-common/pkg/logger" | ||
"github.com/smartcontractkit/chainlink-common/pkg/loop" | ||
"github.com/smartcontractkit/chainlink-common/pkg/sqlutil" | ||
"github.com/smartcontractkit/chainlink-common/pkg/types" | ||
|
@@ -163,22 +162,23 @@ func (d *Delegate) ServicesForSpec(ctx context.Context, jb job.Job) (services [] | |
"ContractTransmitterTransmitTimeout", lc.ContractTransmitterTransmitTimeout, | ||
"DatabaseTimeout", lc.DatabaseTimeout, | ||
) | ||
ocrLogger := ocrcommon.NewOCRWrapper(lggr.Named("OCRBootstrap"), d.ocr2Cfg.TraceLogging(), func(ctx context.Context, msg string) { | ||
logger.Sugared(lggr).ErrorIf(d.jobORM.RecordError(ctx, jb.ID, msg), "unable to record error") | ||
}) | ||
bootstrapNodeArgs := ocr.BootstrapperArgs{ | ||
BootstrapperFactory: d.peerWrapper.Peer2, | ||
ContractConfigTracker: configProvider.ContractConfigTracker(), | ||
Database: NewDB(d.ds, spec.ID, lggr), | ||
LocalConfig: lc, | ||
Logger: commonlogger.NewOCRWrapper(lggr.Named("OCRBootstrap"), d.ocr2Cfg.TraceLogging(), func(msg string) { | ||
logger.Sugared(lggr).ErrorIf(d.jobORM.RecordError(ctx, jb.ID, msg), "unable to record error") | ||
}), | ||
BootstrapperFactory: d.peerWrapper.Peer2, | ||
ContractConfigTracker: configProvider.ContractConfigTracker(), | ||
Database: NewDB(d.ds, spec.ID, lggr), | ||
LocalConfig: lc, | ||
Logger: ocrLogger, | ||
OffchainConfigDigester: configProvider.OffchainConfigDigester(), | ||
} | ||
lggr.Debugw("Launching new bootstrap node", "args", bootstrapNodeArgs) | ||
bootstrapper, err := ocr.NewBootstrapper(bootstrapNodeArgs) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "error calling NewBootstrapNode") | ||
} | ||
return []job.ServiceCtx{configProvider, job.NewServiceAdapter(bootstrapper)}, nil | ||
return []job.ServiceCtx{configProvider, ocrLogger, job.NewServiceAdapter(bootstrapper)}, nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is ocrLogger to be wired through in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The bootstrapper depends on it, but it doesn't understand that it needs to be closed, so it is not closed by the |
||
} | ||
|
||
// AfterJobCreated satisfies the job.Delegate interface. | ||
|
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.
For my own understanding: the benefit of this is that now consumers of ocrLogger will provide their own context to
saveError
rather than inheriting it from the delegate?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, it is tied to the lifecyle of the job now. Inheriting from the delegate method was the real problem. My first hack added a
StopChan
backedClose()
method to theDelegate
s instead. That works but ties it to the lifecyle of the delegate itself, which is not cancelled until shutdown. This new approach manages the lifecycle appropriately, at the cost of a little code duplication where we used to be able to share.