fix(telemetry): Application Insights Shutdown on Retina Operator #1227

mereta · 2025-01-16T12:18:44Z

Description

Fix for missing telemetry after application shutdown.
This pull request includes changes to improve error handling in the operator/cmd/legacy/deployment.go and operator/cmd/root.go files. The main focus is to replace os.Exit(1) calls with error returns to allow for better error management and testing.

Error handling improvements:

operator/cmd/legacy/deployment.go: Modified the Start method to return an error instead of calling os.Exit(1) on failure. This change affects multiple points within the method where errors are now returned instead of causing the program to exit.
operator/cmd/root.go: Updated the Run function to RunE to handle errors returned by the Start method of the Operator. This ensures that any errors encountered during the startup process are properly returned and handled.

Related Issue

#1203

Checklist

I have read the contributing documentation.
I signed and signed-off the commits (git commit -S -s ...). See this documentation on signing commits.
I have correctly attributed the author(s) of the code.
I have tested the changes locally.
I have followed the project's style guidelines.
I have updated the documentation, if necessary.
I have added tests, if applicable.

Screenshots (if applicable) or Testing Completed

Please add any relevant screenshots or GIFs to showcase the changes made.

Additional Notes

Add any additional notes or context about the pull request here.

Please refer to the CONTRIBUTING.md file for more information on how to contribute to this project.

huntergregory

Can you also make the relevant changes for the non-legacy (cilium-crd) operator? Up to you if it should be the same PR or separate.

In your testing, could you build a new retina image with your changes, deploy it in your cluster, and induce an error to verify that the final error log shows up in app insights? Could induce an error by modifying the code for testing.

huntergregory · 2025-01-16T17:54:00Z

operator/cmd/legacy/deployment.go

@@ -176,10 +165,14 @@ func (o *Operator) Start() {
 			"version":                   buildinfo.Version,
 			telemetry.PropertyApiserver: apiserverURL,
 		}
+
+		telemetry.InitAppInsights(buildinfo.ApplicationInsightsID, buildinfo.Version)


I thought we verified in the past that operator was sending logs to app insights. I'm confused what this function does

We also seem to init app insights here and with a different config:

retina/pkg/log/zap.go

Lines 95 to 100 in a2c261f

aiTelemetryConfig := appinsights.NewTelemetryConfiguration(lOpts.ApplicationInsightsID)

sinkcfg := zapai.SinkConfig{

GracePeriod: 30 * time.Second, //nolint:gomnd // ignore

TelemetryConfiguration: *aiTelemetryConfig,

}

sinkcfg.MaxBatchSize = 10000

@jimassa / @matmerr what's the difference between configuring zap with AI vs initializing AI client? What's the impact of having different configs?

huntergregory · 2025-01-16T17:56:11Z

operator/cmd/legacy/deployment.go

@@ -176,10 +165,14 @@ func (o *Operator) Start() {
 			"version":                   buildinfo.Version,
 			telemetry.PropertyApiserver: apiserverURL,
 		}
+
+		telemetry.InitAppInsights(buildinfo.ApplicationInsightsID, buildinfo.Version)
+		defer telemetry.ShutdownAppInsights()


nit: let's shutdown wherever we handle the error?

I don't understand the request @huntergregory . The defer will run when we return the errors?

huntergregory · 2025-01-16T17:57:02Z

operator/cmd/root.go

-			d.Start()
+
+			if err := d.Start(); err != nil {
+				return errors.Wrap(err, "failed to start retina-operator")


let's log the error before the program exits

I'm going to amend that. "Let's assure that any error returned is logged immediately before os.Exit()". I'm pretty sure the answer to this is "yes, RunE already does that." But it would be good to double-check.

Log in-situ and return error is redundant and carries a lot of mental overhead both in reading and writing. The simple rule of "if an error makes it to the top of the call stack, it gets logged, loudly" is semantically cleaner and does the right thing by default. When combined with "all errors are wrapped," you also get detailed context to track down what went wrong.

huntergregory · 2025-01-16T17:58:04Z

operator/cmd/root.go

@@ -25,10 +26,14 @@ var (
 		Use:   "retina-operator",
 		Short: "Retina Operator",
 		Long:  "Start Retina Operator",
-		Run: func(cmd *cobra.Command, args []string) {
+		RunE: func(_ *cobra.Command, _ []string) error {


what does RunE do? Another option is to leave as Run and os.exit on err?

same as Run but with an error return

Yes, and importantly it will return a non-zero exit on err != nil for parity with existing behavior

huntergregory · 2025-01-16T17:59:35Z

operator/cmd/legacy/deployment.go

 	}

 	mainLogger.Info("Starting manager")
 	if err := mgr.Start(ctrlCtx); err != nil {
-		mainLogger.Error("Problem running manager", zap.Error(err))
-		os.Exit(1)
+		return errors.Wrap(err, "problem running manager")
 	}

 	// start heartbeat goroutine for application insights
 	go tel.Heartbeat(ctx, HeartbeatFrequency)


I think Start is a blocking call, so it never gets reached. Heartbeat should be called above

timraymond · 2025-01-17T16:20:06Z

operator/cmd/legacy/deployment.go

@@ -165,8 +155,7 @@ func (o *Operator) Start() {

 	apiserverURL, err := telemetry.GetK8SApiserverURLFromKubeConfig()
 	if err != nil {
-		mainLogger.Error("Apiserver URL is cannot be found", zap.Error(err))
-		os.Exit(1)
+		return errors.Wrap(err, "apiserver URL is cannot be found")


Suggested change

return errors.Wrap(err, "apiserver URL is cannot be found")

return errors.Wrap(err, "apiserver URL cannot be found")

timraymond · 2025-01-17T16:22:57Z

operator/cmd/legacy/deployment.go

@@ -176,10 +165,14 @@ func (o *Operator) Start() {
 			"version":                   buildinfo.Version,
 			telemetry.PropertyApiserver: apiserverURL,
 		}
+
+		telemetry.InitAppInsights(buildinfo.ApplicationInsightsID, buildinfo.Version)
+		defer telemetry.ShutdownAppInsights()


I don't understand the request @huntergregory . The defer will run when we return the errors?

timraymond · 2025-01-17T16:24:48Z

operator/cmd/root.go

@@ -25,10 +26,14 @@ var (
 		Use:   "retina-operator",
 		Short: "Retina Operator",
 		Long:  "Start Retina Operator",
-		Run: func(cmd *cobra.Command, args []string) {
+		RunE: func(_ *cobra.Command, _ []string) error {


Yes, and importantly it will return a non-zero exit on err != nil for parity with existing behavior

timraymond · 2025-01-17T16:31:18Z

operator/cmd/root.go

-			d.Start()
+
+			if err := d.Start(); err != nil {
+				return errors.Wrap(err, "failed to start retina-operator")


I'm going to amend that. "Let's assure that any error returned is logged immediately before os.Exit()". I'm pretty sure the answer to this is "yes, RunE already does that." But it would be good to double-check.

Log in-situ and return error is redundant and carries a lot of mental overhead both in reading and writing. The simple rule of "if an error makes it to the top of the call stack, it gets logged, loudly" is semantically cleaner and does the right thing by default. When combined with "all errors are wrapped," you also get detailed context to track down what went wrong.

mereta added 2 commits January 16, 2025 12:07

Adding telemetry shutdown for Retina Operator

cbf4179

Adding telemetry shutdown for Retina Operator

e2dcffb

mereta requested a review from a team as a code owner January 16, 2025 12:18

mereta requested review from mushiboy and MikeZappa87 January 16, 2025 12:18

Merge branch 'main' into telemetry-shutdown

75dcf89

mereta changed the title ~~Telemetry shutdown~~ fix(telemetry shutdown) Jan 16, 2025

mereta changed the title ~~fix(telemetry shutdown)~~ fix(telemetry): Application Insights Shutdown on Retina Operator Jan 16, 2025

mereta added 4 commits January 16, 2025 13:00

Wrap errors

ae09d71

Wrap errors

e91b496

Wrap errors

75f2ee7

Fix linter issue - unused var

094e24c

mereta linked an issue Jan 16, 2025 that may be closed by this pull request

log: if there's a crash, final telemetry logs might not be sent #1203

Open

Merge branch 'main' into telemetry-shutdown

21af2b2

huntergregory requested changes Jan 16, 2025

View reviewed changes

timraymond reviewed Jan 17, 2025

View reviewed changes

mereta self-assigned this Jan 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(telemetry): Application Insights Shutdown on Retina Operator #1227

fix(telemetry): Application Insights Shutdown on Retina Operator #1227

mereta commented Jan 16, 2025

huntergregory left a comment

huntergregory Jan 16, 2025

huntergregory Jan 16, 2025

timraymond Jan 17, 2025

huntergregory Jan 16, 2025

timraymond Jan 17, 2025

huntergregory Jan 16, 2025

rbtr Jan 16, 2025

timraymond Jan 17, 2025

huntergregory Jan 16, 2025

timraymond Jan 17, 2025

timraymond Jan 17, 2025

timraymond Jan 17, 2025

timraymond Jan 17, 2025

	aiTelemetryConfig := appinsights.NewTelemetryConfiguration(lOpts.ApplicationInsightsID)
	sinkcfg := zapai.SinkConfig{
	GracePeriod: 30 * time.Second, //nolint:gomnd // ignore
	TelemetryConfiguration: *aiTelemetryConfig,
	}
	sinkcfg.MaxBatchSize = 10000

	return errors.Wrap(err, "apiserver URL is cannot be found")
	return errors.Wrap(err, "apiserver URL cannot be found")

fix(telemetry): Application Insights Shutdown on Retina Operator #1227

Are you sure you want to change the base?

fix(telemetry): Application Insights Shutdown on Retina Operator #1227

Conversation

mereta commented Jan 16, 2025

Description

Related Issue

Checklist

Screenshots (if applicable) or Testing Completed

Additional Notes

huntergregory 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

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