Skip to content

Commit

Permalink
Record inferred build label to log to scuba.
Browse files Browse the repository at this point in the history
Summary: The build label can now be inferred on the server-side like the tenant id. We should log this in the client side data as well.

Reviewed By: alisdair04

fbshipit-source-id: 9b837db
  • Loading branch information
shivanker authored and facebook-github-bot committed Jun 6, 2018
1 parent 27728f5 commit 54aa229
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ abstract class AbstractDistBuildClientStats {

abstract boolean buckClientError();

abstract String buildLabel();
abstract String userOrInferredBuildLabel();

abstract String minionType();

Expand Down
13 changes: 9 additions & 4 deletions src/com/facebook/buck/distributed/ClientStatsTracker.java
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,19 @@ public enum DistBuildClientStat {

private volatile Optional<String> buckClientErrorMessage = Optional.empty();

private final String buildLabel;
private volatile String userOrInferredBuildLabel;

private final String minionType;

public ClientStatsTracker(String buildLabel, String minionType) {
this.buildLabel = buildLabel;
public ClientStatsTracker(String userOrInferredBuildLabel, String minionType) {
this.userOrInferredBuildLabel = userOrInferredBuildLabel;
this.minionType = minionType;
}

public void setUserOrInferredBuildLabel(String userOrInferredBuildLabel) {
this.userOrInferredBuildLabel = userOrInferredBuildLabel;
}

@GuardedBy("this")
private void generateStatsPreconditionChecksNoException() {
// Unless there was an exception, we expect all the following fields to be present.
Expand Down Expand Up @@ -139,7 +144,7 @@ public synchronized DistBuildClientStats generateStats() {
.setStampedeId(stampedeId)
.setPerformedLocalBuild(performedLocalBuild)
.setBuckClientError(buckClientError)
.setBuildLabel(buildLabel)
.setUserOrInferredBuildLabel(userOrInferredBuildLabel)
.setMinionType(minionType);

builder.setDistributedBuildExitCode(distributedBuildExitCode);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public class PreBuildPhase {
private final BuildExecutorArgs buildExecutorArgs;
private final ImmutableSet<BuildTarget> topLevelTargets;
private final ActionAndTargetGraphs actionAndTargetGraphs;
private final String buildLabel;
private volatile String buildLabel;

public PreBuildPhase(
DistBuildService distBuildService,
Expand Down Expand Up @@ -119,6 +119,12 @@ public Pair<StampedeId, ListenableFuture<Void>> runPreDistBuildLocalStepsAsync(
buildId, buildMode, minionRequirements, repository, tenantId, buildTargets, buildLabel);
distBuildClientStats.stopTimer(CREATE_DISTRIBUTED_BUILD);

if (job.getBuildLabel() != null) {
// Override the build label with the server-side inferred label.
this.buildLabel = job.getBuildLabel();
distBuildClientStats.setUserOrInferredBuildLabel(buildLabel);
}

StampedeId stampedeId = job.getStampedeId();
eventBus.post(new DistBuildCreatedEvent(stampedeId));

Expand Down
12 changes: 11 additions & 1 deletion test/com/facebook/buck/distributed/ClientStatsTrackerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,16 @@ public void testGenerateSucceedsWhenAllStandardStatsSet() {
assertCommonStats(stats);
}

@Test
public void testBuildLabelOverrideWorks() {
ClientStatsTracker tracker = new ClientStatsTracker(BUILD_LABEL, MINION_TYPE);
String newLabel = "this-label-is-better";
tracker.setUserOrInferredBuildLabel(newLabel);
initializeCommonStats(tracker);
DistBuildClientStats stats = tracker.generateStats();
Assert.assertEquals(newLabel, stats.userOrInferredBuildLabel());
}

@Test(expected = java.lang.IllegalArgumentException.class)
public void testGenerateFailsWhenLocalBuildButMissingLocalBuildFields() {
ClientStatsTracker tracker = new ClientStatsTracker(BUILD_LABEL, MINION_TYPE);
Expand Down Expand Up @@ -123,7 +133,7 @@ public void testGeneratesPartialResultWhenErrorButRequiredFields() {
DISTRIBUTED_BUILD_EXIT_CODE, (long) stats.distributedBuildExitCode().getAsInt());
Assert.assertEquals(IS_LOCAL_FALLBACK_BUILD_ENABLED, stats.isLocalFallbackBuildEnabled().get());
Assert.assertEquals(BUCK_CLIENT_ERROR_MESSAGE, stats.buckClientErrorMessage().get());
Assert.assertEquals(BUILD_LABEL, stats.buildLabel());
Assert.assertEquals(BUILD_LABEL, stats.userOrInferredBuildLabel());
Assert.assertEquals(MINION_TYPE, stats.minionType());
Assert.assertTrue(stats.buckClientError());

Expand Down

0 comments on commit 54aa229

Please sign in to comment.