From 736af2211a49a182d1595bfd908dd0a84f93d279 Mon Sep 17 00:00:00 2001 From: Ben Segall Date: Thu, 10 Aug 2023 16:38:52 -0400 Subject: [PATCH] Split metrics uploading into separtate binary Bug: b/294945709 Test: Updated unit and integration tests Change-Id: I6e3dd4ed90696a51f9e014709e697409c38c8dc0 GitOrigin-RevId: 3ff0af7e2384671e2b796e418011133cdb0d0e7e --- api/stats/stats.proto | 4 + cfg/release/cipd/cipd-linux-csd.yaml | 1 + cfg/release/cipd/cipd-linux.yaml | 1 + cfg/release/cipd/cipd-mac-arm64-csd.yaml | 1 + cfg/release/cipd/cipd-mac-arm64.yaml | 1 + cfg/release/cipd/cipd-mac-csd.yaml | 1 + cfg/release/cipd/cipd-mac.yaml | 1 + cfg/release/cipd/cipd-windows-csd.yaml | 1 + cfg/release/cipd/cipd-windows.yaml | 1 + cmd/bootstrap/BUILD.bazel | 3 +- cmd/bootstrap/main.go | 122 +++++++--- cmd/metricsuploader/BUILD.bazel | 27 +++ cmd/metricsuploader/main.go | 116 +++++++++ cmd/reproxy/main.go | 2 +- internal/pkg/bigquery/BUILD.bazel | 2 +- internal/pkg/bigquery/bigquery.go | 7 +- internal/pkg/logger/logger.go | 15 +- internal/pkg/logger/logger_test.go | 85 ++++--- internal/pkg/monitoring/BUILD.bazel | 4 +- internal/pkg/monitoring/monitoring.go | 80 +++--- internal/pkg/monitoring/monitoring_test.go | 229 +++--------------- internal/pkg/reproxypid/BUILD.bazel | 11 +- internal/pkg/reproxypid/reproxypid.go | 4 +- internal/pkg/subprocess/BUILD.bazel | 6 +- .../{reproxypid => subprocess}/exists_unix.go | 6 +- .../exists_windows.go | 7 +- 26 files changed, 411 insertions(+), 327 deletions(-) create mode 100644 cmd/metricsuploader/BUILD.bazel create mode 100644 cmd/metricsuploader/main.go rename internal/pkg/{reproxypid => subprocess}/exists_unix.go (90%) rename internal/pkg/{reproxypid => subprocess}/exists_windows.go (87%) diff --git a/api/stats/stats.proto b/api/stats/stats.proto index bd162bb2..7a085ecb 100644 --- a/api/stats/stats.proto +++ b/api/stats/stats.proto @@ -49,6 +49,10 @@ message Stats { // action. 0 if there are no actions. double build_latency = 11; + // Whether FATAL log files were found in the log directory when reproxy was + // shutdown by bootstrap. + bool fatal_exit = 12; + reserved 3, 8; } diff --git a/cfg/release/cipd/cipd-linux-csd.yaml b/cfg/release/cipd/cipd-linux-csd.yaml index df04a0e6..2b30efb3 100644 --- a/cfg/release/cipd/cipd-linux-csd.yaml +++ b/cfg/release/cipd/cipd-linux-csd.yaml @@ -29,3 +29,4 @@ data: - file: rewrapper - file: reproxystatus - file: reclientreport + - file: metricsuploader diff --git a/cfg/release/cipd/cipd-linux.yaml b/cfg/release/cipd/cipd-linux.yaml index 2f32391d..423d2549 100644 --- a/cfg/release/cipd/cipd-linux.yaml +++ b/cfg/release/cipd/cipd-linux.yaml @@ -31,3 +31,4 @@ data: - file: remotetool - file: scandeps_server - file: scandeps_server.sym + - file: metricsuploader diff --git a/cfg/release/cipd/cipd-mac-arm64-csd.yaml b/cfg/release/cipd/cipd-mac-arm64-csd.yaml index ec2dc84a..c8d6b1a3 100644 --- a/cfg/release/cipd/cipd-mac-arm64-csd.yaml +++ b/cfg/release/cipd/cipd-mac-arm64-csd.yaml @@ -29,3 +29,4 @@ data: - file: reproxystatus - file: reclientreport - file: remotetool + - file: metricsuploader diff --git a/cfg/release/cipd/cipd-mac-arm64.yaml b/cfg/release/cipd/cipd-mac-arm64.yaml index 8920a558..ed14753b 100644 --- a/cfg/release/cipd/cipd-mac-arm64.yaml +++ b/cfg/release/cipd/cipd-mac-arm64.yaml @@ -31,3 +31,4 @@ data: - file: scandeps_server - file: scandeps_server.sym - file: remotetool + - file: metricsuploader diff --git a/cfg/release/cipd/cipd-mac-csd.yaml b/cfg/release/cipd/cipd-mac-csd.yaml index be9d0136..7b8ac0c5 100644 --- a/cfg/release/cipd/cipd-mac-csd.yaml +++ b/cfg/release/cipd/cipd-mac-csd.yaml @@ -29,3 +29,4 @@ data: - file: reproxystatus - file: reclientreport - file: remotetool + - file: metricsuploader diff --git a/cfg/release/cipd/cipd-mac.yaml b/cfg/release/cipd/cipd-mac.yaml index 16c36e8f..a27311eb 100644 --- a/cfg/release/cipd/cipd-mac.yaml +++ b/cfg/release/cipd/cipd-mac.yaml @@ -31,3 +31,4 @@ data: - file: remotetool - file: scandeps_server - file: scandeps_server.sym + - file: metricsuploader diff --git a/cfg/release/cipd/cipd-windows-csd.yaml b/cfg/release/cipd/cipd-windows-csd.yaml index 0e6aff50..28a5c8aa 100644 --- a/cfg/release/cipd/cipd-windows-csd.yaml +++ b/cfg/release/cipd/cipd-windows-csd.yaml @@ -28,3 +28,4 @@ data: - file: rewrapper.exe - file: reproxystatus.exe - file: reclientreport.exe + - file: metricsuploader.exe diff --git a/cfg/release/cipd/cipd-windows.yaml b/cfg/release/cipd/cipd-windows.yaml index 7e7eb31e..501d5b5d 100644 --- a/cfg/release/cipd/cipd-windows.yaml +++ b/cfg/release/cipd/cipd-windows.yaml @@ -31,3 +31,4 @@ data: - file: scandeps_server.exe - file: scandeps_server.pdb - file: scandeps_server.sym + - file: metricsuploader.exe diff --git a/cmd/bootstrap/BUILD.bazel b/cmd/bootstrap/BUILD.bazel index b8d27ab9..023692a1 100644 --- a/cmd/bootstrap/BUILD.bazel +++ b/cmd/bootstrap/BUILD.bazel @@ -9,10 +9,8 @@ go_library( "//api/log", "//api/stats", "//internal/pkg/auth", - "//internal/pkg/bigquery", "//internal/pkg/bootstrap", "//internal/pkg/logger", - "//internal/pkg/monitoring", "//internal/pkg/pathtranslator", "//internal/pkg/rbeflag", "//internal/pkg/stats", @@ -21,6 +19,7 @@ go_library( "@com_github_bazelbuild_remote_apis_sdks//go/pkg/command", "@com_github_bazelbuild_remote_apis_sdks//go/pkg/moreflag", "@com_github_golang_glog//:go_default_library", + "@org_golang_google_protobuf//proto", ], ) diff --git a/cmd/bootstrap/main.go b/cmd/bootstrap/main.go index 137cd155..b3d70caf 100644 --- a/cmd/bootstrap/main.go +++ b/cmd/bootstrap/main.go @@ -28,10 +28,8 @@ import ( lpb "team/foundry-x/re-client/api/log" spb "team/foundry-x/re-client/api/stats" "team/foundry-x/re-client/internal/pkg/auth" - "team/foundry-x/re-client/internal/pkg/bigquery" "team/foundry-x/re-client/internal/pkg/bootstrap" "team/foundry-x/re-client/internal/pkg/logger" - "team/foundry-x/re-client/internal/pkg/monitoring" "team/foundry-x/re-client/internal/pkg/pathtranslator" "team/foundry-x/re-client/internal/pkg/rbeflag" "team/foundry-x/re-client/internal/pkg/stats" @@ -42,6 +40,7 @@ import ( "github.com/bazelbuild/remote-apis-sdks/go/pkg/command" "github.com/bazelbuild/remote-apis-sdks/go/pkg/moreflag" log "github.com/golang/glog" + "google.golang.org/protobuf/proto" ) // bootstrapStart saves the start time of the bootstrap binary. @@ -50,7 +49,6 @@ var bootstrapStart = time.Now() var ( homeDir, _ = os.UserHomeDir() - labels = make(map[string]string) gcertErrMsg = fmt.Sprintf("\nTry restarting the build after running %q\n", "gcert") gcloudErrMsg = fmt.Sprintf("\nTry restarting the build after running %q\n", "gcloud auth login") logDir = os.TempDir() @@ -68,9 +66,6 @@ var ( fastLogCollection = flag.Bool("fast_log_collection", false, "Enable optimized log aggregation pipeline. Does not work for multileg builds") asyncReproxyShutdown = flag.Bool("async_reproxy_termination", false, "Allows reproxy to finish shutdown asyncronously. Only applicable with fast_log_collection=true") metricsProject = flag.String("metrics_project", "", "If set, action and build metrics are exported to Cloud Monitoring in the specified GCP project") - metricsPrefix = flag.String("metrics_prefix", "", "Prefix of metrics exported to Cloud Monitoring") - metricsNamespace = flag.String("metrics_namespace", "", "Namespace of metrics exported to Cloud Monitoring (e.g. RBE project)") - metricsTable = flag.String("metrics_table", "", "Resource specifier of the BigQuery table to upload the contents of rbe_metrics.pb to. If the project is not provided in the specifier metrics_project will be used.") outputDir = flag.String("output_dir", os.TempDir(), "The location to which stats should be written.") useADC = flag.Bool(auth.UseAppDefaultCredsFlag, false, "Indicates whether to use application default credentials for authentication") useGCE = flag.Bool(auth.UseGCECredsFlag, false, "Indicates whether to use GCE VM credentials for authentication") @@ -79,12 +74,12 @@ var ( credFile = flag.String(auth.CredentialFileFlag, "", "The name of a file that contains service account credentials to use when calling remote execution. Used only if --use_application_default_credentials and --use_gce_credentials are false.") remoteDisabled = flag.Bool("remote_disabled", false, "Whether to disable all remote operations and run all actions locally.") cacheDir = flag.String("cache_dir", "", "Directory from which to load the cache files at startup and update at shutdown.") + metricsUploader = flag.String("metrics_uploader", defaultMetricsUploader(), "Path to the metrics uploader binary.") ) func main() { defer log.Flush() flag.Var((*moreflag.StringListValue)(&proxyLogDir), "proxy_log_dir", "If provided, the directory path to a proxy log file of executed records.") - flag.Var((*moreflag.StringMapValue)(&labels), "metrics_labels", "Comma-separated key value pairs in the form key=value. This is used to add arbitrary labels to exported metrics.") rbeflag.Parse() version.PrintAndExitOnVersionFlag(true) @@ -136,45 +131,49 @@ func main() { From: bootstrapStart, To: time.Now(), }) - if *metricsProject != "" { - start := time.Now() - var e *monitoring.Exporter - e, err = newExporter(creds) - if err != nil { - log.Warningf("Failed to initialize cloud monitoring: %v", err) - } else { - e.ExportBuildMetrics(context.Background(), s, spi.EventTimes[logger.EventBootstrapShutdown]) - defer e.Close() - } - spi.EventTimes[logger.EventPostBuildMetricsUpload] = command.TimeIntervalToProto(&command.TimeInterval{From: start, To: time.Now()}) - spi.Metrics[logger.EventPostBuildMetricsUpload] = &lpb.Metric{Value: &lpb.Metric_BoolValue{err == nil}} - } s.ProxyInfo = append(s.ProxyInfo, spi) + s.FatalExit = fatalLogsExist(logDir) log.Infof("Writing stats to %v", *outputDir) if err := stats.WriteStats(s, *outputDir); err != nil { log.Errorf("WriteStats(%s) failed: %v", *outputDir, err) } else { log.Infof("Stats dumped successfully.") } - if *metricsTable != "" { - inserter, cleanup, err := bigquery.NewInserter(context.Background(), *metricsTable, *metricsProject, creds) - if err != nil { - log.Warningf("Error creating a bigquery client: %v", err) - return + if *metricsProject == "" { + return + } + + tempRbeMetricsFilePath, err := createTempRbeMetricsFile(s) + if err != nil { + log.Errorf("Unable to make temp rbe_metrics.pb for upload: %v", err) + return + } + + uploaderArgs := []string{"--rbe_metrics_path=" + tempRbeMetricsFilePath} + if cfg := flag.Lookup("cfg"); cfg != nil { + if cfg.Value.String() != "" { + uploaderArgs = append(uploaderArgs, "--cfg="+cfg.Value.String()) } - defer cleanup() - err = inserter.Put(context.Background(), &stats.ProtoSaver{s}) - if err != nil { - log.Warningf("Error uploading stats to bigquery: %v", err) - return + } + if ts := creds.TokenSource(); ts != nil { + if t, err := ts.Token(); err == nil { + uploaderArgs = append(uploaderArgs, "--oauth_token="+t.AccessToken) } } - log.Infof("Stats uploaded successfully.") + log.V(2).Infof("Running %v %v", *metricsUploader, uploaderArgs) + + uploaderCmd := exec.Command(*metricsUploader, uploaderArgs...) + err = uploaderCmd.Start() + if err != nil { + log.Warningf("Failed to start metrics uploader with command line %v %v: %v", *metricsUploader, uploaderArgs, err) + } + log.Infof("Stats uploader started successfully") + log.V(2).Infof("Stats uploader pid: %d", uploaderCmd.Process.Pid) return } - monitoring.CleanLogDir(logDir) + cleanFatalLogs(logDir) args := []string{} if cfg := flag.Lookup("cfg"); cfg != nil { @@ -201,6 +200,58 @@ func main() { os.Exit(exitCode) } +var failureFiles = []string{"reproxy.FATAL", "bootstrap.FATAL", "rewrapper.FATAL", "reproxy.exe.FATAL", "bootstrap.exe.FATAL", "rewrapper.exe.FATAL"} + +// cleanLogDir removes stray log files which may cause confusion when bootstrap starts +func cleanFatalLogs(logDir string) { + for _, f := range failureFiles { + fp := filepath.Join(logDir, f) + if err := os.Remove(fp); err != nil && !os.IsNotExist(err) { + log.Errorf("Failed to remove %v: %v", fp, err) + } + } +} + +// fatalLogsExist returns true if any *.FATAL log file exists in +func fatalLogsExist(logDir string) bool { + for _, f := range failureFiles { + s, err := os.Stat(filepath.Join(logDir, f)) + if err != nil { + continue + } + if s.Size() > 0 { + return true + } + } + return false +} + +func createTempRbeMetricsFile(s *spb.Stats) (string, error) { + temp, err := os.CreateTemp("", "rbe_metrics_*.pb") + if err != nil { + return "", err + } + defer temp.Close() + blob, err := proto.Marshal(s) + if err != nil { + return "", err + } + _, err = temp.Write(blob) + if err != nil { + return "", err + } + return temp.Name(), nil +} + +func defaultMetricsUploader() string { + metricsUploader, err := pathtranslator.BinaryRelToAbs("metricsuploader") + if err != nil { + log.Warningf("Did not find `metricsuploader` binary in the same directory as `bootstrap`: %v", err) + return "" + } + return metricsUploader +} + func shutdownReproxy() (*spb.Stats, error) { if *asyncReproxyShutdown { // On shutdown we may not want to wait for deps cache to finish writing @@ -238,13 +289,6 @@ func bootstrapReproxy(args []string, startTime time.Time) (string, int) { return "Proxy started successfully.", 0 } -func newExporter(creds *auth.Credentials) (*monitoring.Exporter, error) { - if err := monitoring.SetupViews(labels); err != nil { - return nil, err - } - return monitoring.NewExporter(context.Background(), *metricsProject, *metricsPrefix, *metricsNamespace, *remoteDisabled, logDir, creds) -} - func credsFilePath() (string, error) { dir := os.TempDir() if *cacheDir != "" { diff --git a/cmd/metricsuploader/BUILD.bazel b/cmd/metricsuploader/BUILD.bazel new file mode 100644 index 00000000..9f9cdd73 --- /dev/null +++ b/cmd/metricsuploader/BUILD.bazel @@ -0,0 +1,27 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library") + +go_library( + name = "metricsuploader_lib", + srcs = ["main.go"], + importpath = "team/foundry-x/re-client/cmd/metricsuploader", + visibility = ["//visibility:private"], + deps = [ + "//api/stats", + "//internal/pkg/bigquery", + "//internal/pkg/monitoring", + "//internal/pkg/rbeflag", + "//internal/pkg/stats", + "//pkg/version", + "@com_github_bazelbuild_remote_apis_sdks//go/pkg/moreflag", + "@com_github_golang_glog//:go_default_library", + "@org_golang_google_grpc//credentials/oauth", + "@org_golang_google_protobuf//proto", + "@org_golang_x_oauth2//:oauth2", + ], +) + +go_binary( + name = "metricsuploader", + embed = [":metricsuploader_lib"], + visibility = ["//visibility:public"], +) diff --git a/cmd/metricsuploader/main.go b/cmd/metricsuploader/main.go new file mode 100644 index 00000000..ed3ce250 --- /dev/null +++ b/cmd/metricsuploader/main.go @@ -0,0 +1,116 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package main uploads reproxy build level metrics to cloudmonitoring and bigquery. +package main + +import ( + "context" + "flag" + "fmt" + "os" + + "team/foundry-x/re-client/internal/pkg/bigquery" + "team/foundry-x/re-client/internal/pkg/monitoring" + "team/foundry-x/re-client/internal/pkg/rbeflag" + "team/foundry-x/re-client/internal/pkg/stats" + "team/foundry-x/re-client/pkg/version" + + "github.com/bazelbuild/remote-apis-sdks/go/pkg/moreflag" + log "github.com/golang/glog" + "golang.org/x/oauth2" + "google.golang.org/grpc/credentials/oauth" + "google.golang.org/protobuf/proto" + + spb "team/foundry-x/re-client/api/stats" +) + +var ( + labels = make(map[string]string) + metricsProject = flag.String("metrics_project", "", "If set, action and build metrics are exported to Cloud Monitoring in the specified GCP project.") + metricsPrefix = flag.String("metrics_prefix", "", "Prefix of metrics exported to Cloud Monitoring.") + metricsNamespace = flag.String("metrics_namespace", "", "Namespace of metrics exported to Cloud Monitoring (e.g. RBE project).") + metricsTable = flag.String("metrics_table", "", "Resource specifier of the BigQuery table to upload the contents of rbe_metrics.pb to. If the project is not provided in the specifier metrics_project will be used.") + rbeMetricsPb = flag.String("rbe_metrics_path", "", "Path to rbe_metrics.pb that will be uploaded to bigquery and parsed for cloud monitoring build level metrics.") + oauthToken = flag.String("oauth_token", "", "Token to use when authenticating with GCP.") +) + +func main() { + defer log.Flush() + flag.Var((*moreflag.StringMapValue)(&labels), "metrics_labels", "Comma-separated key value pairs in the form key=value. This is used to add arbitrary labels to exported metrics.") + rbeflag.Parse() + version.PrintAndExitOnVersionFlag(true) + + if *metricsProject == "" { + log.Fatalf("--metrics_project is required.") + } + s := &spb.Stats{} + if rbeMetricsBytes, err := os.ReadFile(*rbeMetricsPb); err != nil { + log.Fatalf("Error reading rbe_metrics.pb file: %v", err) + } else if err := proto.Unmarshal(rbeMetricsBytes, s); err != nil { + log.Fatalf("Failed to parse rbe_metrics.pb: %v", err) + } + err := os.Remove(*rbeMetricsPb) + if err != nil { + log.Errorf("Failed to delete rbe_metrics.pb: %v", err) + } + + var ts *oauth.TokenSource + if *oauthToken != "" { + ts = &oauth.TokenSource{ + TokenSource: oauth2.StaticTokenSource(&oauth2.Token{AccessToken: *oauthToken}), + } + } + + if err := uploadToCloudMonitoring(s, ts); err != nil { + log.Errorf("Error uploading to cloud monitoring: %v", err) + } + + if *metricsTable != "" { + if err := uploadToBigQuery(s, ts); err != nil { + log.Errorf("Error uploading to bigquery: %v", err) + } + } +} + +func uploadToCloudMonitoring(s *spb.Stats, ts *oauth.TokenSource) error { + if err := monitoring.SetupViews(labels); err != nil { + return fmt.Errorf("failed to initialize cloud monitoring views: %w", err) + } + e, err := monitoring.NewExporter(context.Background(), *metricsProject, *metricsPrefix, *metricsNamespace, ts) + if err != nil { + return fmt.Errorf("failed to initialize cloud monitoring exporter: %w", err) + } + defer e.Close() + e.ExportBuildMetrics(context.Background(), s) + return nil +} + +func uploadToBigQuery(s *spb.Stats, ts *oauth.TokenSource) error { + inserter, cleanup, err := bigquery.NewInserter(context.Background(), *metricsTable, *metricsProject, ts) + if err != nil { + return fmt.Errorf("error creating a bigquery client: %w", err) + } + defer func() { + err := cleanup() + if err != nil { + log.Errorf("Error cleaning up bigquery client: %v", err) + } + }() + err = inserter.Put(context.Background(), &stats.ProtoSaver{Stats: s}) + if err != nil { + return fmt.Errorf("error uploading stats to bigquery: %w", err) + } + return nil +} diff --git a/cmd/reproxy/main.go b/cmd/reproxy/main.go index 04e53147..97c09091 100644 --- a/cmd/reproxy/main.go +++ b/cmd/reproxy/main.go @@ -476,7 +476,7 @@ func newExporter(creds *auth.Credentials) (*monitoring.Exporter, error) { if err := monitoring.SetupViews(labels); err != nil { return nil, err } - return monitoring.NewExporter(context.Background(), *metricsProject, *metricsPrefix, *metricsNamespace, *remoteDisabled, getLogDir(), creds) + return monitoring.NewExporter(context.Background(), *metricsProject, *metricsPrefix, *metricsNamespace, creds.TokenSource()) } func getLogDir() string { diff --git a/internal/pkg/bigquery/BUILD.bazel b/internal/pkg/bigquery/BUILD.bazel index f2177c35..48da7cff 100644 --- a/internal/pkg/bigquery/BUILD.bazel +++ b/internal/pkg/bigquery/BUILD.bazel @@ -6,9 +6,9 @@ go_library( importpath = "team/foundry-x/re-client/internal/pkg/bigquery", visibility = ["//:__subpackages__"], deps = [ - "//internal/pkg/auth", "@com_google_cloud_go_bigquery//:bigquery", "@org_golang_google_api//option", + "@org_golang_google_grpc//credentials/oauth", ], ) diff --git a/internal/pkg/bigquery/bigquery.go b/internal/pkg/bigquery/bigquery.go index b5d83cf4..7abefa30 100644 --- a/internal/pkg/bigquery/bigquery.go +++ b/internal/pkg/bigquery/bigquery.go @@ -19,10 +19,9 @@ import ( "fmt" "strings" - "team/foundry-x/re-client/internal/pkg/auth" - "cloud.google.com/go/bigquery" "google.golang.org/api/option" + "google.golang.org/grpc/credentials/oauth" ) // parseResourceSpec parses spec as per the bq command-line tool format described @@ -50,14 +49,14 @@ func parseResourceSpec(spec, defaultProject string) (project, dataset, table str // NewInserter creates a an inserter for the table specified by the given resourceSpec in // the form :. or .
-func NewInserter(ctx context.Context, resourceSpec, defaultProject string, creds *auth.Credentials) (inserter *bigquery.Inserter, cleanup func() error, err error) { +func NewInserter(ctx context.Context, resourceSpec, defaultProject string, ts *oauth.TokenSource) (inserter *bigquery.Inserter, cleanup func() error, err error) { project, dataset, table, err := parseResourceSpec(resourceSpec, defaultProject) cleanup = func() error { return nil } if err != nil { return nil, cleanup, err } var opts []option.ClientOption - if ts := creds.TokenSource(); ts != nil { + if ts != nil { opts = append(opts, option.WithTokenSource(ts)) } client, err := bigquery.NewClient(ctx, project, opts...) diff --git a/internal/pkg/logger/logger.go b/internal/pkg/logger/logger.go index 45c05b8f..e6b6abb8 100644 --- a/internal/pkg/logger/logger.go +++ b/internal/pkg/logger/logger.go @@ -23,6 +23,7 @@ import ( "os" "path/filepath" "regexp" + "strconv" "strings" "sync" "time" @@ -145,7 +146,7 @@ type statCollector interface { } // ExportActionMetricsFunc is the type of "team/foundry-x/re-client/internal/pkg/monitoring".Exporter.ExportActionMetrics -type ExportActionMetricsFunc func(ctx context.Context, lr *lpb.LogRecord) +type ExportActionMetricsFunc func(ctx context.Context, lr *lpb.LogRecord, remoteDisabled bool) // Logger logs Records asynchronously into a file. type Logger struct { @@ -155,6 +156,7 @@ type Logger struct { recsFile *os.File infoFile *os.File info *lpb.ProxyInfo + remoteDisabled bool stats statCollector mi *ignoremismatch.MismatchIgnorer exportActionMetrics ExportActionMetricsFunc @@ -183,7 +185,8 @@ func (s *startActionEvent) apply(l *Logger) { } type endActionEvent struct { - lr *LogRecord + lr *LogRecord + remoteDisabled bool } func (e *endActionEvent) apply(l *Logger) { @@ -191,7 +194,7 @@ func (e *endActionEvent) apply(l *Logger) { return } if l.exportActionMetrics != nil { - l.exportActionMetrics(context.Background(), e.lr.LogRecord) + l.exportActionMetrics(context.Background(), e.lr.LogRecord, l.remoteDisabled) } // Process any mismatches to be ignored for this log record. l.mi.ProcessLogRecord(e.lr.LogRecord) @@ -477,6 +480,12 @@ func (l *Logger) AddFlagStringToProxyInfo(key string, value string) { l.mu.Lock() defer l.mu.Unlock() l.info.Flags[key] = value + if key == "remote_disabled" { + v, err := strconv.ParseBool(value) + if err == nil { + l.remoteDisabled = v + } + } } // AddFlags will add all reproxy flags to the ProxyInfo object. diff --git a/internal/pkg/logger/logger_test.go b/internal/pkg/logger/logger_test.go index dec52bd9..51fdb6e7 100644 --- a/internal/pkg/logger/logger_test.go +++ b/internal/pkg/logger/logger_test.go @@ -570,35 +570,37 @@ func TestStatManagement(t *testing.T) { } } -// TestExportMetric stests if Logger is calling the correct metrics handling +// TestExportMetric tests if Logger is calling the correct metrics handling // functions when appropriate. This uses a stub struct for Exporter. func TestExportMetrics(t *testing.T) { - recs := []*lpb.LogRecord{ - &lpb.LogRecord{ - Command: &cpb.Command{ - Identifiers: &cpb.Identifiers{ - CommandId: "a", - InvocationId: "b", - ToolName: "c", - }, - Args: []string{"a", "b", "c"}, - ExecRoot: "/exec/root", - Input: &cpb.InputSpec{ - Inputs: []string{"foo.h", "bar.h"}, - EnvironmentVariables: map[string]string{ - "k": "v", - "k1": "v1", + recs := []ExportActionMetricsCall{ + { + Rec: &lpb.LogRecord{ + Command: &cpb.Command{ + Identifiers: &cpb.Identifiers{ + CommandId: "a", + InvocationId: "b", + ToolName: "c", + }, + Args: []string{"a", "b", "c"}, + ExecRoot: "/exec/root", + Input: &cpb.InputSpec{ + Inputs: []string{"foo.h", "bar.h"}, + EnvironmentVariables: map[string]string{ + "k": "v", + "k1": "v1", + }, + }, + Output: &cpb.OutputSpec{ + OutputFiles: []string{"a/b/out"}, }, }, - Output: &cpb.OutputSpec{ - OutputFiles: []string{"a/b/out"}, + Result: &cpb.CommandResult{ + Status: cpb.CommandResultStatus_CACHE_HIT, + ExitCode: 42, + Msg: "message", }, }, - Result: &cpb.CommandResult{ - Status: cpb.CommandResultStatus_CACHE_HIT, - ExitCode: 42, - Msg: "message", - }, }, } execRoot := t.TempDir() @@ -610,7 +612,7 @@ func TestExportMetrics(t *testing.T) { } for _, lr := range recs { r := logger.LogActionStart() - r.LogRecord = lr + r.LogRecord = lr.Rec logger.Log(r) } logger.CloseAndAggregate() @@ -618,18 +620,40 @@ func TestExportMetrics(t *testing.T) { // Test valid exportActionMetrics function. e := &stubExporter{} logger, err = New(TextFormat, execRoot, "testScanner", &stubStats{}, nil, e.ExportActionMetrics) + logger.remoteDisabled = false if err != nil { t.Errorf("Failed to create new Logger: %v", err) } for _, lr := range recs { r := logger.LogActionStart() - r.LogRecord = lr + r.LogRecord = lr.Rec logger.Log(r) } logger.CloseAndAggregate() if diff := cmp.Diff(recs, e.exportedRecs, protocmp.Transform()); diff != "" { t.Errorf("Log records sent to exporter returned diff in result: (-want +got)\n%s", diff) } + + // Test valid exportActionMetrics function with remoteDisabled=true. + e = &stubExporter{} + logger, err = New(TextFormat, execRoot, "testScanner", &stubStats{}, nil, e.ExportActionMetrics) + logger.remoteDisabled = true + if err != nil { + t.Errorf("Failed to create new Logger: %v", err) + } + var recsRemoteDisabled []ExportActionMetricsCall + for _, rec := range recs { + recsRemoteDisabled = append(recsRemoteDisabled, ExportActionMetricsCall{Rec: rec.Rec, RemoteDisabled: true}) + } + for _, lr := range recsRemoteDisabled { + r := logger.LogActionStart() + r.LogRecord = lr.Rec + logger.Log(r) + } + logger.CloseAndAggregate() + if diff := cmp.Diff(recsRemoteDisabled, e.exportedRecs, protocmp.Transform()); diff != "" { + t.Errorf("Log records sent to exporter returned diff in result: (-want +got)\n%s", diff) + } } type stubStats struct { @@ -650,10 +674,15 @@ func (s *stubStats) ToProto() *spb.Stats { return s.proto } +type ExportActionMetricsCall struct { + Rec *lpb.LogRecord + RemoteDisabled bool +} + type stubExporter struct { - exportedRecs []*lpb.LogRecord + exportedRecs []ExportActionMetricsCall } -func (e *stubExporter) ExportActionMetrics(ctx context.Context, rec *lpb.LogRecord) { - e.exportedRecs = append(e.exportedRecs, rec) +func (e *stubExporter) ExportActionMetrics(ctx context.Context, rec *lpb.LogRecord, remoteDisabled bool) { + e.exportedRecs = append(e.exportedRecs, ExportActionMetricsCall{Rec: rec, RemoteDisabled: remoteDisabled}) } diff --git a/internal/pkg/monitoring/BUILD.bazel b/internal/pkg/monitoring/BUILD.bazel index 22b35ef4..2ec26332 100644 --- a/internal/pkg/monitoring/BUILD.bazel +++ b/internal/pkg/monitoring/BUILD.bazel @@ -8,11 +8,9 @@ go_library( deps = [ "//api/log", "//api/stats", - "//internal/pkg/auth", "//internal/pkg/labels", "//internal/pkg/logger", "//pkg/version", - "@com_github_bazelbuild_remote_apis_sdks//go/api/command", "@com_github_bazelbuild_remote_apis_sdks//go/pkg/command", "@com_github_golang_glog//:go_default_library", "@com_github_google_uuid//:uuid", @@ -22,6 +20,7 @@ go_library( "@io_opencensus_go_contrib_exporter_stackdriver//:stackdriver", "@org_golang_google_api//option", "@org_golang_google_grpc//codes", + "@org_golang_google_grpc//credentials/oauth", "@org_golang_google_grpc//status", ], ) @@ -32,7 +31,6 @@ go_test( embed = [":monitoring"], deps = [ "//api/log", - "//internal/pkg/auth", "//internal/pkg/logger", "//internal/pkg/stats", "//pkg/version", diff --git a/internal/pkg/monitoring/monitoring.go b/internal/pkg/monitoring/monitoring.go index 240341b9..4d7683a6 100644 --- a/internal/pkg/monitoring/monitoring.go +++ b/internal/pkg/monitoring/monitoring.go @@ -26,10 +26,8 @@ import ( "sync" "time" - cpb "github.com/bazelbuild/remote-apis-sdks/go/api/command" lpb "team/foundry-x/re-client/api/log" spb "team/foundry-x/re-client/api/stats" - "team/foundry-x/re-client/internal/pkg/auth" "team/foundry-x/re-client/internal/pkg/labels" "team/foundry-x/re-client/internal/pkg/logger" "team/foundry-x/re-client/pkg/version" @@ -45,6 +43,7 @@ import ( "google.golang.org/api/option" "google.golang.org/grpc/codes" + "google.golang.org/grpc/credentials/oauth" "google.golang.org/grpc/status" ) @@ -96,26 +95,20 @@ type Exporter struct { prefix string // MetricNamespace is the namespace of the exported metrics. namespace string - // remoteDisabled indicates if this build ran without rbe. - remoteDisabled bool - // logDir is the directory where reclient log files are stored. - logDir string // recorder is responsible for recording metrics. recorder recorder - // authCredentials is a token to use for authenticating to the monitoring service. - authCredentials *auth.Credentials + // ts is a token source to use for authenticating to the monitoring service. + ts *oauth.TokenSource } // NewExporter returns a new Cloud monitoring metrics exporter. -func NewExporter(ctx context.Context, project, prefix, namespace string, remoteDisabled bool, logDir string, creds *auth.Credentials) (*Exporter, error) { +func NewExporter(ctx context.Context, project, prefix, namespace string, ts *oauth.TokenSource) (*Exporter, error) { e := &Exporter{ - project: project, - prefix: prefix, - namespace: namespace, - logDir: logDir, - recorder: &stackDriverRecorder{}, - authCredentials: creds, - remoteDisabled: remoteDisabled, + project: project, + prefix: prefix, + namespace: namespace, + recorder: &stackDriverRecorder{}, + ts: ts, } if err := e.initCloudMonitoring(ctx); err != nil { return nil, err @@ -211,8 +204,8 @@ func (e *Exporter) initCloudMonitoring(ctx context.Context) error { MonitoredResource: e, DefaultMonitoringLabels: &stackdriver.Labels{}, } - if ts := e.authCredentials.TokenSource(); ts != nil { - clientOpt := option.WithTokenSource(ts) + if e.ts != nil { + clientOpt := option.WithTokenSource(e.ts) opts.MonitoringClientOptions = []option.ClientOption{clientOpt} opts.TraceClientOptions = []option.ClientOption{clientOpt} } @@ -221,7 +214,7 @@ func (e *Exporter) initCloudMonitoring(ctx context.Context) error { } // ExportActionMetrics exports metrics for one log record to opencensus. -func (e *Exporter) ExportActionMetrics(ctx context.Context, r *lpb.LogRecord) { +func (e *Exporter) ExportActionMetrics(ctx context.Context, r *lpb.LogRecord, remoteDisabled bool) { aCtx := e.recorder.tagsContext(ctx, staticKeys) aCtx = e.recorder.tagsContext(aCtx, map[tag.Key]string{ osFamilyKey: runtime.GOOS, @@ -235,15 +228,15 @@ func (e *Exporter) ExportActionMetrics(ctx context.Context, r *lpb.LogRecord) { latency = float64(ti.To.Sub(ti.From).Milliseconds()) } } - e.recorder.recordWithTags(aCtx, e.makeActionTags(r), ActionCount.M(1)) - e.recorder.recordWithTags(aCtx, e.makeActionTags(r), ActionLatency.M(latency)) + e.recorder.recordWithTags(aCtx, e.makeActionTags(r, remoteDisabled), ActionCount.M(1)) + e.recorder.recordWithTags(aCtx, e.makeActionTags(r, remoteDisabled), ActionLatency.M(latency)) } -func (e *Exporter) makeActionTags(r *lpb.LogRecord) map[tag.Key]string { +func (e *Exporter) makeActionTags(r *lpb.LogRecord, remoteDisabled bool) map[tag.Key]string { return map[tag.Key]string{ labelsKey: labels.ToKey(r.GetLocalMetadata().GetLabels()), statusKey: r.GetResult().GetStatus().String(), - remoteDisabledKey: strconv.FormatBool(e.remoteDisabled), + remoteDisabledKey: strconv.FormatBool(remoteDisabled), remoteStatusKey: r.GetRemoteMetadata().GetResult().GetStatus().String(), exitCodeKey: strconv.FormatInt(int64(r.GetResult().GetExitCode()), 10), remoteExitCodeKey: strconv.FormatInt(int64(r.GetRemoteMetadata().GetResult().GetExitCode()), 10), @@ -251,13 +244,14 @@ func (e *Exporter) makeActionTags(r *lpb.LogRecord) map[tag.Key]string { } // ExportBuildMetrics exports overall build metrics to opencensus. -func (e *Exporter) ExportBuildMetrics(ctx context.Context, sp *spb.Stats, shutdownTimeInterval *cpb.TimeInterval) { +func (e *Exporter) ExportBuildMetrics(ctx context.Context, sp *spb.Stats) { numRecs := sp.NumRecords aCtx := e.recorder.tagsContext(ctx, staticKeys) + aCtx = e.recorder.tagsContext(aCtx, map[tag.Key]string{ osFamilyKey: runtime.GOOS, versionKey: version.CurrentVersion(), - remoteDisabledKey: strconv.FormatBool(e.remoteDisabled), + remoteDisabledKey: remoteDisabledFlagValue(sp), }) if numRecs == 0 { return @@ -265,7 +259,7 @@ func (e *Exporter) ExportBuildMetrics(ctx context.Context, sp *spb.Stats, shutdo e.recorder.recordWithTags(aCtx, nil, BuildCacheHitRatio.M(sp.BuildCacheHitRatio)) e.recorder.recordWithTags(aCtx, nil, BuildLatency.M(sp.BuildLatency)) status := "SUCCESS" - if e.checkBuildFailure(aCtx) { + if sp.FatalExit { status = "FAILURE" } e.recorder.recordWithTags(aCtx, map[tag.Key]string{statusKey: status}, BuildCount.M(1)) @@ -277,29 +271,33 @@ func (e *Exporter) ExportBuildMetrics(ctx context.Context, sp *spb.Stats, shutdo millis := command.TimeFromProto(ti.To).Sub(command.TimeFromProto(ti.From)).Milliseconds() e.recorder.recordWithTags(aCtx, nil, BootstrapStartupLatency.M(millis)) } + if ti, ok := pi.EventTimes[logger.EventBootstrapShutdown]; ok { + millis := command.TimeFromProto(ti.To).Sub(command.TimeFromProto(ti.From)).Milliseconds() + e.recorder.recordWithTags(aCtx, nil, BootstrapShutdownLatency.M(millis)) + } } - millis := command.TimeFromProto(shutdownTimeInterval.To).Sub(command.TimeFromProto(shutdownTimeInterval.From)).Milliseconds() - e.recorder.recordWithTags(aCtx, nil, BootstrapShutdownLatency.M(millis)) } -// Close stops the metrics exporter and waits for the exported data to be uploaded. -func (e *Exporter) Close() { - e.recorder.close() -} - -func (e *Exporter) checkBuildFailure(ctx context.Context) bool { - for _, f := range failureFiles { - fp := filepath.Join(e.logDir, f) - s, err := os.Stat(fp) - if err != nil { +func remoteDisabledFlagValue(sp *spb.Stats) string { + for _, pi := range sp.GetProxyInfo() { + sv, ok := pi.Flags["remote_disabled"] + if !ok { continue } - if s.Size() == 0 { + v, err := strconv.ParseBool(sv) + if err != nil { continue } - return true + if v { + return strconv.FormatBool(true) + } } - return false + return strconv.FormatBool(false) +} + +// Close stops the metrics exporter and waits for the exported data to be uploaded. +func (e *Exporter) Close() { + e.recorder.close() } // stackDriverRecorder is a recorder for stack driver metrics. diff --git a/internal/pkg/monitoring/monitoring_test.go b/internal/pkg/monitoring/monitoring_test.go index e07ae157..12476690 100644 --- a/internal/pkg/monitoring/monitoring_test.go +++ b/internal/pkg/monitoring/monitoring_test.go @@ -18,8 +18,6 @@ import ( "context" "errors" "fmt" - "os" - "path/filepath" "runtime" "sort" "strconv" @@ -28,7 +26,6 @@ import ( "time" lpb "team/foundry-x/re-client/api/log" - "team/foundry-x/re-client/internal/pkg/auth" "team/foundry-x/re-client/internal/pkg/logger" st "team/foundry-x/re-client/internal/pkg/stats" "team/foundry-x/re-client/pkg/version" @@ -44,18 +41,21 @@ import ( ) func TestExportMetrics(t *testing.T) { - tests := []struct { - name string - remoteDisabled bool - }{ - { - name: "RemoteEnabled", - remoteDisabled: false, - }, - { - name: "RemoteDisabled", - remoteDisabled: true, - }, + var tests []struct { + name string + remoteDisabled, fatalExit bool + } + for _, remoteDisabled := range []bool{true, false} { + for _, fatalExit := range []bool{true, false} { + tests = append(tests, struct { + name string + remoteDisabled, fatalExit bool + }{ + name: fmt.Sprintf("RemoteDisabled=%v,FatalExit=%v", remoteDisabled, fatalExit), + remoteDisabled: remoteDisabled, + fatalExit: fatalExit, + }) + } } for _, tc := range tests { tc := tc @@ -123,30 +123,41 @@ func TestExportMetrics(t *testing.T) { To: command.TimeToProto(start), }, }, + Flags: map[string]string{ + "remote_disabled": strconv.FormatBool(tc.remoteDisabled), + }, + }, + { + EventTimes: map[string]*cpb.TimeInterval{ + logger.EventBootstrapShutdown: { + From: command.TimeToProto(start.Add(-500 * time.Millisecond)), + To: command.TimeToProto(start), + }, + }, }, }) sp := s.ToProto() + sp.FatalExit = tc.fatalExit r := &stubRecorder{reports: make([]*metricReport, 0)} e := &Exporter{ - project: "fake-project", - recorder: r, - authCredentials: &auth.Credentials{}, - remoteDisabled: tc.remoteDisabled, + project: "fake-project", + recorder: r, + ts: nil, } err := e.initCloudMonitoring(context.Background()) if err != nil { t.Errorf("Failed to initialize cloud monitoring: %v", err) } - now := time.Now() - e.ExportBuildMetrics(context.Background(), sp, &cpb.TimeInterval{ - From: command.TimeToProto(now.Add(-500 * time.Millisecond)), - To: command.TimeToProto(now), - }) + e.ExportBuildMetrics(context.Background(), sp) for _, r := range recs { - e.ExportActionMetrics(context.Background(), r) + e.ExportActionMetrics(context.Background(), r, tc.remoteDisabled) } e.Close() + wantBuildStatus := "SUCCESS" + if tc.fatalExit { + wantBuildStatus = "FAILURE" + } wantReports := []*metricReport{ { Name: ActionCount.Name(), @@ -238,7 +249,7 @@ func TestExportMetrics(t *testing.T) { Tags: map[string]string{ osFamilyKey.Name(): runtime.GOOS, versionKey.Name(): version.CurrentVersion(), - statusKey.Name(): "SUCCESS", + statusKey.Name(): wantBuildStatus, remoteDisabledKey.Name(): strconv.FormatBool(tc.remoteDisabled), }, }, @@ -288,176 +299,14 @@ func TestExportMetrics(t *testing.T) { }) } } - -func TestExportBuildFailureMetrics(t *testing.T) { - tests := []struct { - name string - remoteDisabled bool - }{ - { - name: "RemoteEnabled", - remoteDisabled: false, - }, - { - name: "RemoteDisabled", - remoteDisabled: true, - }, - } - for _, tc := range tests { - tc := tc - t.Run(tc.name, func(t *testing.T) { - t1 := time.Now() - t2 := t1.Add(time.Second) - recs := []*lpb.LogRecord{ - { - Result: &cpb.CommandResult{Status: cpb.CommandResultStatus_CACHE_HIT}, - RemoteMetadata: &lpb.RemoteMetadata{ - Result: &cpb.CommandResult{Status: cpb.CommandResultStatus_CACHE_HIT}, - }, - LocalMetadata: &lpb.LocalMetadata{ - EventTimes: map[string]*cpb.TimeInterval{ - "ProxyExecution": { - From: command.TimeToProto(t1), - To: command.TimeToProto(t2), - }, - }, - Labels: map[string]string{"type": "tool"}, - }, - }, - } - start := time.Now() - s := st.NewFromRecords(recs, []*lpb.ProxyInfo{ - { - EventTimes: map[string]*cpb.TimeInterval{ - logger.EventBootstrapStartup: { - From: command.TimeToProto(start.Add(-200 * time.Millisecond)), - To: command.TimeToProto(start), - }, - }, - }, - }) - sp := s.ToProto() - logDir := t.TempDir() - r := &stubRecorder{reports: make([]*metricReport, 0)} - e := &Exporter{ - project: "fake-project", - recorder: r, - logDir: logDir, - authCredentials: &auth.Credentials{}, - remoteDisabled: tc.remoteDisabled, - } - err := e.initCloudMonitoring(context.Background()) - if err != nil { - t.Errorf("Failed to initialize cloud monitoring: %v", err) - } - logFile := "reproxy.FATAL" - if runtime.GOOS == "windows" { - logFile = "reproxy.exe.FATAL" - } - os.WriteFile(filepath.Join(logDir, logFile), []byte("FATAL"), 0666) - now := time.Now() - e.ExportBuildMetrics(context.Background(), sp, &cpb.TimeInterval{ - From: command.TimeToProto(now.Add(-500 * time.Millisecond)), - To: command.TimeToProto(now), - }) - for _, r := range recs { - e.ExportActionMetrics(context.Background(), r) - } - e.Close() - wantReports := []*metricReport{ - { - Name: ActionCount.Name(), - Val: 1, - Tags: map[string]string{ - labelsKey.Name(): "[type=tool]", - osFamilyKey.Name(): runtime.GOOS, - versionKey.Name(): version.CurrentVersion(), - remoteStatusKey.Name(): "CACHE_HIT", - statusKey.Name(): "CACHE_HIT", - remoteExitCodeKey.Name(): "0", - exitCodeKey.Name(): "0", - remoteDisabledKey.Name(): strconv.FormatBool(tc.remoteDisabled), - }, - }, - { - Name: ActionLatency.Name(), - Val: 1000, - Tags: map[string]string{ - labelsKey.Name(): "[type=tool]", - osFamilyKey.Name(): runtime.GOOS, - versionKey.Name(): version.CurrentVersion(), - remoteStatusKey.Name(): "CACHE_HIT", - statusKey.Name(): "CACHE_HIT", - remoteExitCodeKey.Name(): "0", - exitCodeKey.Name(): "0", - remoteDisabledKey.Name(): strconv.FormatBool(tc.remoteDisabled), - }, - }, - { - Name: BuildCount.Name(), - Val: 1, - Tags: map[string]string{ - osFamilyKey.Name(): runtime.GOOS, - versionKey.Name(): version.CurrentVersion(), - statusKey.Name(): "FAILURE", - remoteDisabledKey.Name(): strconv.FormatBool(tc.remoteDisabled), - }, - }, - { - Name: BuildLatency.Name(), - Val: 1, - Tags: map[string]string{ - osFamilyKey.Name(): runtime.GOOS, - versionKey.Name(): version.CurrentVersion(), - remoteDisabledKey.Name(): strconv.FormatBool(tc.remoteDisabled), - }, - }, - { - Name: BuildCacheHitRatio.Name(), - Val: 1.0, - Tags: map[string]string{ - osFamilyKey.Name(): runtime.GOOS, - versionKey.Name(): version.CurrentVersion(), - remoteDisabledKey.Name(): strconv.FormatBool(tc.remoteDisabled), - }, - }, - { - Name: BootstrapShutdownLatency.Name(), - Val: 500, - Tags: map[string]string{ - osFamilyKey.Name(): runtime.GOOS, - versionKey.Name(): version.CurrentVersion(), - remoteDisabledKey.Name(): strconv.FormatBool(tc.remoteDisabled), - }, - }, - { - Name: BootstrapStartupLatency.Name(), - Val: 200, - Tags: map[string]string{ - osFamilyKey.Name(): runtime.GOOS, - versionKey.Name(): version.CurrentVersion(), - remoteDisabledKey.Name(): strconv.FormatBool(tc.remoteDisabled), - }, - }, - } - repCmp := cmpopts.SortSlices(func(a, b *metricReport) bool { - return a.hash() < b.hash() - }) - if diff := cmp.Diff(wantReports, r.reports, repCmp); diff != "" { - t.Errorf("Recorded metrics have diff: (-want +got)\n%s", diff) - } - }) - } -} func TestInitCloudMonitoringError(t *testing.T) { r := &stubRecorder{ reports: make([]*metricReport, 0), err: errors.New("fake error"), } e := &Exporter{ - project: "fake-project", - recorder: r, - authCredentials: &auth.Credentials{}, + project: "fake-project", + recorder: r, } if err := e.initCloudMonitoring(context.Background()); err == nil { t.Errorf("initCloudMonitoring succeeded; expected failure") diff --git a/internal/pkg/reproxypid/BUILD.bazel b/internal/pkg/reproxypid/BUILD.bazel index 2425af12..0656e097 100644 --- a/internal/pkg/reproxypid/BUILD.bazel +++ b/internal/pkg/reproxypid/BUILD.bazel @@ -2,14 +2,13 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "reproxypid", - srcs = [ - "exists_unix.go", - "exists_windows.go", - "reproxypid.go", - ], + srcs = ["reproxypid.go"], importpath = "team/foundry-x/re-client/internal/pkg/reproxypid", visibility = ["//:__subpackages__"], - deps = ["@com_github_golang_glog//:go_default_library"], + deps = [ + "//internal/pkg/subprocess", + "@com_github_golang_glog//:go_default_library", + ], ) go_test( diff --git a/internal/pkg/reproxypid/reproxypid.go b/internal/pkg/reproxypid/reproxypid.go index 61c4dd39..4034b75d 100644 --- a/internal/pkg/reproxypid/reproxypid.go +++ b/internal/pkg/reproxypid/reproxypid.go @@ -24,6 +24,8 @@ import ( "strings" "time" + "team/foundry-x/re-client/internal/pkg/subprocess" + log "github.com/golang/glog" ) @@ -86,7 +88,7 @@ func (f *File) Delete() { // IsAlive retuns true if the reproxy process is still running. func (f *File) IsAlive() (bool, error) { - return exists(f.Pid) + return subprocess.Exists(f.Pid) } func pathForServerAddr(serverAddr string) (string, error) { diff --git a/internal/pkg/subprocess/BUILD.bazel b/internal/pkg/subprocess/BUILD.bazel index 359bda2c..9629c15d 100644 --- a/internal/pkg/subprocess/BUILD.bazel +++ b/internal/pkg/subprocess/BUILD.bazel @@ -2,7 +2,11 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "subprocess", - srcs = ["subprocess.go"], + srcs = [ + "exists_unix.go", + "exists_windows.go", + "subprocess.go", + ], importpath = "team/foundry-x/re-client/internal/pkg/subprocess", visibility = ["//visibility:public"], deps = [ diff --git a/internal/pkg/reproxypid/exists_unix.go b/internal/pkg/subprocess/exists_unix.go similarity index 90% rename from internal/pkg/reproxypid/exists_unix.go rename to internal/pkg/subprocess/exists_unix.go index 1cc35a9b..b8d88917 100644 --- a/internal/pkg/reproxypid/exists_unix.go +++ b/internal/pkg/subprocess/exists_unix.go @@ -14,16 +14,16 @@ //go:build !windows -package reproxypid +package subprocess import ( "os" "syscall" ) -// exists returns true if a pid is assigned to a process that is actively running. +// Exists returns true if a pid is assigned to a process that is actively running. // Based on comment from: https://github.com/golang/go/issues/34396 -func exists(pid int) (bool, error) { +func Exists(pid int) (bool, error) { proc, err := os.FindProcess(pid) if err != nil { return false, err diff --git a/internal/pkg/reproxypid/exists_windows.go b/internal/pkg/subprocess/exists_windows.go similarity index 87% rename from internal/pkg/reproxypid/exists_windows.go rename to internal/pkg/subprocess/exists_windows.go index f03aae16..64799c16 100644 --- a/internal/pkg/reproxypid/exists_windows.go +++ b/internal/pkg/subprocess/exists_windows.go @@ -14,15 +14,14 @@ //go:build windows -package reproxypid +package subprocess import "os" -// exists returns true if a pid is assigned to a process that is actively running. -// +// Exists returns true if a pid is assigned to a process that is actively running. // In the windows case, a call to FindProcess should be sufficient, as it will return an // error if there is no process assigned to that pid. -func exists(pid int) (bool, error) { +func Exists(pid int) (bool, error) { p, err := os.FindProcess(pid) if err != nil || p == nil { return false, nil