RSDK-9369: Initialize FTDC after the web service starts. (#4593)

Co-authored-by: Benjamin Rewis <[email protected]>

Author: dgottlieb

ftdc/ftdc.go

@@ -70,9 +70,13 @@ type Statser interface {
 	// The Stats method must return a struct with public field members that are either:
 	// - Numbers (e.g: int, float64, byte, etc...)
 	// - A "recursive" structure that has the same properties as this return value (public field
-	//   members with numbers, or more structures). (NOT YET SUPPORTED)
+	//   members with numbers, or more structures).
 	//
-	// The return value must not be a map. This is to enforce a "schema" constraints.
+	// The return value must always have the same schema. That is, the returned value has the same
+	// set of keys/structure members. A simple structure where all the member fields are numbers
+	// satisfies this requirement.
+	//
+	// The return value may not (yet) be a `map`. Even if the returned map always has the same keys.
 	Stats() any
 }
 

robot/impl/local_robot.go

@@ -88,6 +88,7 @@ type localRobot struct {
 	// map keyed by Module.Name. This is necessary to get the package manager to use a new folder
 	// when a local tarball is updated.
 	localModuleVersions map[string]semver.Version
+	startFtdcOnce       sync.Once
 	ftdc                *ftdc.FTDC
 
 	// whether the robot is actively reconfiguring
@@ -244,7 +245,13 @@ func (r *localRobot) Logger() logging.Logger {
 
 // StartWeb starts the web server, will return an error if server is already up.
 func (r *localRobot) StartWeb(ctx context.Context, o weboptions.Options) (err error) {
-	return r.webSvc.Start(ctx, o)
+	ret := r.webSvc.Start(ctx, o)
+	r.startFtdcOnce.Do(func() {
+		if r.ftdc != nil {
+			r.ftdc.Start()
+		}
+	})
+	return ret
 }
 
 // StopWeb stops the web server, will be a noop if server is not up.
@@ -421,8 +428,23 @@ func newWithResources(
 			partID = cfg.Cloud.ID
 		}
 		// CloudID is also known as the robot part id.
+		//
+		// RSDK-9369: We create a new FTDC worker, but do not yet start it. This is because the
+		// `webSvc` gets registered with FTDC before we construct the underlying
+		// `webSvc.rpcServer`. Which happens when calling `localRobot.StartWeb`. We've postponed
+		// starting FTDC to when that method is called (the first time).
+		//
+		// As per the FTDC.Statser interface documentation, the return value of `webSvc.Stats` must
+		// always have the same schema. Otherwise we risk the ftdc "schema" getting out of sync with
+		// the data being written. Having `webSvc.Stats` conform to the API requirements is
+		// challenging when we want to include stats from the `rpcServer`.
+		//
+		// RSDK-9369 can be reverted, having the FTDC worker getting started here, when we either:
+		// - Relax the requirement that successive calls to `Stats` have the same schema or
+		// - Guarantee that the `rpcServer` is initialized (enough) when the web service is
+		//   constructed to get a valid copy of its stats object (for the schema's sake). Even if
+		//   the web service has not been "started".
 		ftdcWorker = ftdc.New(ftdc.DefaultDirectory(config.ViamDotDir, partID), logger.Sublogger("ftdc"))
-		ftdcWorker.Start()
 	}
 
 	closeCtx, cancel := context.WithCancel(ctx)

robot/web/web.go

@@ -925,6 +925,20 @@ func (svc *webService) foreignServiceHandler(srv interface{}, stream googlegrpc.
 	}
 }
 
+type stats struct {
+	RPCServer any
+}
+
+// Stats returns ftdc data on behalf of the rpcServer and other web services.
+func (svc *webService) Stats() any {
+	// RSDK-9369: It's not ideal to block in `Stats`. But we don't today expect this to be
+	// problematic, and alternatives are more complex/expensive.
+	svc.mu.Lock()
+	defer svc.mu.Unlock()
+
+	return stats{svc.rpcServer.Stats()}
+}
+
 // RestartStatusResponse is the JSON response of the `restart_status` HTTP
 // endpoint.
 type RestartStatusResponse struct {

robot/web/web_c.go

@@ -57,12 +57,6 @@ type webService struct {
 	modWorkers   sync.WaitGroup
 }
 
-func (svc *webService) Stats() any {
-	return struct {
-		RPCServer any
-	}{svc.rpcServer.Stats()}
-}
-
 // Reconfigure pulls resources and updates the stream server audio and video streams with the new resources.
 func (svc *webService) Reconfigure(ctx context.Context, deps resource.Dependencies, _ resource.Config) error {
 	svc.mu.Lock()

robot/web/web_notc.go

@@ -66,11 +66,5 @@ func (svc *webService) initStreamServer(ctx context.Context) error {
 	return nil
 }
 
-func (svc *webService) Stats() any {
-	return struct {
-		RPCServer any
-	}{svc.rpcServer.Stats()}
-}
-
 // stub for missing gostream
 type options struct{}