-
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
CCIP-4447 Promwrapper for OCR3 plugins and factories #15521
Conversation
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
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.
Nice! Can you add it to the LLO plugin in core/services/llo/delegate.go?
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.
LGTM, i had few minors
result, err := withObservedExecution(p, reports, func() ([]ocr3types.ReportPlus[RI], error) { | ||
return p.ReportingPlugin.Reports(ctx, seqNr, outcome) | ||
}) | ||
p.trackReports(reports, len(result)) |
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.
Does it mean that we are tracking report even when we have an error?
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.
reports should be nil or empty in that case
) | ||
|
||
var ( | ||
buckets = []float64{ |
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.
Are these buckets applicable for all chains? Solana might be significantly faster than Ethereum. I wonder if we should have buckets per chain.
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 CCIP it shouldn't be a big deal. Most of the work we do in OCR is reading from the database (ChainReader), doing some data transformation, reaching 3rd party APIs etc. There is still some communication with the chain involved but it shouldn't be that relevant. However, we might come back to that after seeing different values on non-evm chains
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.
To answer your question: yes, buckets are applicable for all chains
* PoC * Basic implementation * Basic implementation * Basic implementation * fixes * fixes * fixes * fixes * fixes * fixes * fixes * fixes * fixes * fixes * fixes
Implementing a very basic monitoring layer for OCR3 Reporting Plugins to support similar behavior to what we have for OCR2 https://github.com/smartcontractkit/chainlink/blob/develop/core/services/ocr2/plugins/promwrapper/plugin.go
Compared to the OCR2 implementation some labels like
chainType
,configDigest
andoracleID
were removed. This is done to reduce the cardinality of the metrics published to Prometheus. Intentionally, starting with a fundamental set of metrics which can be extended in the future if needed.These metrics will help us answer the following observability questions: