Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add content validation result metrics #796

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

njgheorghita
Copy link
Collaborator

@njgheorghita njgheorghita commented Jul 18, 2023

What was wrong?

Add metrics reporting whether or not received content is being successfully validated or not.

How was it fixed?

Added metric reporting logic. Update to grafana dashboard template will come in a following pr along with other dashboard changes.

To-Do

  • Add entry to the release notes (may forgo for trivial changes)
  • Clean up commit history

@njgheorghita njgheorghita marked this pull request as ready for review September 18, 2023 14:14
@njgheorghita njgheorghita force-pushed the validation-metrics branch 2 times, most recently from cf3864b to 47bde5e Compare September 18, 2023 20:22
Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validate_content is used so many places, it seems like it would be easy to miss one of them during a refactor or a new feature. Maybe we could pass the metric object into the metrics and increment it inside the validator once, instead of at every call site.

Or maybe a utility method that calls the validator, records the metric and returns the result, so the match calls can be replaced? Then we would generally make sure that no new direct calls to validate_content are created, though.

github tells me there are 9 call sites altogether, and I only see 4 places where the PR reports the validation result. So maybe there are still some missing?

format!(
"offers={}/{}, accepts={}/{}",
"offers={}/{}, accepts={}/{}, successful_validations={}/{}",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This log line gets quite long, so maybe we can bend towards shorter labels here, like:

Suggested change
"offers={}/{}, accepts={}/{}, successful_validations={}/{}",
"offers={}/{}, accepts={}/{}, validations={}/{}",

@njgheorghita
Copy link
Collaborator Author

@carver

github tells me there are 9 call sites altogether

If I'm not missing something, the other call sites are from within tests, in which case it doesn't seem worthwhile to manage metrics

validate_content is used so many places, it seems like it would be easy to miss one of them during a refactor or a new feature.

I agree, it's not ideal, and it's easy to miss something. I do see this as a concern.

But, this is how we handle all metrics inside overlay.rs / overlay_protocol.rs, just inserting them individually whenever a metric needs reporting. I'm uncertain about treating just the validation metric specially. And I'm not sure there's a better way to handle the metrics as a whole in this situation.

wrt the idea of passing metrics into the validate_content() method.

  1. It'll require some finagling/dead code in the tests.
  2. We'll have to import the OverlayMetrics arg in the trin-validation/trin-beacon/trin-state/trin-history crates.
  3. It feels messy / poor separation of responsibility to let only one of the metrics bleed into other parts of the codebase

Or maybe a utility method that calls the validator..

I went with a bit of a hybrid solution here. Added a utility method inside overlay.rs, but I opted to leave overlay_service.rs as-is. Mostly because that file is already quite complex, and I don't see much benefit of adding a utility function there.

I'm going to go ahead with the merge here since you gave this pr 👍 - but I'm happy to come back to this if you have any further thoughts.

@njgheorghita njgheorghita merged commit b43d206 into ethereum:master Sep 19, 2023
@njgheorghita njgheorghita deleted the validation-metrics branch September 19, 2023 17:01
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants