MF-1308 - Use IETF Health Check standard #1541

manuio · 2022-01-10T21:25:10Z

Signed-off-by: Manuel Imperiale [email protected]

Resolves #1308

Resolved in UI repo here: mainflux/ui#233
Resolved in devops repo here: absmach/devops#97
Resolved in docs repo here: absmach/magistrala-docs#119

Since we wanted to have information about the build time and the corresponding commit hash of the version we added build_time and commit fields. However these fields are non standard and we should decide what to do about in this issue: https://github.com/mainflux/mainflux/issues/1549

Signed-off-by: Manuel Imperiale <[email protected]>

drasko · 2022-01-10T22:12:54Z

I like this implementation. For now it looks maybe redundant, as version was functioning well, but I think in the future we will add some of the fields. For example, I think that uptime would be useful. Also, I find it more logical that we test status of the service via /health endpoint, rather than via /version endpoint.

On the other hand, I would like to hear from @nmarcetic and @dusanb94 does this change add the value, or maybe complicates things without particular benefit. @mteodor and @blokovi also - what do you think?

drasko

We should update Swagger YAML files as well

drasko · 2022-01-10T21:55:21Z

consumers/writers/api/transport.go

@@ -1,6 +1,7 @@
 // Copyright (c) Mainflux
 // SPDX-License-Identifier: Apache-2.0

+//go:build !test


Why do we have both these lines. Are they necessary?

I don't know why, it was done by the editor. I removed this one and all others.

Signed-off-by: Manuel Imperiale <[email protected]>

manuio · 2022-01-11T09:09:03Z

I like this implementation. For now it looks maybe redundant, as version was functioning well, but I think in the future we will add some of the fields. For example, I think that uptime would be useful. Also, I find it more logical that we test status of the service via /health endpoint, rather than via /version endpoint.

On the other hand, I would like to hear from @nmarcetic and @dusanb94 does this change add the value, or maybe complicates things without particular benefit. @mteodor and @blokovi also - what do you think?

@drasko I checked the code of the repo that you proposed and I prefered to use it as example than add it in the vendor. We were almost using nothing from there and the standard is not so complicated to follow.

Signed-off-by: Manuel Imperiale <[email protected]>

blokovi

Just for reference, we should see if this can be used in Dockerfile for HEALTCHECK instruction:
https://docs.docker.com/engine/reference/builder/#healthcheck

We already use this instruction in VerneMQ Dockerfile

Signed-off-by: Manuel Imperiale <[email protected]>

codecov-commenter · 2022-01-11T12:28:24Z

Codecov Report

Merging #1541 (42de5ef) into master (bcc8cf7) will increase coverage by 1.61%.
The diff coverage is 60.86%.

@@            Coverage Diff             @@
##           master    #1541      +/-   ##
==========================================
+ Coverage   67.47%   69.08%   +1.61%     
==========================================
  Files         139      134       -5     
  Lines       11321    11057     -264     
==========================================
  Hits         7639     7639              
+ Misses       3051     2787     -264     
  Partials      631      631

Impacted Files	Coverage Δ
pkg/sdk/go/sdk.go	`100.00% <ø> (ø)`
provision/api/transport.go	`0.00% <0.00%> (ø)`
pkg/sdk/go/health.go	`46.66% <46.66%> (ø)`
bootstrap/api/transport.go	`93.10% <100.00%> (ø)`
consumers/notifiers/api/transport.go	`87.62% <100.00%> (ø)`
http/api/transport.go	`75.00% <100.00%> (ø)`
readers/api/transport.go	`78.12% <100.00%> (ø)`
things/api/things/http/transport.go	`85.30% <100.00%> (ø)`
twins/api/http/transport.go	`92.48% <100.00%> (ø)`
users/api/transport.go	`66.99% <100.00%> (ø)`
... and 5 more

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 bcc8cf7...42de5ef. Read the comment docs.

drasko · 2022-01-11T14:37:09Z

Just for reference, we should see if this can be used in Dockerfile for HEALTCHECK instruction: https://docs.docker.com/engine/reference/builder/#healthcheck

We already use this instruction in VerneMQ Dockerfile

I find this very interesting. Do we have to change our Dockerfile for this?

What is K8s looking for in the response - only status code?

blokovi · 2022-01-11T15:37:12Z

I find this very interesting. Do we have to change our Dockerfile for this?

What is K8s looking for in the response - only status code?

@drasko

Yes, change in Dockerfile is needed.

That Docker healthcheck has not been integrated in the Kubernetes, there we can use Liveness and readiness probes

dborovcanin · 2022-01-12T12:46:01Z

http/api/logging.go

-// +build !test
-


Please don't remove test tags.

Ok but can you explain me why this tags are not everywhere? For what this is needed?

Replaced by the new one //go:build

Ok but can you explain me why this tags are not everywhere? For what this is needed?

It is used to ignore logging and metrics for testing (coverage included) - it is used here: https://github.com/mainflux/mainflux/blob/6483969927adb71736b8d4ed7553f8c2988973ce/scripts/ci.sh#L98

Signed-off-by: Manuel Imperiale <[email protected]>

dborovcanin · 2022-01-13T11:07:32Z

pkg/sdk/go/health_test.go

@@ -24,14 +24,14 @@ func TestVersion(t *testing.T) {
 		empty bool
 		err   error
 	}{
-		"get version": {
+		"get things service version": {


Please check all the response fields, not only Version.

Signed-off-by: Manuel Imperiale <[email protected]>

arvindh123 · 2022-01-19T17:19:58Z

health.go

+
+const (
+	contentType     = "Content-Type"
+	contentTypeJSON = "application/json"


Content type can be
application/health+json
as mentioned in https://datatracker.ietf.org/doc/html/draft-inadarei-api-health-check-05#section-3
https://datatracker.ietf.org/doc/html/draft-inadarei-api-health-check-05#section-7

Signed-off-by: Manuel Imperiale <[email protected]>

arvindh123 · 2022-01-19T18:27:44Z

In this PR we have only the version of service and do not have any other health check status or metrics like uptime, CPU usage , other component status.

Example Output : https://tools.ietf.org/id/draft-inadarei-api-health-check-04.html#rfc.section.5

It seems to be similar to the version endpoint

manuio · 2022-01-19T18:52:35Z

In this PR we have only the version of service and do not have any other health check status or metrics like uptime, CPU usage , other component status.

Example Output : https://tools.ietf.org/id/draft-inadarei-api-health-check-04.html#rfc.section.5

It seems to be similar to the version endpoint

@arvindh123 Indeed, for now we only have 3 standard fields: status, version, and description. build_time and commit are custom fields. I didn't want to add it in this PR but we can maybe open another issue to talk about this check field. However, as previously discussed maybe this can be solved with docker.

drasko · 2022-01-19T22:47:19Z

Makefile

@@ -23,6 +28,9 @@ define make_docker
 		--build-arg SVC=$(svc) \
 		--build-arg GOARCH=$(GOARCH) \
 		--build-arg GOARM=$(GOARM) \
+		--build-arg VERSION=$(shell git describe --abbrev=0 --tags) \


Please check if you need to change our Dockerfile as well, as it is alpine go, maybe you need to install Git into it (i.e. try to build Docker images, see if compilation will work).

@drasko Compilation work, I already tested. There is no need to install git in the Dockerfile since you pass this values as arguments.

dborovcanin · 2022-01-20T09:32:49Z

Makefile

+	-X 'github.com/mainflux/mainflux.BuildTime=$(TIME)' \
+	-X 'github.com/mainflux/mainflux.Version=$(VERSION)' \
+	-X 'github.com/mainflux/mainflux.Commit=$(COMMIT)'" \


By default, these (TIME, VERSION, COMMIT) will be empty. Please set them like in make_dockers.

I we do so we have to install git in the Dockerfile. Maybe it's more understandable to do it like that:

go build -mod=vendor -ldflags "-s -w \ -X 'github.com/mainflux/mainflux.BuildTime=$(6)' \ -X 'github.com/mainflux/mainflux.Version=$(4)' \ -X 'github.com/mainflux/mainflux.Commit=$(5)'" \ -o ${BUILD_DIR}/mainflux-$(1) cmd/$(1)/main.go

No need for that. You can set it as variables in the Makefile like so:

VERSION ?= $(shell git describe --abbrev=0 --tags) COMMIT ?= $(shell git rev-parse HEAD) TIME ?= $(shell date +%F_%T)

The ?= will take care to set only if variables are empty, which will be the case for the local build, and won't be the case for Docker build.

But I think we need those filled in the Docker build as well, otherwise /health will not work correctly for our Docker images, right?

Also, maybe it's better to have for TIME something like date --rfc-3339=seconds rather than a custom format.

@dusanb94 Let me try what you proposed. It's probably the way to go.
@drasko It is filled in the docker build. Here:

--build-arg VERSION=$(VERSION) \ --build-arg COMMIT=$(COMMIT) \ --build-arg TIME=$(TIME) \

and here:

ARG VERSION ARG COMMIT ARG TIME

Signed-off-by: Manuel Imperiale <[email protected]>

drasko · 2022-01-20T13:29:02Z

api/schemas/HealthInfo.yml

+    type: string
+    description: Service version.
+    example: 0.0.1
+  commit:


We cannot leave commit. We must adhere to standard. If you have fields that are missing, we probably must work with standardization body and argument why we need those, so that they can rectify in the next version pf the draft. Otherwise please find the closest field and put commit information there (probably releaseId).

ok, where I put build_time?

That's a good question. Let me check.

@manuio I have sent an e-mail to the author. Maybe we'll have to open an issue here as well. Let's give it a day or two, and if we have no answers, I will merge the current PR.

I have opened an issue as well: inadarei/rfc-healthcheck#76

drasko · 2022-01-22T23:00:30Z

health.go

+	// Version contains current service version.
+	Version string `json:"version"`
+
+	// Commit represents the service build time.


This is not correct. It is a Git commit hash.

drasko · 2022-01-24T19:12:49Z

health.go

+	// Version contains current service version.
+	Version string `json:"version"`
+
+	// Commit represents the service build time.


Signed-off-by: Manuel Imperiale <[email protected]>

drasko

LGTM

drasko · 2022-01-24T20:22:18Z

@manuio @dusanb94 I have merged this PR, I think it is very useful and that we have it. Once @inadarei or someone else reacts on inadarei/rfc-healthcheck#76 we'll see how to progress with https://github.com/mainflux/mainflux/issues/1549

* MF-1308 - Use IETF Health Check standard Signed-off-by: Manuel Imperiale <[email protected]> * Add nginx health endpoint Signed-off-by: Manuel Imperiale <[email protected]> * Rm github.com/nelkinda dependency Signed-off-by: Manuel Imperiale <[email protected]> * Check error Signed-off-by: Manuel Imperiale <[email protected]> * Replace Version by Health in the CLI and SDK Signed-off-by: Manuel Imperiale <[email protected]> * Fix typo Signed-off-by: Manuel Imperiale <[email protected]> * Use new build flag go:build Signed-off-by: Manuel Imperiale <[email protected]> * Revert wrong renaming Signed-off-by: Manuel Imperiale <[email protected]> * sdk health test Signed-off-by: Manuel Imperiale <[email protected]> * Add /health endpoint to openapi doc Signed-off-by: Manuel Imperiale <[email protected]> * Use const for description message Signed-off-by: Manuel Imperiale <[email protected]> * Add version and build time during build Signed-off-by: Manuel Imperiale <[email protected]> * Time format Signed-off-by: Manuel Imperiale <[email protected]> * Add version and commit using git and build args Signed-off-by: Manuel Imperiale <[email protected]> * Add comments Signed-off-by: Manuel Imperiale <[email protected]> * Add tests Signed-off-by: Manuel Imperiale <[email protected]> * Add missing api properties Signed-off-by: Manuel Imperiale <[email protected]> * Fix api Signed-off-by: Manuel Imperiale <[email protected]> * Use ./schemas/HealthInfo.yml as Signed-off-by: Manuel Imperiale <[email protected]> * Fix example Signed-off-by: Manuel Imperiale <[email protected]> * Use content type application/health+json Signed-off-by: Manuel Imperiale <[email protected]> * Set Makefile variables only if empty Signed-off-by: Manuel Imperiale <[email protected]> * Fix typo Signed-off-by: Manuel Imperiale <[email protected]> Signed-off-by: skovacevic <[email protected]>

MF-1308 - Use IETF Health Check standard

b91a6ce

Signed-off-by: Manuel Imperiale <[email protected]>

manuio requested a review from a team as a code owner January 10, 2022 21:25

Add nginx health endpoint

a12d4fc

Signed-off-by: Manuel Imperiale <[email protected]>

drasko requested review from dborovcanin, mteodor, nmarcetic and blokovi January 10, 2022 22:13

drasko requested changes Jan 10, 2022

View reviewed changes

Rm github.com/nelkinda dependency

c7f534b

Signed-off-by: Manuel Imperiale <[email protected]>

Check error

794e871

Signed-off-by: Manuel Imperiale <[email protected]>

blokovi previously approved these changes Jan 11, 2022

View reviewed changes

Replace Version by Health in the CLI and SDK

247d5b4

Signed-off-by: Manuel Imperiale <[email protected]>

manuio dismissed blokovi’s stale review via 247d5b4 January 11, 2022 12:20

Fix typo

f439983

Signed-off-by: Manuel Imperiale <[email protected]>

blokovi previously approved these changes Jan 11, 2022

View reviewed changes

dborovcanin mentioned this pull request Jan 12, 2022

Consider using service healthcheck in Dockerfile #1543

Closed

dborovcanin requested changes Jan 12, 2022

View reviewed changes

Use new build flag go:build

24e8cf5

Signed-off-by: Manuel Imperiale <[email protected]>

manuio dismissed blokovi’s stale review via 24e8cf5 January 13, 2022 10:39

manuio added 2 commits January 13, 2022 11:41

Fix conflicts

775dd7c

Signed-off-by: Manuel Imperiale <[email protected]>

Revert wrong renaming

848982a

Signed-off-by: Manuel Imperiale <[email protected]>

dborovcanin requested changes Jan 13, 2022

View reviewed changes

sdk health test

4c06dee

Signed-off-by: Manuel Imperiale <[email protected]>

dborovcanin previously approved these changes Jan 13, 2022

View reviewed changes

Add /health endpoint to openapi doc

ecdefa1

Signed-off-by: Manuel Imperiale <[email protected]>

manuio dismissed blokovi’s stale review via 3517aeb January 19, 2022 12:20

manuio and others added 2 commits January 19, 2022 13:23

Fix example

a4b7c0f

Signed-off-by: Manuel Imperiale <[email protected]>

Merge branch 'master' into health

36727bb

manuio requested review from drasko, blokovi and mteodor January 19, 2022 17:11

arvindh123 reviewed Jan 19, 2022

View reviewed changes

manuio added 2 commits January 19, 2022 18:23

Use content type application/health+json

2cc816a

Signed-off-by: Manuel Imperiale <[email protected]>

Merge branch 'health' of http://github.com/manuio/mainflux into health

d47bdc6

drasko requested changes Jan 19, 2022

View reviewed changes

dborovcanin requested changes Jan 20, 2022

View reviewed changes

Set Makefile variables only if empty

d3c3638

Signed-off-by: Manuel Imperiale <[email protected]>

dborovcanin previously approved these changes Jan 20, 2022

View reviewed changes

drasko requested changes Jan 20, 2022

View reviewed changes

drasko mentioned this pull request Jan 20, 2022

Git commit hash and service build time information inadarei/rfc-healthcheck#76

Open

mteodor previously approved these changes Jan 21, 2022

View reviewed changes

drasko requested changes Jan 22, 2022

View reviewed changes

drasko requested changes Jan 24, 2022

View reviewed changes

Fix typo

42de5ef

Signed-off-by: Manuel Imperiale <[email protected]>

manuio dismissed stale reviews from mteodor and dborovcanin via 42de5ef January 24, 2022 19:47

drasko approved these changes Jan 24, 2022

View reviewed changes

drasko merged commit 42dd813 into absmach:master Jan 24, 2022

drasko mentioned this pull request Jan 24, 2022

NOISSUE - Change endpoint /version by /health absmach/magistrala-docs#119

Merged

manuio deleted the health branch February 23, 2022 12:38

chombium mentioned this pull request Mar 15, 2022

Fallback comment "//+build !test" missing for "//go:build !test" for Go versions v.16.x #1577

Closed

MF-1308 - Use IETF Health Check standard #1541

MF-1308 - Use IETF Health Check standard #1541

Conversation

manuio commented Jan 10, 2022 • edited Loading

drasko commented Jan 10, 2022

drasko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuio commented Jan 11, 2022 • edited Loading

blokovi left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 11, 2022 • edited Loading

Codecov Report

drasko commented Jan 11, 2022

blokovi commented Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuio Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvindh123 Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvindh123 commented Jan 19, 2022

manuio commented Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuio Jan 20, 2022 • 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

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

drasko left a comment

Choose a reason for hiding this comment

drasko commented Jan 24, 2022

manuio commented Jan 10, 2022 •

edited

Loading

manuio commented Jan 11, 2022 •

edited

Loading

codecov-commenter commented Jan 11, 2022 •

edited

Loading

blokovi commented Jan 11, 2022 •

edited

Loading

manuio Jan 13, 2022 •

edited

Loading

arvindh123 Jan 19, 2022 •

edited

Loading

manuio commented Jan 19, 2022 •

edited

Loading

manuio Jan 20, 2022 •

edited

Loading