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 a facility for exposing metrics from the native engine #5808

Merged
merged 10 commits into from May 14, 2018

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

stuhood commented May 11, 2018

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 the BuildGraph requirement in #4769, we'd like to replace those metrics with forward compatible metrics computed in the native engine.

Solution

Split out a Session type which represents a scope for metrics capture. Currently the Session records 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 the Session object at the beginning (ie, when it is created) or "end" (when session.metrics() is called) of the Session.

The python objects representing the scheduler were renamed to Scheduler and SchedulerSession to represent this split. pantsd reuses a Scheduler for its entire lifetime, but creates a new SchedulerSession per client request, which is then used post fork as well.

Result

The affected_target_file_count metric was replaced with affected_file_count metric, based on the count of digests reachable from all of the roots in a session. Fixes #5736 and unblocks adding further scheduler metrics.

@stuhood stuhood requested review from illicitonion , kwlzn and dotordogh May 11, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks good! Thanks! :)

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

This comment has been minimized.

@illicitonion

illicitonion May 11, 2018

Contributor

Any reason not to now?

This comment has been minimized.

@stuhood

stuhood May 13, 2018

Member

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.

This comment has been minimized.

@stuhood

stuhood May 13, 2018

Member

...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(

This comment has been minimized.

@illicitonion

illicitonion May 11, 2018

Contributor

Does this need to be inside the lock?

This comment has been minimized.

@stuhood

stuhood May 13, 2018

Member

Yes, it should be. Wasn't before, but should be.

.into_iter()
.map(|(metric, value)| {
externs::store_tuple(&[
externs::store_bytes(&metric.bytes().collect::<Vec<_>>()),

This comment has been minimized.

@illicitonion

illicitonion May 11, 2018

Contributor

externs::store_bytes(metric.as_bytes()),

@@ -93,6 +117,21 @@ impl Scheduler {
})
}

///
/// TODO: Convert `extern::store_i32` to i64, and expand the range here.

This comment has been minimized.

@illicitonion

illicitonion May 11, 2018

Contributor

Any reason not to do this now?

This comment has been minimized.

@stuhood

stuhood May 13, 2018

Member

This one is easier. Will do here.

@@ -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.

This comment has been minimized.

@illicitonion

illicitonion May 11, 2018

Contributor

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

This comment has been minimized.

@baroquebobcat

baroquebobcat May 11, 2018

Contributor

I think you can xx this now that it is no longer used.

This comment has been minimized.

@stuhood

stuhood May 14, 2018

Member

Ah, yep. Thanks.

This comment has been minimized.

@stuhood

stuhood May 14, 2018

Member

...except I'm expecting green CI. Will nuke it in a followup.

@@ -15,6 +16,29 @@ use nodes::{NodeKey, Select};
use rule_graph;
use selectors;

pub struct Session {

This comment has been minimized.

@baroquebobcat

baroquebobcat May 11, 2018

Contributor

How do you feel about adding a doc comment to this? Is it too early?

This comment has been minimized.

@stuhood

stuhood May 14, 2018

Member

Added.

@stuhood stuhood force-pushed the pantsbuild:master branch from b6bb42d to 9e2fdb5 May 11, 2018

@stuhood stuhood force-pushed the twitter:stuhood/affected-files-in-rust branch from 202ec2b to 07a4e88 May 14, 2018

@stuhood stuhood merged commit 530a826 into pantsbuild:master May 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/affected-files-in-rust branch May 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment