Add statusz endpoint with basic diagnostic information. #131

davidbtucker · 2019-06-03T18:19:12Z

No description provided.

cmd/stackdriver-prometheus-sidecar/statusz_test.go

cmd/stackdriver-prometheus-sidecar/main.go

cmd/stackdriver-prometheus-sidecar/statusz.go

davidbtucker

Thanks! PTAL.

cmd/stackdriver-prometheus-sidecar/main.go

cmd/stackdriver-prometheus-sidecar/statusz.go

cmd/stackdriver-prometheus-sidecar/statusz_test.go

jkohen

The build is failing. Can you fix that so we can detect other errors?

jkohen · 2019-06-11T18:30:14Z

cmd/stackdriver-prometheus-sidecar/main.go

+//
+// Some fields are not part of this table; for example, metricRenames
+// and staticMetadata are displayed separately.
+func (c *mainConfig) TableForStatusz() map[string]string {


Does this method need to be exported? If not, start with a lowercase letter.

jkohen · 2019-06-11T18:31:05Z

cmd/stackdriver-prometheus-sidecar/main.go

+// endpoint.
+//
+// Some fields are not part of this table; for example, metricRenames
+// and staticMetadata are displayed separately.


Can you document how to decide what goes here and what doesn't? If there is a public design, we should link to that.

Per other comments, I removed this function and just referenced the config directly from the template.

I think all config fields should display on the /statusz page as long as there is some meaningful value (it now displays everything but manualResolver).

We don't have a public design yet.

jkohen · 2019-06-11T18:35:23Z

cmd/stackdriver-prometheus-sidecar/main.go

@@ -255,6 +283,9 @@ func main() {
 	a.Flag("web.listen-address", "Address to listen on for UI, API, and telemetry.").
 		Default("0.0.0.0:9091").StringVar(&cfg.listenAddress)

+	a.Flag("web.enable-statusz", "If true, then enables a /statusz endpoint on the web server with diagnostic information.").
+		Default("false").BoolVar(&cfg.enableStatusz)


What's the thinking for not leaving this on by default or always?

Changed to on by default. But yeah, maybe we should just omit the option entirely.

I don't have a strong feeling either way -- what do you suggest?

jkohen · 2019-06-11T18:44:35Z

cmd/stackdriver-prometheus-sidecar/statusz.go

+	}
+}
+
+var statuszTmpl = template.Must(template.New("").Parse(`


Can you extract this to a file? It'll be more convenient to maintain.

jkohen · 2019-06-11T18:54:07Z

cmd/stackdriver-prometheus-sidecar/statusz.go

+	data.GKEInfo.ClusterLocation = h.cfg.kubernetesLabels.location
+	data.GKEInfo.ClusterName = h.cfg.kubernetesLabels.clusterName
+
+	data.ConfigTable = h.cfg.TableForStatusz()


Have you considered using the main config struct as-is, and handling all the presentation in the template? Then we could remove the TableForStatusz method.

Good idea, done.

jkohen · 2019-06-11T18:55:26Z

cmd/stackdriver-prometheus-sidecar/statusz.go

+	data.Uname = Uname()
+	data.FdLimits = FdLimits()
+
+	data.StartTime = serverStart.Format(time.RFC1123)


We should use the newer RFC 3339 unless backward compatibility is needed.

jkohen · 2019-06-11T18:57:58Z

cmd/stackdriver-prometheus-sidecar/statusz.go

+	data.StartTime = serverStart.Format(time.RFC1123)
+	uptime := int64(time.Since(serverStart).Seconds())
+	data.Uptime = fmt.Sprintf("%d hr %02d min %02d sec",
+		uptime/3600, (uptime/60)%60, uptime%60)


Can you use time.Duration's default string conversion?

jkohen · 2019-06-11T19:16:29Z

kube/patch.sh

+          - name: POD_NAMESPACE
+            valueFrom:
+              fieldRef:
+                fieldPath: metadata.namespace


This will go straight into our Getting Started docs and I'm concerned about clutter. What do you think about adding this to the updating the configuration section instead?

Aha. I had put this in here so that I could use deploy.sh on a test cluster. Is there a better way make that work?

How about creating a copy the file for docs? Alternatively you could ask one of our tech writers to give a thumbs up on the larger snippet. I'd do it outside this PR, so you can submit the rest as-is.

Ok, removed these for now.

davidbtucker

Thanks! PTAL.

davidbtucker · 2019-06-11T19:34:20Z

cmd/stackdriver-prometheus-sidecar/statusz.go

+	data.Uname = Uname()
+	data.FdLimits = FdLimits()
+
+	data.StartTime = serverStart.Format(time.RFC1123)


davidbtucker · 2019-06-11T19:36:54Z

cmd/stackdriver-prometheus-sidecar/statusz.go

+	data.StartTime = serverStart.Format(time.RFC1123)
+	uptime := int64(time.Since(serverStart).Seconds())
+	data.Uptime = fmt.Sprintf("%d hr %02d min %02d sec",
+		uptime/3600, (uptime/60)%60, uptime%60)


davidbtucker · 2019-06-11T19:43:03Z

cmd/stackdriver-prometheus-sidecar/statusz.go

+	}
+}
+
+var statuszTmpl = template.Must(template.New("").Parse(`


davidbtucker · 2019-06-11T22:24:03Z

cmd/stackdriver-prometheus-sidecar/main.go

@@ -255,6 +283,9 @@ func main() {
 	a.Flag("web.listen-address", "Address to listen on for UI, API, and telemetry.").
 		Default("0.0.0.0:9091").StringVar(&cfg.listenAddress)

+	a.Flag("web.enable-statusz", "If true, then enables a /statusz endpoint on the web server with diagnostic information.").
+		Default("false").BoolVar(&cfg.enableStatusz)


Changed to on by default. But yeah, maybe we should just omit the option entirely.

I don't have a strong feeling either way -- what do you suggest?

davidbtucker · 2019-06-12T18:22:43Z

cmd/stackdriver-prometheus-sidecar/main.go

+// endpoint.
+//
+// Some fields are not part of this table; for example, metricRenames
+// and staticMetadata are displayed separately.


Per other comments, I removed this function and just referenced the config directly from the template.

I think all config fields should display on the /statusz page as long as there is some meaningful value (it now displays everything but manualResolver).

We don't have a public design yet.

davidbtucker · 2019-06-12T18:22:55Z

cmd/stackdriver-prometheus-sidecar/main.go

+//
+// Some fields are not part of this table; for example, metricRenames
+// and staticMetadata are displayed separately.
+func (c *mainConfig) TableForStatusz() map[string]string {


davidbtucker · 2019-06-12T18:24:07Z

cmd/stackdriver-prometheus-sidecar/statusz.go

+	data.GKEInfo.ClusterLocation = h.cfg.kubernetesLabels.location
+	data.GKEInfo.ClusterName = h.cfg.kubernetesLabels.clusterName
+
+	data.ConfigTable = h.cfg.TableForStatusz()


Good idea, done.

davidbtucker · 2019-06-12T18:27:49Z

kube/patch.sh

+          - name: POD_NAMESPACE
+            valueFrom:
+              fieldRef:
+                fieldPath: metadata.namespace


Aha. I had put this in here so that I could use deploy.sh on a test cluster. Is there a better way make that work?

jkohen

Really nice. Thanks, Dave!

jkohen · 2019-06-12T22:10:00Z

cmd/stackdriver-prometheus-sidecar/statusz.go

-	data.Uptime = fmt.Sprintf("%d hr %02d min %02d sec",
-		uptime/3600, (uptime/60)%60, uptime%60)
+	data.StartTime = serverStart.Format(time.RFC3339)
+	data.Uptime = time.Since(serverStart).String()


Nit, I'm guessing the template system will happily accept the time and duration objects, and may even provider better formatting options. It's fine if you decide to keep this as-is.

jkohen · 2019-06-12T22:14:10Z

kube/patch.sh

+          - name: POD_NAMESPACE
+            valueFrom:
+              fieldRef:
+                fieldPath: metadata.namespace


How about creating a copy the file for docs? Alternatively you could ask one of our tech writers to give a thumbs up on the larger snippet. I'd do it outside this PR, so you can submit the rest as-is.

davidbtucker

Thanks.

davidbtucker · 2019-06-17T16:54:39Z

cmd/stackdriver-prometheus-sidecar/statusz.go

-	data.Uptime = fmt.Sprintf("%d hr %02d min %02d sec",
-		uptime/3600, (uptime/60)%60, uptime%60)
+	data.StartTime = serverStart.Format(time.RFC3339)
+	data.Uptime = time.Since(serverStart).String()


davidbtucker · 2019-06-17T16:56:25Z

kube/patch.sh

+          - name: POD_NAMESPACE
+            valueFrom:
+              fieldRef:
+                fieldPath: metadata.namespace


Ok, removed these for now.

davidbtucker requested a review from StevenYCChou June 3, 2019 18:19

googlebot added the cla: yes label Jun 3, 2019

StevenYCChou reviewed Jun 3, 2019

View reviewed changes

davidbtucker commented Jun 7, 2019

View reviewed changes

jkohen suggested changes Jun 11, 2019

View reviewed changes

davidbtucker commented Jun 12, 2019

View reviewed changes

jkohen approved these changes Jun 12, 2019

View reviewed changes

davidbtucker commented Jun 17, 2019

View reviewed changes

davidbtucker added 5 commits June 17, 2019 16:15

Add statusz endpoint with basic diagnostic information.

7053fe3

Address Yen's comments.

e2ef196

Address Javier's comments.

5565f44

Address more comments.

3466379

Explicitly call .String() on log level in template.

aecdfe0

davidbtucker force-pushed the davidbtucker-statusz branch from 48745ff to aecdfe0 Compare June 17, 2019 20:20

Fix test for log level.

8e130f4

davidbtucker merged commit 3695e69 into master Jun 17, 2019

davidbtucker deleted the davidbtucker-statusz branch June 17, 2019 21:08

Add statusz endpoint with basic diagnostic information. #131

Add statusz endpoint with basic diagnostic information. #131

Conversation

davidbtucker commented Jun 3, 2019

davidbtucker left a comment

Choose a reason for hiding this comment

jkohen 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

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

davidbtucker 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

jkohen 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

davidbtucker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment