rollout: Include health report in service object #51

gvso · 2020-08-04T13:43:58Z

This adds annotation about the last health report. It uses the key rollout.cloud.run/lastHealthReport and assigns it a string with the last diagnosis (healthy, unhealthy) and the check results.

gvso · 2020-08-04T13:44:59Z

go.mod

@@ -6,7 +6,6 @@ require (
 	cloud.google.com/go v0.60.0 // indirect
 	github.com/TV4/logrus-stackdriver-formatter v0.1.0
 	github.com/go-stack/stack v1.8.0 // indirect
-	github.com/google/go-cmp v0.5.0


testify is able to compare more than primitives types, so I really didn't need go-cmp.

gvso · 2020-08-04T13:51:12Z

pkg/health/report.go

+		report := map[string]interface{}{
+			"metricsType":   criteria.Type,
+			"threshold":     result.Threshold,
+			"actualValue":   result.ActualValue,
+			"isCriteriaMet": result.IsCriteriaMet,
+		}
+		if criteria.Type == config.LatencyMetricsCheck {
+			report["percentile"] = criteria.Percentile
+		}


I also considered using the health.Diagnosis struct directly, but there are two drawbacks

We have previously decided not to include the metrics type in health.CheckResult since that would be redundant. Plus, the percentile is also not part of that struct.

The OverallDiagnosis property is a DiagnosisResult (int), so the JSON representation would turn out to be a value 0..3, which is not very helpful for our users.

gvso · 2020-08-04T16:12:36Z

Btw, in the project document, we talk about storing a history of all decisions. This makes this annotation seem redundant, but it might be useful to find why the last decision was taken faster.

If not, we can remove the lastHealthReport annotation part and keep the logic to generate an element of the history of decisions.

ahmetb · 2020-08-04T18:53:50Z

pkg/health/health.go

@@ -22,6 +22,19 @@ const (
 	Unhealthy
 )

+func (d DiagnosisResult) String() string {


+++ good way

gvso · 2020-08-05T19:24:50Z

pkg/health/report.go

+		if criteria.Type == config.LatencyMetricsCheck {
+			report += fmt.Sprintf("\n- %s[p%.0f]: %.2f (threshold %.2f)", criteria.Type, criteria.Percentile, result.ActualValue, criteria.Threshold)
+			continue
+		}
+
+		format := "\n- %s: %.2f (threshold %.2f)"
+		if criteria.Type == config.RequestCountMetricsCheck {
+			// No decimals for request count.
+			format = "\n- %s: %.0f (threshold %.0f)"
+		}


Hopefully, this is readable enough and the example in the comment make it clear. If not, any recommendations?

gvso · 2020-08-05T19:28:08Z

pkg/health/report.go

+// JSONReport returns a JSON representation of the diagnosis.
+//
+// TODO: consider if this function is useful (e.g. for a dashboard) since it's
+// not used anywhere yet.


I kept this function since we might need it eventually and the tests are a little bit "tedious" to write.

ahmetb · 2020-08-06T17:45:28Z

pkg/health/report.go

+//
+// TODO: consider if this function is useful (e.g. for a dashboard) since it's
+// not used anywhere yet.
+func JSONReport(healthCriteria []config.Metric, diagnosis Diagnosis) (string, error) {


I'd delete json for now

I deleted it. I am keeping this branch in my fork, so I have access to that work anyways.

This adds annotation about the last health report. It uses the key rollout.cloud.run/lastHealthReport and assigns it a JSON string with the last diagnosis (healthy, unhealthy) and the check results. Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>

Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>

ahmetb · 2020-08-06T21:33:36Z

pkg/health/report.go

+// The returned string has the format:
+// status: healthy
+// metrics:
+// - request-latency[p99]: 500.00 (needs 750.0)
+// - request-count: 800 (needs 1000)


this will most likely go out of date, I'd refrain from listing it here, but it's ok

ahmetb · 2020-08-06T21:34:34Z

pkg/health/report.go

+// - request-latency[p99]: 500.00 (needs 750.0)
+// - request-count: 800 (needs 1000)
+func StringReport(healthCriteria []config.Metric, diagnosis Diagnosis) string {
+	report := fmt.Sprintf("status: %s\n", diagnosis.OverallResult.String())


what if we also added the time we wrote this down?
does the user have any way of telling when was this status report for?

We could, but I'd probably have a function setHealthReportAnnotation(report string) in rollout since there are cases in which this function is not called (e.g. new candidate). In that function, we can append the time at the end of any report.

I will defer this after #57 since we would need the mocked time introduced there. Doing it here would be double work and would make this PR much longer.

ahmetb · 2020-08-06T21:36:26Z

pkg/rollout/rollout.go

@@ -114,7 +122,7 @@ func (r *Rollout) UpdateService(svc *run.Service) (*run.Service, error) {
 	if isNewCandidate(svc, candidate) {
 		r.log.Debug("new candidate, assign some traffic")
 		svc = r.PrepareRollForward(svc, stable, candidate)
-		svc = r.updateAnnotations(svc, stable, candidate)
+		svc = r.updateAnnotations(svc, stable, candidate, "new candidate, no health report available yet")


this is not a great way. it keeps overloading the updateAnnotations. I'll offer alternatives.

pass a statusMsgFunc func()string that returns the msg you wanna set.

create a new method svc = r.setAnnotation(key, msg)

(2) is cleaner.

ahmetb

some nits

Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>

ahmetb · 2020-08-07T16:11:14Z

pkg/rollout/rollout.go

 		delete(svc.Metadata.Annotations, CandidateRevisionAnnotation)
 		return svc
 	}

-	svc.Metadata.Annotations[StableRevisionAnnotation] = stable
-	svc.Metadata.Annotations[CandidateRevisionAnnotation] = candidate
+	setAnnotation(svc, StableRevisionAnnotation, stable)


nice, yeah you can just use it here :D

ahmetb · 2020-08-07T16:11:33Z

pkg/rollout/rollout.go

+		svc = r.updateAnnotations(svc, stable, candidate)
+
+		// TODO(gvso): create a setHealthReportAnnotation that appends the
+		// current time to the report before setting the annotation.


this probably could be part of the health report string generation IMO.
doesn't have to be here.

My initial thought was also this one, but the problem is that we don't generate a report when we have a new candidate. We have a string literal in that case

gvso commented Aug 4, 2020

View reviewed changes

ahmetb reviewed Aug 4, 2020

View reviewed changes

pkg/health/health.go

@@ -22,6 +22,19 @@ const (

Unhealthy

)

func (d DiagnosisResult) String() string {

Copy link

Contributor

ahmetb Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+++ good way

gvso commented Aug 5, 2020

View reviewed changes

gvso requested a review from ahmetb August 5, 2020 19:28

ahmetb reviewed Aug 6, 2020

View reviewed changes

Getulio Valentin Sánchez added 6 commits August 6, 2020 17:03

Report in human-readable format

0d42934

Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>

Add todo about JSONReport

a1f4653

Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>

Rename last status to status

4e641f1

Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>

Fix typo

b7d207b

Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>

Remove JSON report

6da8e9e

Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>

ahmetb reviewed Aug 6, 2020

View reviewed changes

ahmetb approved these changes Aug 6, 2020

View reviewed changes

Create setAnnotation function

52a0478

Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>

ahmetb reviewed Aug 7, 2020

View reviewed changes

ahmetb approved these changes Aug 7, 2020

View reviewed changes

gvso merged commit 6f36920 into GoogleCloudPlatform:main Aug 7, 2020

gvso deleted the health-annotations branch August 7, 2020 17:55

gvso mentioned this pull request Aug 7, 2020

rollout: Add updated time to health report annotation #61

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rollout: Include health report in service object #51

rollout: Include health report in service object #51

gvso commented Aug 4, 2020 •

edited

gvso Aug 4, 2020 •

edited

gvso Aug 4, 2020 •

edited

gvso commented Aug 4, 2020 •

edited

ahmetb Aug 4, 2020

gvso Aug 5, 2020

gvso Aug 5, 2020

ahmetb Aug 6, 2020

gvso Aug 6, 2020

ahmetb Aug 6, 2020

ahmetb Aug 6, 2020

gvso Aug 7, 2020

ahmetb Aug 6, 2020 •

edited

ahmetb left a comment

ahmetb Aug 7, 2020

ahmetb Aug 7, 2020 •

edited

gvso Aug 7, 2020

rollout: Include health report in service object #51

rollout: Include health report in service object #51

Conversation

gvso commented Aug 4, 2020 • edited

gvso Aug 4, 2020 • edited

Choose a reason for hiding this comment

gvso Aug 4, 2020 • edited

Choose a reason for hiding this comment

gvso commented Aug 4, 2020 • edited

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

ahmetb Aug 6, 2020 • edited

Choose a reason for hiding this comment

ahmetb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmetb Aug 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvso commented Aug 4, 2020 •

edited

gvso Aug 4, 2020 •

edited

gvso Aug 4, 2020 •

edited

gvso commented Aug 4, 2020 •

edited

ahmetb Aug 6, 2020 •

edited

ahmetb Aug 7, 2020 •

edited