Add a facility for exposing metrics from the native engine #5808
Conversation
src/python/pants/engine/scheduler.py
Outdated
| self.visualize_rule_graph_to_file(os.path.join(self.visualize_to_dir(), rule_graph_name)) | ||
|
|
||
| # Create the native Scheduler and Session. | ||
| # TODO: This `_tasks` reference could be a local variable, since it is not used |
There was a problem hiding this comment.
Any reason not to now?
There was a problem hiding this comment.
I was trying not to get sucked into sideprojects, since I generally make PRs that are way too large. But sure, why not!
Oh, but the backstory is that it was already factored this way, and I was just moving things around while avoiding refactoring them.
There was a problem hiding this comment.
...and actually, I think it might be a non-trivial sideproject. Going to leave it as is for now.
| with self.fork_lock: | ||
| self._graph_helper.warm_product_graph(spec_roots) | ||
| return self._graph_helper | ||
| target_roots = target_roots_calculator.create( |
There was a problem hiding this comment.
Does this need to be inside the lock?
There was a problem hiding this comment.
Yes, it should be. Wasn't before, but should be.
src/rust/engine/src/lib.rs
Outdated
| .into_iter() | ||
| .map(|(metric, value)| { | ||
| externs::store_tuple(&[ | ||
| externs::store_bytes(&metric.bytes().collect::<Vec<_>>()), |
There was a problem hiding this comment.
externs::store_bytes(metric.as_bytes()),
src/rust/engine/src/scheduler.rs
Outdated
| @@ -93,6 +117,21 @@ impl Scheduler { | |||
| }) | |||
| } | |||
|
|
|||
| /// | |||
| /// TODO: Convert `extern::store_i32` to i64, and expand the range here. | |||
There was a problem hiding this comment.
Any reason not to do this now?
There was a problem hiding this comment.
This one is easier. Will do here.
src/rust/engine/src/scheduler.rs
Outdated
| @@ -15,6 +16,29 @@ use nodes::{NodeKey, Select}; | |||
| use rule_graph; | |||
| use selectors; | |||
|
|
|||
| pub struct Session { | |||
| // The set of roots that have been requested within this session. | |||
There was a problem hiding this comment.
Can you add a comment explaining why we're using interior mutability here rather than &mut self on extend?
| @@ -15,6 +15,10 @@ def __init__(self): | |||
| self.affected_targets_size = 0 | |||
| self.affected_targets_file_count = 0 | |||
There was a problem hiding this comment.
I think you can xx this now that it is no longer used.
There was a problem hiding this comment.
...except I'm expecting green CI. Will nuke it in a followup.
src/rust/engine/src/scheduler.rs
Outdated
| @@ -15,6 +16,29 @@ use nodes::{NodeKey, Select}; | |||
| use rule_graph; | |||
| use selectors; | |||
|
|
|||
| pub struct Session { | |||
There was a problem hiding this comment.
How do you feel about adding a doc comment to this? Is it too early?
…en to actually gather metrics.
…edulerSession. Change the execution lifecycle to include explicitly creating a Session.
stuhood
force-pushed
the
stuhood/affected-files-in-rust
branch
from
202ec2b
to
07a4e88
Compare
Problem
As described in #5736, some of the metrics we've recently added introduce dependencies on having constructed a
BuildGraph. In order to unblock removing theBuildGraphrequirement in #4769, we'd like to replace those metrics with forward compatible metrics computed in the native engine.Solution
Split out a
Sessiontype which represents a scope for metrics capture. Currently theSessionrecords the root requests that were made during its lifetime, which allows it to later determine which nodes are reachable from those roots. In future, additional metrics could be captured in theSessionobject at the beginning (ie, when it is created) or "end" (whensession.metrics()is called) of theSession.The python objects representing the scheduler were renamed to
SchedulerandSchedulerSessionto represent this split.pantsdreuses aSchedulerfor its entire lifetime, but creates a newSchedulerSessionper client request, which is then used post fork as well.Result
The
affected_target_file_countmetric was replaced withaffected_file_countmetric, based on the count of digests reachable from all of the roots in a session. Fixes #5736 and unblocks adding further scheduler metrics.