Skip to content
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

Add statistics. Move client stats from Monitor to EventManager #2940

Merged
merged 9 commits into from
Feb 6, 2025

Conversation

Evian-Zhang
Copy link
Contributor

@Evian-Zhang Evian-Zhang commented Feb 5, 2025

Follow up discussions in #2927.

At top of the description, I want to emphasize that, although this PR involves many lines, I do not add, change or delete any user-aware features. Only code refactor and performance optimize.

Previously, the statistics shown by monitors, including exec_per_secs, etc., are called client stats. All client stats are maintained by monitor. This is not ideal, since monitors should be something to display the stats, not to maintain the stats. As a consequence, there are lots of boilerplates across different Monitor's implementations, and is hard for extension.

In this PR, there are following changes:

  • The client statistics structures are extracted and refactored to an individual module statistics. monitors module are only something related to display. Thanks for this change, the monitors/mod.rs finally looks fresh to me.
  • The client statistics values are moved from fields of Monitors to fields of EventManager. Since monitors are always invoked from eventmanager, users don't need change anything.
  • The user_monitor is renamed to user_stats, which is related to some statistics more flexible, and is maintained ad-hocly across the whole crate. Accordingly, the origin Aggregator, which is used for aggregate user stats, is merged to ClientStatsManager, a manager used to manage all statistics. As a result, all monitors can access the aggregated information easily.
  • The introspection_monitor is renamed to introspection_stats, and the perf-related structures are renamed accordingly.

Future possibility:

  • The monitors could be free of the base-pattern, as long as we figure out how to deal with the start_time.
  • There may be some way to get rid of the key.clone() of user stats across the crate. For now, it is hard since we have llmp.
  • We could write a unified structure for aggregated client stats. For now, different monitors may provide different metrics of aggregated client stats.

I hope this PR is kept as code refactoring, any feature modification could be added in followed PRs.

Feel free to put any questions :)

@Evian-Zhang
Copy link
Contributor Author

Evian-Zhang commented Feb 5, 2025

The GH Actions just fails due to network issues. It's not my fault aHahaha

It's fine now.

@tokatoka
Copy link
Member

tokatoka commented Feb 6, 2025

the rest looks good

@tokatoka tokatoka merged commit ab50afe into AFLplusplus:main Feb 6, 2025
106 checks passed
@tokatoka
Copy link
Member

tokatoka commented Feb 6, 2025

@Evian-Zhang

Since your PR involved a lot of changes, can you described briefly what you changed in MIGRATION.md?

The client statistics structures are extracted and refactored to an individual module statistics. monitors module are only something related to display. Thanks for this change, the monitors/mod.rs finally looks fresh to me.
The client statistics values are moved from fields of Monitors to fields of EventManager. Since monitors are always invoked from eventmanager, users don't need change anything.
The user_monitor is renamed to user_stats, which is related to some statistics more flexible, and is maintained ad-hocly across the whole crate. Accordingly, the origin Aggregator, which is used for aggregate user stats, is merged to ClientStatsManager, a manager used to manage all statistics. As a result, all monitors can access the aggregated information easily.
The introspection_monitor is renamed to introspection_stats, and the perf-related structures are renamed accordingly.

Also,

The monitors could be free of the base-pattern, as long as we figure out how to deal with the start_time.

Why start_time is a problem here? You can just add start_time to every class right? Every class has to impl Monitor so people won't forget adding that field

@Evian-Zhang
Copy link
Contributor Author

@tokatoka I'll update MIGRATIONS asap.

As for start_time, I don't know why it is maintained by each Monitors individually. Does it exist only for calculating and displaying some statistics? If so, I could move it to ClientStatsManager. I don't think it's a good idea to maintain a start time for each Monitors, since there may be chances where two monitors have different start time due to different initialization time.

@Evian-Zhang
Copy link
Contributor Author

I have updated MIGRATION in #2947 , and move start_time to ClientStatsManager in #2948 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants