Add required diagnostic CLI options.
Change-Id: Ifeb4bc72802b25441caa3a64a83e6712b5f7869b
diff --git a/catapult/cmd/catapult/make_histogram.go b/catapult/cmd/catapult/make_histogram.go
index cbc677f..f5085d0 100644
--- a/catapult/cmd/catapult/make_histogram.go
+++ b/catapult/cmd/catapult/make_histogram.go
@@ -15,34 +15,92 @@
"fuchsia.googlesource.com/testing/catapult"
schema "fuchsia.googlesource.com/testing/perf/schema/v1"
"github.com/google/subcommands"
+ uuid "github.com/satori/go.uuid"
)
-type MakeHistogramCommand struct{}
+// sharedDiagnostics contains the values to use in the HistogramSet generated
+// by this subcommand.
+//
+// Catapult requires a specific set of shared diagnostics to be present on a
+// HistogramSet before uploading to the dashboard. Those required diagnostics
+// are:
+//
+// * bots - a GenericSet of strings containing bot hostnames
+// * benchmarks - a GenericSet of strings containing Telemetry
+// benchmark names.
+// * chromiumCommitPositions - a GenericSet of numbers containing
+// Chromium commit positions.
+// * masters - a GenericSet of strings containing buildbot master hostnames.
+//
+// There are other, optional diagnostics. Those may be added to this struct as
+// needed.
+//
+// Because the names of some diagnostics are not relevant to users of this tool
+// it is ok for this subcommand and this struct to use different names for any
+// of them so long as the names are mapped back to their counterparts when the
+// HistogramSet is constructed.
+//
+// To learn more about Catapult Diagnostics, see:
+// https://github.com/catapult-project/catapult/blob/master/docs/how-to-write-metrics.md
+type sharedDiagnostics struct {
+ // testSuite specifies the test suite that we are creating a
+ // HistogramSet for. e.g. "zircon" "ledger"
+ //
+ // Corresponds to the diagnostic "benchmarks".
+ testSuite string
+
+ // builder specifies the LUCI builder that ran these tests.
+ //
+ // Corresponds to the diagnostic "bots".
+ builder string
+
+ // bucket specifies the LUCI bucket containing the job that ran
+ // these tests.
+ //
+ // Corresponds to the diagnostic "masters".
+ bucket string
+
+ // TODO(kjharland): Add placeholder for chromiumCommitPositions
+ // once the catapult team confirms what we can use this field
+ // for.
+}
+
+type MakeHistogramCommand struct {
+ sharedDiagnostics sharedDiagnostics
+}
func (*MakeHistogramCommand) Name() string {
return "make_histogram"
}
func (*MakeHistogramCommand) Usage() string {
- return "make_histogram [input_file]"
+ return "make_histogram [options] input_file"
}
func (*MakeHistogramCommand) Synopsis() string {
return "Converts performance test output to a catapult HistogramSet"
}
-func (*MakeHistogramCommand) SetFlags(flags *flag.FlagSet) {}
+func (cmd *MakeHistogramCommand) SetFlags(flags *flag.FlagSet) {
+ flags.StringVar(&cmd.sharedDiagnostics.testSuite, "test-suite", "",
+ "Test suite corresponding to the input file")
+ flags.StringVar(&cmd.sharedDiagnostics.builder, "builder", "",
+ "LUCI builder that generated the test results")
+ flags.StringVar(&cmd.sharedDiagnostics.bucket, "bucket", "",
+ "Buildbucket bucket containing the job that ran the test")
+}
-// Execute Converts performance test output to a catapult HistogramSet
+// Execute converts performance test output to a catapult HistogramSet
// The histogram set created by this command does not contain the necessary
// diagnostics for sending to catapult. To add these diagnostics, use:
//
-// $ catapult add_diagnostics ....
+// $ catapult add_diagnostics ....
//
// See https://github.com/catapult-project/catapult/blob/master/docs/histogram-set-json-format.md
-func (*MakeHistogramCommand) Execute(ctx context.Context, f *flag.FlagSet, _ ...interface{}) subcommands.ExitStatus {
- var variants struct {
- Variants []schema.Variant `json:"variants"`
+func (cmd *MakeHistogramCommand) Execute(ctx context.Context, f *flag.FlagSet, _ ...interface{}) subcommands.ExitStatus {
+ if f.NArg() != 1 {
+ log.Println("missing input_file")
+ return subcommands.ExitFailure
}
inputFile := f.Arg(0)
@@ -52,13 +110,20 @@
return subcommands.ExitFailure
}
+ histogramSet := new(catapult.HistogramSet)
+
+ var variants struct {
+ Variants []schema.Variant `json:"variants"`
+ }
+
if err := json.Unmarshal(blob, &variants); err != nil {
log.Println(err)
return subcommands.ExitFailure
}
- histogramSet := catapult.HistogramSet{}
+ // Generate Histograms from test results.
histograms := catapult.ConvertVariantsToHistograms(variants.Variants)
+
// Track whether any single Histogram addition failed. We do this instead of
// exiting after a single error to expose as many errors as possible.
anyAddFailed := false
@@ -66,19 +131,41 @@
if err := histogramSet.AddHistogram(histogram); err != nil {
log.Println(err)
anyAddFailed = true
-
}
}
if anyAddFailed {
return subcommands.ExitFailure
}
- js, err := json.Marshal(&histogramSet)
+ // Add all shared diagnostics, mapping to the appropriate Catapult
+ // diagnostic names.
+ diagnosticInfoPairs := []struct {
+ Name string
+ Value string
+ }{
+ {"benchmarks", cmd.sharedDiagnostics.testSuite},
+ {"bots", cmd.sharedDiagnostics.builder},
+ {"masters", cmd.sharedDiagnostics.bucket},
+ // Required for uploading, even if value is just empty.
+ {"chromiumCommitPositions", ""},
+ }
+ for _, info := range diagnosticInfoPairs {
+ histogramSet.AddSharedDiagnostic(info.Name, &catapult.GenericSetDiagnostic{
+ Type: catapult.DiagnosticTypeGenericSet,
+ GUID: uuid.NewV4().String(),
+ Values: []string{info.Value},
+ })
+ }
+
+ // Serialize and log the new HistogramSet.
+ js, err := json.Marshal(histogramSet.ToJSON())
if err != nil {
log.Println(err)
return subcommands.ExitFailure
}
+ // TODO(kjharland): Add option to write to a file so this can be used
+ // from a recipe.
fmt.Println(string(js))
return subcommands.ExitSuccess
}
diff --git a/catapult/histogram_set.go b/catapult/histogram_set.go
index 1a6f2c6..2858be8 100644
--- a/catapult/histogram_set.go
+++ b/catapult/histogram_set.go
@@ -44,8 +44,11 @@
guid := diagnostic.GetGUID()
// Reference the diagnostic's GUID in each Histogram in this set.
- for _, histogram := range h.histograms {
- histogram.AddDiagnostic(name, guid)
+ //
+ // Use the loop index instead of the loop variable since this directly
+ // mutates the histogram.
+ for i := range h.histograms {
+ h.histograms[i].AddDiagnostic(name, guid)
}
}
diff --git a/catapult/histogram_set_test.go b/catapult/histogram_set_test.go
index aa93e5a..e324524 100644
--- a/catapult/histogram_set_test.go
+++ b/catapult/histogram_set_test.go
@@ -106,8 +106,10 @@
Diagnostics: make(map[string]string),
})
histogramSet.AddHistogram(catapult.Histogram{
- Name: "test_histogram_2",
- Diagnostics: make(map[string]string),
+ Name: "test_histogram_2",
+ // Intentionally leaving this Diagnostics map as nil to
+ // make sure the map still recieves a reference to the
+ // new Diagnostic.
})
histogramSet.AddSharedDiagnostic("test_diagnostic", diagnostic)
