feat(kaze/controllers): Implement Server sent events at regular speci… #187

karan0299 · 2020-01-05T19:57:53Z

…fied period for metrics

services/kaze/controllers/application.go

alphadose · 2020-01-05T20:04:57Z

services/kaze/controllers/application.go

+		time.Sleep(2 * configs.ServiceConfig.Mizu.MetricsInterval)
+	}()
+	c.Stream(func(w io.Writer) bool {
+		if _, ok := <-chanStream; ok {


why is the chanStream part necessary?

You can just include the time.Sleep part in this function itself

alphadose · 2020-01-05T20:05:27Z

services/kaze/controllers/application.go

+	go func() {
+		defer close(chanStream)
+		chanStream <- true
+		time.Sleep(2 * configs.ServiceConfig.Mizu.MetricsInterval)


the 2 * part is not required

2 * part was due to you reply to the question I asked in slack for time interval between two Server Sent events

My bad, it should be just configs.ServiceConfig.Mizu.MetricsInterval

alphadose · 2020-01-05T20:11:54Z

services/kaze/controllers/application.go

+			c.SSEvent("metrics", metrics)
+			return true
+		}
+		return false


the overall function will be simply

c.Stream(func(w io.Writer) bool { metrics := mongo.FetchContainerMetrics(types.M{ mongo.NameKey: appName, mongo.TimestampKey: types.M{ "$gte": time.Now().Unix() - configs.ServiceConfig.Mizu.MetricsInterval * time.Second, }, }) c.SSEvent("metrics", metrics) time.Sleep(configs.ServiceConfig.Mizu.MetricsInterval * time.Second) return true }

How is using a channel any different than the above function ^ ?

Well, as was discussed over slack, there was unknown i/o timeout error. I still am not sure about the exact reason but here what I infer from the success:

The original time.Sleep() was forcing the stream to halt somehow, but SSEvent was not prepared for discrete streams.

So by using this channel mutex triggered SSEvent, it is suppressed during the halt duration. And hence no timeout errors. I tried to mimic some of the errors by manipulating the goroutine and this inference feels rational.

gin's SSEvent implementation is buggy and we might need a replacement later.

alphadose · 2020-01-05T20:15:43Z

After doing the requested changes test your feature using this example https://www.w3schools.com/html/html5_serversentevents.asp

Also keep a track of the requests in the network tab

alphadose · 2020-02-14T05:12:13Z

services/kaze/controllers/application.go

-			"$gte": time.Now().Unix() - timeSpan,
-		},
-	})
+	count, _ := strconv.ParseInt(metricsCount, 10, 64)


You need to check for error handling here. If someone sends a string "meow" in the "fetch_count" query param then the above line will throw an error

alphadose · 2020-02-14T05:13:39Z

services/kaze/controllers/application.go

+			},
+		}, count)
+		c.SSEvent("metrics", metrics)
+		fetchInt, _ := strconv.ParseInt(metricsFetchInterval, 10, 64)


Check for error handling here too in case "metricsFetchInterval" is a string like "meow"

alphadose · 2020-02-14T05:14:56Z

services/kaze/controllers/application.go

+	metricsFetchInterval := c.Query("fetch_interval")
+	metricsCount := c.Query("fetch_count")


In case these query params are not set, you should have some default values in place.

alphadose · 2020-02-14T05:16:56Z

Also, fix your commit message as per the CONTRIBUTING.md guidelines

alphadose · 2020-02-15T13:33:35Z

services/kaze/controllers/application.go

+		if fetchIntTime < configs.ServiceConfig.Mizu.MetricsInterval {
+			fetchIntTime = 2 * fetchIntTime
+		}
+		time.Sleep(configs.ServiceConfig.Mizu.MetricsInterval * time.Second * fetchIntTime)


Is it necessary to multiply configs.ServiceConfig.Mizu.MetricsInterval and fetchIntTime in this context ?

alphadose · 2020-02-15T13:38:43Z

services/kaze/controllers/application.go

+		if fetchIntTime < configs.ServiceConfig.Mizu.MetricsInterval {
+			fetchIntTime = 2 * fetchIntTime


You should check for this condition outside c.Stream.
Also to retain safety you can change fetchIntTime = 2 * fetchIntTime to fetchIntTime = 2 * configs.ServiceConfig.Mizu.MetricsInterval

alphadose · 2020-02-15T13:44:03Z

services/kaze/controllers/application.go

+	metricsFetchInterval := c.Query("fetch_interval")
+	fetchInt, err := strconv.ParseInt(metricsFetchInterval, 10, 64)


the line metricsFetchInterval := c.Query("fetch_interval") is unnecessary
You can just do fetchInt, err := strconv.ParseInt(c.Query("fetch_interval"), 10, 64)

Doing this might prevent an extra register being used unnecessarily in case the compiler's assembly optimization was not good enough

Same for the "fetch_counts" part

alphadose · 2020-02-15T13:45:06Z

services/kaze/controllers/application.go

@@ -228,29 +230,36 @@ func TransferApplicationOwnership(c *gin.Context) {

 // FetchMetrics retrieves the metrics of an application's container
 func FetchMetrics(c *gin.Context) {
+	c.Writer.Header().Set("Content-Type", "text/event-stream")


this line is unnecessary as the response headers are automatically set during c.SSEvent

@supra08 fix this

alphadose · 2020-02-15T13:47:29Z

Also you can rename "fetch_interval" and "fetch_count" to just "interval" and "count" respectively. That way it is more readable and easier to use from the perspective of an API user.

alphadose · 2020-02-17T20:15:00Z

services/kaze/controllers/application.go

+		c.AbortWithStatusJSON(400, gin.H{
+			"success": false,
+			"error":   "App name not provided for metrics",
+		})


a return statement is required after this to terminate the function

Also you can leave the check of appName == "".

This is already handled by the IsAppOwner middleware. You cannot create an app with an empty string as name. This rule is already enforced.

@supra08 remove the check for appName == ""

services/mizu/monitor.go

alphadose · 2020-04-04T11:01:37Z

services/kaze/controllers/application.go

@@ -228,29 +230,36 @@ func TransferApplicationOwnership(c *gin.Context) {

 // FetchMetrics retrieves the metrics of an application's container
 func FetchMetrics(c *gin.Context) {
+	c.Writer.Header().Set("Content-Type", "text/event-stream")


@supra08 fix this

alphadose · 2020-04-04T11:02:02Z

services/kaze/controllers/application.go

+		c.AbortWithStatusJSON(400, gin.H{
+			"success": false,
+			"error":   "App name not provided for metrics",
+		})


@supra08 remove the check for appName == ""

alphadose · 2020-04-04T11:03:07Z

services/kaze/controllers/application.go

+			c.SSEvent("metrics", metrics)
+			return true
+		}
+		return false


How is using a channel any different than the above function ^ ?

…fied period for metrics

…and fallback default values

…metrics in container unavailability

…le broken streams

… checks and headers

…to provide static time series data

…imit specified

supra08 · 2020-04-05T23:17:01Z

@alphadose

…ration

codecov-io · 2020-04-07T03:01:13Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@7135437). Click here to learn what that means.
The diff coverage is n/a.

karan0299 requested a review from alphadose January 5, 2020 19:58

alphadose requested changes Jan 5, 2020

View reviewed changes

alphadose requested changes Feb 14, 2020

View reviewed changes

supra08 force-pushed the implement-SSE branch from cf9cd4f to 3b8a764 Compare February 15, 2020 11:39

alphadose reviewed Feb 15, 2020

View reviewed changes

alphadose reviewed Feb 17, 2020

View reviewed changes

services/mizu/monitor.go Show resolved Hide resolved

alphadose requested changes Apr 4, 2020

View reviewed changes

karan0299 and others added 8 commits April 4, 2020 17:39

feat(kaze/controllers): Implement Server sent events at regular speci…

5d3bb7c

…fied period for metrics

feat(*): enhance metrics streamer with new options

f03eae5

fix(services/.../application.go): error handing for query parameters …

2694fa2

…and fallback default values

fix(services/.../application.go): fix error handling gracefully

cedab06

fix(services/.../monitor.go): implement temporary solution to handle …

fd39360

…metrics in container unavailability

fix(services/.../application.go): add return statement

20d6980

fix(services/kaze/controllers/application.go): use goroutines to hand…

e58d276

…le broken streams

cleanup(services/kaze/controllers/application.go): remove unnecessary…

f7bbdc0

… checks and headers

supra08 force-pushed the implement-SSE branch from 73e278b to f7bbdc0 Compare April 4, 2020 12:10

supra08 added 2 commits April 6, 2020 04:37

feat(services/kaze/controllers/application.go): convert FetchMetrics …

ab9aa2f

…to provide static time series data

feat(*): modify mongo FetchContainerMetrics to handle cases without l…

c50744b

…imit specified

alphadose added 2 commits April 7, 2020 08:28

refactor(lib/mongo/read.go): remove redundant options parameter decla…

d0ff30a

…ration

refactor(lib/docker/container.go): lint file

a1e0924

alphadose approved these changes Apr 7, 2020

View reviewed changes

alphadose merged commit 1b12e4c into develop Apr 7, 2020

alphadose deleted the implement-SSE branch April 7, 2020 03:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(kaze/controllers): Implement Server sent events at regular speci… #187

feat(kaze/controllers): Implement Server sent events at regular speci… #187

karan0299 commented Jan 5, 2020

alphadose Jan 5, 2020

alphadose Jan 5, 2020

karan0299 Jan 6, 2020

alphadose Jan 6, 2020

alphadose Jan 5, 2020

alphadose Apr 4, 2020

supra08 Apr 4, 2020

alphadose commented Jan 5, 2020

alphadose Feb 14, 2020

alphadose Feb 14, 2020

alphadose Feb 14, 2020

alphadose commented Feb 14, 2020

alphadose Feb 15, 2020

alphadose Feb 15, 2020 •

edited

Loading

alphadose Feb 15, 2020

alphadose Feb 15, 2020

alphadose Apr 4, 2020

supra08 Apr 4, 2020

alphadose commented Feb 15, 2020

alphadose Feb 17, 2020

alphadose Feb 17, 2020

alphadose Apr 4, 2020

supra08 Apr 4, 2020

alphadose Apr 4, 2020

alphadose Apr 4, 2020

alphadose Apr 4, 2020

supra08 commented Apr 5, 2020

codecov-io commented Apr 7, 2020

		metricsFetchInterval := c.Query("fetch_interval")
		metricsCount := c.Query("fetch_count")

		if fetchIntTime < configs.ServiceConfig.Mizu.MetricsInterval {
		fetchIntTime = 2 * fetchIntTime

		metricsFetchInterval := c.Query("fetch_interval")
		fetchInt, err := strconv.ParseInt(metricsFetchInterval, 10, 64)

feat(kaze/controllers): Implement Server sent events at regular speci… #187

feat(kaze/controllers): Implement Server sent events at regular speci… #187

Conversation

karan0299 commented Jan 5, 2020

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

alphadose commented Jan 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alphadose commented Feb 14, 2020

Choose a reason for hiding this comment

alphadose Feb 15, 2020 • 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

alphadose commented Feb 15, 2020

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

supra08 commented Apr 5, 2020

codecov-io commented Apr 7, 2020

Codecov Report

alphadose Feb 15, 2020 •

edited

Loading