Return info msg for `/health` endpoint #1465

stefanvassilev · 2019-04-08T15:22:39Z

Short description of the changes

Return 200 OK for status ready
Return Json containing time startedAt and upTime statistics

Resolves #1450

black-adder · 2019-04-08T15:41:04Z

pkg/healthcheck/handler.go

+			},
+			Ready: {
+				statusCode: http.StatusOK,
+				StatusMsg:  "up",


perhaps Server available

black-adder · 2019-04-08T15:43:21Z

pkg/healthcheck/internal_test.go

+	assert.Equal(t, hc.upTimeStats.UpTime, hr.upTimeStats.UpTime)
+	assert.True(t, hc.upTimeStats.StartedAt.Equal(hr.upTimeStats.StartedAt))
+
+	time.Sleep(300)


this will add an extra 5 minutes for each test run which is overkill

Those are Nanosecond. I wanted to check the case where some time's passed and the service went down, that the uptime does not get incremented.

black-adder · 2019-04-08T15:43:38Z

pkg/healthcheck/handler.go

 	})
 }

+func (hc *HealthCheck) createRespBody(resp healthCheckResponse) []byte {
+


black-adder · 2019-04-08T15:44:33Z

pkg/healthcheck/handler.go

-			Unavailable: http.StatusServiceUnavailable,
-			Ready:       http.StatusNoContent,
+		upTimeStats: upTimeStats{
+			StartedAt: time.Now(),


technically, shouldn't this be set when the sever is first set to ready?

codecov · 2019-04-08T16:09:04Z

Codecov Report

Merging #1465 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1465      +/-   ##
==========================================
+ Coverage   99.82%   99.82%   +<.01%     
==========================================
  Files         173      173              
  Lines        8179     8203      +24     
==========================================
+ Hits         8165     8189      +24     
  Misses          7        7              
  Partials        7        7

Impacted Files	Coverage Δ
pkg/healthcheck/handler.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48555a8...b84001d. Read the comment docs.

jpkrohling · 2019-04-09T10:27:24Z

crossdock/main.go

@@ -105,7 +105,7 @@ func (h *clientHandler) isInitialized() bool {
 func httpHealthCheck(logger *zap.Logger, service, healthURL string) {
 	for i := 0; i < 240; i++ {
 		res, err := http.Get(healthURL)
-		if err == nil && res.StatusCode == 204 {
+		if err == nil && res.StatusCode == 200 {


I'd say that the most appropriate is to check if the response is a 2xx class. That's what monitoring solutions do.

jpkrohling · 2019-04-09T10:34:17Z

pkg/healthcheck/handler.go

@@ -94,5 +136,8 @@ func (hc *HealthCheck) Get() Status {

 // Ready is a shortcut for Set(Ready) (kept for backwards compatibility)
 func (hc *HealthCheck) Ready() {
+	hc.upTimeStats = upTimeStats{
+		StartedAt: time.Now(),


The process' start time is recorded as a metric:

# HELP process_start_time_seconds Start time of the process since unix epoch in seconds. # TYPE process_start_time_seconds gauge process_start_time_seconds 1.55480578686e+09

Should we add this one as well?

Should I add this as well? This would introduce a new dependency to https://github.com/shirou/gopsutil as well

I meant the other way around, to add the "Started At" as a metric.

Where should I add it?

Looks like the handler isn't receiving a metrics object. Perhaps it's better to open a follow-up issue for now.

Perhaps a follow-up issue of what should be done would be more helpful, as on my local build the /metrics endpoint does not return any process information. :)

Return 200 OK for status `ready` and `upTimeStats` Resolves jaegertracing#1450 Signed-off-by: stefan vassilev <stefanvassilev1@gmail.com>

Signed-off-by: stefan vassilev <stefanvassilev1@gmail.com>

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro · 2019-04-13T16:19:50Z

pkg/healthcheck/handler.go

@@ -46,23 +48,43 @@ func (s Status) String() string {
 	}
 }

+type upTimeStats struct {
+	StartedAt time.Time `json:"startedAt"`
+	UpTime    string    `json:"upTime"`


uptime is one word, no need to capitalize T (fields and struct name)

yurishkuro · 2019-04-13T16:31:29Z

pkg/healthcheck/handler.go

@@ -94,5 +136,8 @@ func (hc *HealthCheck) Get() Status {

 // Ready is a shortcut for Set(Ready) (kept for backwards compatibility)
 func (hc *HealthCheck) Ready() {
+	hc.upTimeStats = upTimeStats{


race condition

yurishkuro · 2019-04-13T18:58:44Z

Thanks for the PR!

Return info msg for `/health` endpoint (jaegertracing#1465)

stefanvassilev requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners April 8, 2019 15:22

stefanvassilev force-pushed the healthcheck-verbose branch from 1640594 to e4e2c29 Compare April 8, 2019 15:24

black-adder reviewed Apr 8, 2019

View reviewed changes

jpkrohling reviewed Apr 9, 2019

View reviewed changes

vprithvi mentioned this pull request Apr 9, 2019

Jaeger operational observability #1054

Open

stefanvassilev force-pushed the healthcheck-verbose branch from a00d1bd to 0ea4d95 Compare April 10, 2019 21:51

stefanvassilev added 4 commits April 10, 2019 23:55

Return info msg for /health endpoint

15f6ca8

Return 200 OK for status `ready` and `upTimeStats` Resolves jaegertracing#1450 Signed-off-by: stefan vassilev <stefanvassilev1@gmail.com>

Address PR comments

eb4c8bc

Signed-off-by: stefan vassilev <stefanvassilev1@gmail.com>

Fix failing test

b23e102

Signed-off-by: stefan vassilev <stefanvassilev1@gmail.com>

Address PR comments

fc3fbe8

Signed-off-by: stefan vassilev <stefanvassilev1@gmail.com>

stefanvassilev force-pushed the healthcheck-verbose branch from 0ea4d95 to fc3fbe8 Compare April 10, 2019 21:56

stefanvassilev and others added 3 commits April 11, 2019 23:40

Retrigger tests

c04713d

Signed-off-by: stefan vassilev <stefanvassilev1@gmail.com>

Retrigger tests

c8603a4

Signed-off-by: stefan vassilev <stefanvassilev1@gmail.com>

Make thread-safe

b84001d

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro approved these changes Apr 13, 2019

View reviewed changes

yurishkuro merged commit 54416c6 into jaegertracing:master Apr 13, 2019

code98 added a commit to code98/jaeger that referenced this pull request Apr 14, 2019

Merge pull request #1 from jaegertracing/master

bcf9222

Return info msg for `/health` endpoint (jaegertracing#1465)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return info msg for `/health` endpoint #1465

Return info msg for `/health` endpoint #1465

stefanvassilev commented Apr 8, 2019

black-adder Apr 8, 2019

black-adder Apr 8, 2019

stefanvassilev Apr 8, 2019

black-adder Apr 8, 2019

black-adder Apr 8, 2019

codecov bot commented Apr 8, 2019 •

edited

Loading

jpkrohling Apr 9, 2019

jpkrohling Apr 9, 2019

stefanvassilev Apr 9, 2019 •

edited

Loading

jpkrohling Apr 9, 2019

stefanvassilev Apr 9, 2019

jpkrohling Apr 10, 2019

stefanvassilev Apr 10, 2019

yurishkuro Apr 13, 2019

yurishkuro Apr 13, 2019

yurishkuro commented Apr 13, 2019

Return info msg for /health endpoint #1465

Return info msg for /health endpoint #1465

Conversation

stefanvassilev commented Apr 8, 2019

Short description of the changes

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

codecov bot commented Apr 8, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanvassilev Apr 9, 2019 • edited Loading

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

yurishkuro commented Apr 13, 2019

Return info msg for `/health` endpoint #1465

Return info msg for `/health` endpoint #1465

codecov bot commented Apr 8, 2019 •

edited

Loading

stefanvassilev Apr 9, 2019 •

edited

Loading