Replay recorded requests by ReqUID #864

Amusement · 2018-08-15T23:48:13Z

This change is

life1347 · 2018-08-16T08:39:16Z

controller/api.go

@@ -79,6 +80,13 @@ func MakeAPI() (*API, error) {
 		api.builderManagerUrl = "http://buildermgr"
 	}

+	u = os.Getenv("POD_NAMESPACE")


In fact, we already did this at L40. You can remove this
https://github.com/fission/fission/pull/864/files?utf8=%E2%9C%93&diff=split#diff-f00d7ac7727a4b9d850826c85fb27520R40

life1347 · 2018-08-16T08:40:33Z

controller/api.go

@@ -50,6 +50,7 @@ type (
 		kubernetesClient  *kubernetes.Clientset
 		storageServiceUrl string
 		builderManagerUrl string
+		podNamespace      string


In same package you can access podNamespace directly without putting it int the structure.

life1347 · 2018-08-16T08:42:14Z

controller/client/replayer.go

@@ -0,0 +1,34 @@
+package client


Add License here and other files

life1347 · 2018-08-16T08:43:05Z

controller/replayAPI.go

+	vars := mux.Vars(r)
+	queriedID := vars["reqUID"]
+
+	routerUrl := fmt.Sprintf("http://router.%v", a.podNamespace)


You can access podNamespace directly

life1347 · 2018-08-16T08:46:19Z

fission/main.go

@@ -225,6 +225,9 @@ func main() {
 		{Name: "view", Usage: "View existing records", Flags: []cli.Flag{filterTimeTo, filterTimeFrom, filterFunction, filterTrigger, verbosityFlag, vvFlag}, Action: recordsView},
 	}

+	// Replay records
+	reqIDFlag := cli.StringFlag{Name: "reqUID", Usage: "Replay a particular request by providing the reqUID (to view reqUIDs, do `fission records view`)"}


Could we use lower case flag requid here?

life1347 · 2018-08-16T08:47:19Z

redis/redisApi.go

+func ReplayByReqUID(routerUrl string, queriedID string) ([]byte, error) {
+	client := NewClient()
+	if client == nil {
+		return []byte{}, errors.New("failed to create redis client")


You can return nil for empty slice like this return nil, err...

life1347 · 2018-08-16T08:59:37Z

replayer/replay.go

+	"github.com/fission/fission/redis/build/gen"
+)
+
+func ReplayRequest(routerUrl string, request *redisCache.Request) ([]string, error) {


Any reason to put this function here? Looks like it's only being in redis/redisApi.go, can we put this the function there as well or redis/replayer/replay.go?

life1347 · 2018-08-16T09:28:11Z

replayer/replay.go

+
+func ReplayRequest(routerUrl string, request *redisCache.Request) ([]string, error) {
+	path := request.URL["Path"] // Includes slash prefix
+	payload := request.URL["Payload"]


I suddenly found some problem I didn't notice for the record PR. You file an issue to address these problem later.

Lost some of the structure fields in original request.
For example, RawPath in url is not included.
https://golang.org/src/net/url/url.go?s=9021:9537#L323

type URL struct { Scheme string Opaque string // encoded opaque data User *Userinfo // username and password information Host string // host or host:port Path string // path (relative paths may omit leading slash) RawPath string // encoded path hint (see EscapedPath method) ForceQuery bool // append a query ('?') even if RawQuery is empty RawQuery string // encoded query values, without '?' Fragment string // fragment for references, without '#' }

We better to embed the whole url.Url object in the redisCache.Request directly.
Also, we lost some of fields in the original request structure like RemoteAddr Trailer . https://github.com/golang/go/blob/master/src/net/http/request.go#L106-L320

Payload should be in redisCache.Request not redisCache.Request.URL

Amusement

Reviewable status: 0 of 7 files reviewed, 8 unresolved discussions (waiting on @life1347 and @Amusement)

controller/api.go, line 53 at r1 (raw file):

Previously, life1347 (Ta-Ching Chen) wrote…

In same package you can access podNamespace directly without putting it int the structure.

Cool tip 👍

controller/api.go, line 83 at r1 (raw file):

Previously, life1347 (Ta-Ching Chen) wrote…

In fact, we already did this at L40. You can remove this
https://github.com/fission/fission/pull/864/files?utf8=%E2%9C%93&diff=split#diff-f00d7ac7727a4b9d850826c85fb27520R40

Done.

controller/replayAPI.go, line 16 at r1 (raw file):

Previously, life1347 (Ta-Ching Chen) wrote…

You can access podNamespace directly

Done.

controller/client/replayer.go, line 1 at r1 (raw file):

Previously, life1347 (Ta-Ching Chen) wrote…

Add License here and other files

Done.

fission/main.go, line 229 at r1 (raw file):

Previously, life1347 (Ta-Ching Chen) wrote…

Could we use lower case flag re 8000 quid here?

Done.

redis/redisApi.go, line 344 at r1 (raw file):

Previously, life1347 (Ta-Ching Chen) wrote…

You can return nil for empty slice like this return nil, err...

Done.

replayer/replay.go, line 13 at r1 (raw file):

Previously, life1347 (Ta-Ching Chen) wrote…

Any reason to put this function here? Looks like it's only being in redis/redisApi.go, can we put this the function there as well or redis/replayer/replay.go?

Good point. Moved it to redis/redisApi.go.

replayer/replay.go, line 15 at r1 (raw file):

Previously, life1347 (Ta-Ching Chen) wrote…

I suddenly found some problem I didn't notice for the record PR. You file an issue to address these problem later.

Lost some of the structure fields in original request.
For example, RawPath in url is not included.
https://golang.org/src/net/url/url.go?s=9021:9537#L323
type URL struct {
	Scheme     string
	Opaque     string    // encoded opaque data
	User       *Userinfo // username and password information
	Host       string    // host or host:port
	Path       string    // path (relative paths may omit leading slash)
	RawPath    string    // encoded path hint (see EscapedPath method)
	ForceQuery bool      // append a query ('?') even if RawQuery is empty
	RawQuery   string    // encoded query values, without '?'
	Fragment   string    // fragment for references, without '#'
}
We better to embed the whole url.Url object in the redisCache.Request directly.
Also, we lost some of fields in the original request structure like RemoteAddr Trailer . https://github.com/golang/go/blob/master/src/net/http/request.go#L106-L320

Payload should be in redisCache.Request not redisCache.Request.URL

In fact this was the first issue I filed under this project. You can view/add to it it here: #800. Yes, it is concerning that we are losing information but I was not able to find an existing standard Protobuf format that fully saves Golang HTTP requests.

As for point #2, I inspected some requests and saw that the Form and PostForm fields are almost never being stored (are always empty). I can remove those fields and add a new one called Payload in redisCache.Request, and save the payload there. Would that be better?

life1347 · 2018-08-17T18:13:29Z

controller/client/replayer.go

+
+	defer resp.Body.Close()
+
+	fmt.Println("Received: ", resp.Status)


remove this line

life1347 · 2018-08-17T18:22:32Z

redis/redisApi.go

+	if request.Method == http.MethodGet {
+		req, err = http.NewRequest("GET", targetUrl, nil)
+		if err != nil {
+			return []string{}, err


return nil should be enough (here and other return)

Iterating iterating solidifying replayer

Amusement

Reviewable status: 0 of 7 files reviewed, 9 unresolved discussions (waiting on @life1347)

redis/redisApi.go, line 395 at r2 (raw file):

Previously, life1347 (Ta-Ching Chen) wrote…

return nil should be enough (here and other return)

Done.

smruthi2187 · 2018-08-17T20:17:31Z

From @Amusement and @life1347 's discussion, this is good to merge! 🎉

life1347 reviewed Aug 16, 2018

View reviewed changes

Amusement commented Aug 16, 2018

View reviewed changes

life1347 reviewed Aug 17, 2018

View reviewed changes

controller/client/replayer.go Outdated

defer resp.Body.Close()

fmt.Println("Received: ", resp.Status)

Copy link

Member

life1347 Aug 17, 2018

Choose a reason for hiding this comment

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

remove this line

life1347 reviewed Aug 17, 2018

View reviewed changes

Amusement added 14 commits August 17, 2018 11:37

Partial replay

66866fb

Iterating iterating solidifying replayer

Missing replay function

a99fae1

sort imports

fa51a00

run travis

37ee8cd

gofmt

26c043f

Minor cleanup

cddfe1a

revert travis

2d3c25e

Reduce duplicate code

7422359

Ta ching comments

6a3fa9b

More licenses; gofmt

38e83a4

More licenses

9af8888

Use nils

d8df6c2

Quotes

fcacad9

stragglers

8d2bb3f

Amusement force-pushed the replay-only branch from 54d9c67 to 8d2bb3f Compare August 17, 2018 18:40

Amusement commented Aug 17, 2018

View reviewed changes

smruthi2187 merged commit 95eef49 into master Aug 17, 2018

life1347 deleted the replay-only branch August 18, 2018 04:02

garyyeap pushed a commit to garyyeap/fission that referenced this pull request Sep 14, 2018

Replay recorded requests by ReqUID (fission#864)

8de132d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Replay recorded requests by ReqUID #864

Replay recorded requests by ReqUID #864

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!


		defer resp.Body.Close()

		fmt.Println("Received: ", resp.Status)

Replay recorded requests by ReqUID #864

Replay recorded requests by ReqUID #864

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!