refactor: Add metrics collector #4407
Conversation
| for (auto& metric_hook : all.metric_output_hooks) { metric_hook(list_metrics); } | ||
|
|
||
| list_metrics.set_uint("total_log_calls", all.logger.get_log_count()); | ||
| auto additional_metrics = [&all](metric_sink& sink) -> void |
There was a problem hiding this comment.
I don't think we should add this extra stuff as a callback. That means if you were to call output_metrics twice it would add the callback twice.
I think we should leave the callbacks as is and add these extra metrics outside of being a callback.
There was a problem hiding this comment.
I agree this should be decoupled from output_metrics. I think it will still need to be added as a callback, since there is no way to interact with the metrics_sink directly in the new design. I added a callback in parse_args so it will not be called multiple times (eg in output_metrics or list_to_json_file)
|
what is the objective of metrics manager? to provide global information on whether metrics are enabled and to provide a way for collection of metrics to be called in lib mode? e.g. python can collect metrics now both by calling persist_metrics and collect_metrics? |
| using metrics_callback_fn = std::function<void(VW::metric_sink&)>; | ||
|
|
||
| bool are_metrics_enabled() const; | ||
| std::string get_filename() const; |
There was a problem hiding this comment.
get_filename should not be here if the class never uses it, it should probably still live in metrics, or are there plans for it to be used somehow in the future?
There was a problem hiding this comment.
reverted back to std::string filename = all.options->get_typed_option<std::string>("extra_metrics").value();
|
|
||
| namespace VW | ||
| { | ||
| class metrics_manager |
There was a problem hiding this comment.
if we remove the filename this could be renamed to metrics_collector and restrict the object from potentially becoming another all object in the future
There was a problem hiding this comment.
removed filename and updated to metics_collector
| private: | ||
| bool _are_metrics_enabled; | ||
| std::vector<metrics_callback_fn> _metrics_callbacks; | ||
| std::string _metrics_filename; |
There was a problem hiding this comment.
| std::string _metrics_filename; |
bassmang
changed the title
NOTE
Remember to remove metric_output_hooks from RLClientlib when updading VW and replace with metrics_manager register_callback_function