feat: Instrument VoteExtension status [BLO-792] #41
Conversation
59c3678 to
b634d12
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #41 +/- ##
==========================================
+ Coverage 72.92% 73.41% +0.48%
==========================================
Files 117 110 -7
Lines 6235 5886 -349
==========================================
- Hits 4547 4321 -226
+ Misses 1387 1284 -103
+ Partials 301 281 -20 ☔ View full report in Codecov by Sentry. |
b634d12 to
de08686
Compare
| latency := time.Since(start) | ||
| h.logger.Info( | ||
| "extend vote handler", | ||
| "duration (seconds)", latency.Seconds(), | ||
| "err", err, | ||
| ) | ||
| h.metrics.ObserveABCIMethodLatency(servicemetrics.ExtendVote, latency) | ||
| slinkyabci.RecordLatencyAndStatus(h.metrics, latency, err, servicemetrics.ExtendVote) |
There was a problem hiding this comment.
Is there a reason you're introducing returned errors in this function? The SDK catches them and returns nil like we are currently doing.
Seems like this will change the behavior of the oracle from defaulting to an empty vote extension on failure to failing to add a vote extension at all. It looks like it will get swallowed by comet, so maybe it nets out to the same outcome in the end?
There was a problem hiding this comment.
yes, for each ABCI method, comet's behavior is to eventually panic on errors returned from the app. Hence, the baseapp's implementations in abci.go log and ignore errors from our handlers. Hence, I thought it made sense to keep our logic in line w/ our other handlers. For instance, Prepare / ProcessProposal return errors in execution to the base-app despite ultimately being swallowed
| func RecordLatencyAndStatus( | ||
| metrics servicemetrics.Metrics, latency time.Duration, err error, method servicemetrics.ABCIMethod, | ||
| ) { |
There was a problem hiding this comment.
Should this just be a method on servicemetrics.Metrics ?
5b98618 to
fd0ed24
Compare
fd0ed24 to
288e75b
Compare
No description provided.