-
Notifications
You must be signed in to change notification settings - Fork 3.7k
op-dispute-mon: node endpoint error metrics #17622
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
base: develop
Are you sure you want to change the base?
Conversation
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.
Looks good overall. There is some unused code that needs to be fixed.
an op-dispute-mon/main
file was accidentally checked in. It should be removed.
|
||
// HasMixedSafety returns true if some rollup endpoints reported the root as safe and others as unsafe | ||
// for this game. This indicates inconsistent safety assessment across the rollup node network. | ||
func (g EnrichedGameData) HasMixedSafety() bool { |
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.
Was this supposed to be recorded in a metric? I only see this function used by tests.
RollupEndpointUnsafeCount int | ||
|
||
// RollupEndpointDifferentOutputRoots tracks whether rollup endpoints returned different output roots for this game. | ||
RollupEndpointDifferentOutputRoots bool |
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.
There's no way to observe the value of this field. I don't see a log or a metric set when this is updated.
uniqueEndpointErrors := make(map[string]bool) | ||
|
||
for _, game := range games { | ||
if game.RollupEndpointErrors != nil { |
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.
if game.RollupEndpointErrors != nil { | |
if len(game.RollupEndpointErrors) != 0 { |
since the map is always initialized in enrichGame
. Also, it's idiomatic to use len
for emptiness because it also works when the map is nil.
"unique_endpoint_count", errorCount, | ||
"endpoints", getEndpointList(uniqueEndpointErrors)) | ||
} else { | ||
m.logger.Debug("No rollup node endpoint errors found") |
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.
nit: possibly noisy debug. Could use Trace
instead.
"total_error_count", totalErrors, | ||
"games_with_errors", countGamesWithErrors(games)) | ||
} else { | ||
m.logger.Debug("No rollup node endpoint errors found") |
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.
nit: Noisy debug. Consider using Trace
m.metrics.RecordMixedAvailabilityGames(count) | ||
|
||
if count > 0 { | ||
m.logger.Info("Mixed availability summary", "gamesWithMixedAvailability", count, "totalGames", len(games)) |
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.
worth logging this as a Warn
. It's far from ideal when this occurs.
This PR adds the following prometheus metrics to dispute-mon:
Reviewers might want to look at each commit separately, there's one for each metric.
Closes #11020