Alpha support for metrics #55
Conversation
| } | ||
| } | ||
| var err error | ||
| nonblockingMetric.metricImpl, err = nonblockingMetricVec.metricVecImpl.GetMetricWith(nonblockingMetric.labels) |
There was a problem hiding this comment.
This can segfault if nbmw.metricsImpl is nil. Should we add a nil check, so it's backward compatible with PeerConfig structs that don't have Metrics field explicitly filled in?
There was a problem hiding this comment.
Yeah, we should def. do that. Note that this is just an alpha branch with the goal of aligning on the updated libocr interface and being able to do some very rough testing of whether things work.
There was a problem hiding this comment.
Do you refer to line 122?
If yes, do I understand correctly that the nil check you refer to is for the constructor?
I.e. function "NewNonblockingMetricsWrapper"
There was a problem hiding this comment.
Yes, line 122. I just mean adding:
if nbmw.metricsImpl == nil {
}
IIRC, when I left this comment I did a couple edits within the first minute or two... you might have seen an earlier version in an email that wasn't right.
There was a problem hiding this comment.
Oh, I may have misunderstood the question: I was actually thinking the check should go here, but yes putting in the constructor might work better? I'd need more context to have a strong opinion on that.
|
i have a WIP implementation. at minimum, it does not segfault during ocr tests and it implements the interfaces smartcontractkit/chainlink#8454 |
No description provided.