RSDK-8819: Finish FTDC #4579

dgottlieb · 2024-11-21T19:11:42Z

No description provided.

dgottlieb · 2024-11-21T19:13:45Z

ftdc/ftdc.go

+// `ftdcDirectory`.
+func New(ftdcDirectory string, logger logging.Logger) *FTDC {
+	ret := newFTDC(logger)
+	ret.maxFileSizeBytes = 1_000_000


Made the settings a more formal division. One either passes in a directory and all these variables about managing files are initialized. Or one passes in a writer and none of those variables matter.

dgottlieb · 2024-11-21T19:14:28Z

ftdc/ftdc.go

@@ -536,7 +553,7 @@ func (ftdc *FTDC) checkAndDeleteOldFiles() error {
 // deletion testing. Filename generation uses padding such that we can rely on there before 2/4
 // digits for every numeric value.
 //
-//nolint
+// nolint


It joys me that our linter lints our linter

Or maybe this was gofmt. The linter complained and it's now back to its unindented state.

dgottlieb · 2024-11-21T19:15:29Z

ftdc/ftdc_test.go

-	ftdc.ftdcDir = ftdcFileDir
+	// We must not use `NewWithWriter`. Forcing a writer for FTDC data is not compatible with FTDC
+	// file rotation.
+	ftdc := New(ftdcFileDir, logger.Sublogger("ftdc"))


The "refactor" of New vs NewWithWriter resulted in changing the order here. First create the directory, then pass it to the constructor.

dgottlieb · 2024-11-21T19:15:52Z

robot/impl/local_robot.go

@@ -416,7 +416,8 @@ func newWithResources(

 	var ftdcWorker *ftdc.FTDC
 	if rOpts.enableFTDC {
-		ftdcWorker = ftdc.New(logger.Sublogger("ftdc"))
+		// CloudID is also known as the robot part id.
+		ftdcWorker = ftdc.New(ftdc.DefaultDirectory(config.ViamDotDir, cfg.Cloud.ID), logger.Sublogger("ftdc"))


Technically has a bug -- cfg.Cloud.ID is nil for local configs. Fixed in last commit.

dgottlieb · 2024-11-21T19:16:12Z

ftdc/cmd/parser.go

@@ -36,6 +39,31 @@ type gnuplotWriter struct {
 	options graphOptions
 }

+type KVPair[K, V any] struct {


I expect you'll enjoy this @benjirewis

dgottlieb · 2024-11-21T19:20:38Z

ftdc/cmd/parser.go

 		nolintPrintln("Expected an FTDC filename. E.g: go run parser.go <path-to>/viam-server.ftdc")
 		return
 	}

-	data, err := ftdc.Parse(ftdcFile)
+	logger := logging.NewDebugLogger("parser")
+	logger.SetLevel(logging.WARN)


Replaced with a simple NewLogger and removed the SetLevel -- using the INFO default.

dgottlieb · 2024-11-21T19:20:51Z

robot/impl/local_robot.go

@@ -416,8 +416,12 @@ func newWithResources(

 	var ftdcWorker *ftdc.FTDC
 	if rOpts.enableFTDC {
+		partID := "local-config"
+		if cfg.Cloud != nil {


What bug was this? Just looks like starting FTDC within a ~/.viam/[part-id|"local-config"] as a new feature.

Oh nvm I see your other comment.

dgottlieb · 2024-11-21T19:21:05Z

web/server/entrypoint.go

@@ -48,7 +48,7 @@ type Arguments struct {
 	OutputTelemetry            bool   `flag:"output-telemetry,usage=print out telemetry data (metrics and spans)"`
 	DisableMulticastDNS        bool   `flag:"disable-mdns,usage=disable server discovery through multicast DNS"`
 	DumpResourcesPath          string `flag:"dump-resources,usage=dump all resource registrations as json to the provided file path"`
-	EnableFTDC                 bool   `flag:"ftdc,usage=enable fulltime data capture for diagnostics [beta feature]"`
+	EnableFTDC                 bool   `flag:"ftdc,default=true,usage=enable fulltime data capture for diagnostics"`


Stole from WebRTC above on line 45. Hand tested that -ftdc=false turns this off.

benjirewis

Nice! Some questions.

benjirewis · 2024-11-22T20:21:58Z

ftdc/cmd/parser.go

+	Val V
+}
+
+func sorted[K cmp.Ordered, V any](mp map[K]V) []kvPair[K, V] {


Hah love it. Normally I've seen

Build a slice of the keys

sort.Strings that slice

Iterate through sorted slice to access map in key order

But this definitely also works.

benjirewis · 2024-11-22T20:22:59Z

ftdc/cmd/parser.go

 		nolintPrintln("Expected an FTDC filename. E.g: go run parser.go <path-to>/viam-server.ftdc")
 		return
 	}

-	data, err := ftdc.Parse(ftdcFile)
+	logger := logging.NewLogger("parser")
+	data, err := ftdc.ParseWithLogger(ftdcFile, logger)


Does the parser main program here get a lot of info-level-and-above logs now?

IIRC, there are no info logs. Just debug for item by item details and warn/error for problems.

benjirewis · 2024-11-22T20:25:36Z

ftdc/ftdc.go

+		// "other" users.
+		//
+		//nolint:gosec
+		if err = os.MkdirAll(ftdc.ftdcDir, 0o755); err != nil {


Won't this recreate the FTDC directory on every attempt to create a single new FTDC file?

MkdirAll has the same behavior as mkdir -p. Where it's a no-op + no error if the directory already exists.

FWIW, the case you're describing is exercised in a test.

benjirewis · 2024-11-22T20:27:27Z

robot/impl/local_robot.go

@@ -416,8 +416,12 @@ func newWithResources(

 	var ftdcWorker *ftdc.FTDC
 	if rOpts.enableFTDC {
+		partID := "local-config"
+		if cfg.Cloud != nil {


What bug was this? Just looks like starting FTDC within a ~/.viam/[part-id|"local-config"] as a new feature.

benjirewis · 2024-11-22T20:28:39Z

robot/web/web.go

@@ -188,6 +188,8 @@ type Service interface {

 	// Returns the unix socket path the module server listens on.
 	ModuleAddress() string
+
+	Stats() any


Surprised linter does not care about lack of comment here.

dgottlieb added 4 commits November 21, 2024 13:41

RSDK-8819: Add goutils rpc stats to FTDC.

a65d835

RSDK-8819: Parameterize FTDC diagnostics.data directory.

6a945bf

RSDK-8819: Sort charts in ftdc viewing by metric name.

8ca673d

RSDK-8819: Turn on FTDC by default.

68a11a6

dgottlieb requested a review from benjirewis November 21, 2024 19:11

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Nov 21, 2024

dgottlieb commented Nov 21, 2024

View reviewed changes

lint

61ef64a

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024

lint

906a3b7

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024

lint

fe547ab

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024

lint

9b1ce55

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024

lint lint lint lint

b21b443

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 22, 2024

benjirewis reviewed Nov 22, 2024

View reviewed changes

dgottlieb requested a review from benjirewis November 23, 2024 01:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSDK-8819: Finish FTDC #4579

RSDK-8819: Finish FTDC #4579

dgottlieb commented Nov 21, 2024

dgottlieb Nov 21, 2024

dgottlieb Nov 21, 2024

dgottlieb Nov 22, 2024

dgottlieb Nov 21, 2024

dgottlieb Nov 21, 2024

dgottlieb Nov 21, 2024

dgottlieb Nov 21, 2024

dgottlieb Nov 21, 2024

benjirewis Nov 22, 2024

benjirewis Nov 22, 2024

dgottlieb Nov 21, 2024

benjirewis left a comment

benjirewis Nov 22, 2024

benjirewis Nov 22, 2024

dgottlieb Nov 23, 2024

benjirewis Nov 22, 2024

dgottlieb Nov 23, 2024

benjirewis Nov 22, 2024

benjirewis Nov 22, 2024

RSDK-8819: Finish FTDC #4579

Are you sure you want to change the base?

RSDK-8819: Finish FTDC #4579

Conversation

dgottlieb commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjirewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment